Hi Daniel, On Tue, 22 Mar 2016 11:20:47 +0000 Daniel Stone <[email protected]> wrote:
> Hi, > > On 21 March 2016 at 16:41, Miguel A. Vico <[email protected]> > wrote: > > As previously stated, EGLDevice and EGLOutput will provide means > > to access native device objects and different portions of display > > control hardware respectively. > > > > Whenever EGL_EXT_device_drm extension is present, EGLDevice can > > be used to enumerate and access DRM KMS devices, and EGLOutputLayer > > to enumerate and access DRM KMS crtcs and planes. > > EGLDevice isn't enumerating devices though: you still use udev to > enumerate DRM devices, and then walk the list of EGLDevices to find > the device you've already settled on. EGL_DRM_MASTER_FD_EXT then gets > used to inject an open connection back in to replace the device you've > already opened. Messy. :\ You're right. I tried to keep compositor-drm as unchanged as possible. EGLDevice itself doesn't open any connection with the device though. It's not until EGLDisplay creation that the connection is opened. EGL_DRM_MASTER_FD_EXT let's us provide EGL with a master FD so certain operations requiring of master permission can be performed. We could do the other way around as well: enumerate devices with EGLDevice, pick the desired one and use it's filename to open a drm device, then create an EGL display connection. > > > By using EGLStreams and attaching an EGLOutputLayer consumer > > (representing a DRM KMS crtc or plane) to it, compositor-drm can > > produce final composition frames and present them on a DRM device. > > It's gl-renderer which produces the frames, really. Correct. > > > This change adds required logic to support presentation through > > EGLDevice+EGLOutput+EGLStream. Whether GBM or EGLDevice should be > > used can be controlled by --use-egldevice backend argument. > > Should this not be automatic, i.e. use gbm if platform_gbm is present > and EGLDevice if gbm isn't present but the other extensions are? I wanted to be symmetrical to what's currently done with --use-pixman. If we ever have a driver supporting both GBM and EGLStream implementations, we might want to be able to switch between them at runtime. > > > @@ -531,17 +540,21 @@ drm_output_render_gl(struct drm_output > > *output, pixman_region32_t *damage) > > output->base.compositor->renderer->repaint_output(&output->base, > > damage); > > > > - bo = gbm_surface_lock_front_buffer(output->gbm_surface); > > - if (!bo) { > > - weston_log("failed to lock front buffer: %m\n"); > > - return; > > - } > > + if (b->use_egldevice) > > + output->next = output->dumb[0]; > > Huh? > > I'm assuming you use two (never actually used) dumb BOs as > output->{current,next} to fit in with compositor-drm's bookkeeping, > whilst Streams handles the actual buffer management for you, right? > That seems like quite a hack. Right. > > > /* When initializing EGL, if the preferred buffer format isn't > > available > > * we may be able to susbstitute an ARGB format for an XRGB one. > > This seems like it doesn't apply > changed the logic to al What do you mean? > > @@ -1594,38 +1642,59 @@ fallback_format_for(uint32_t format) > > static int > > drm_backend_create_gl_renderer(struct drm_backend *b) > > { > > - EGLint format[3] = { > > + EGLint platform_attribs[] = { > > + EGL_DRM_MASTER_FD_EXT, b->drm.fd, > > + EGL_NONE > > + }; > > Seems like this should be called device_platform_attribs, since > they're only used in the EGLDevice case, not gbm. Will apply your suggestion. > > > + EGLint format[] = { > > b->gbm_format, > > fallback_format_for(b->gbm_format), > > - 0, > > + 0 > > Unrelated changes to the format array here. True, will undo. > > > + if (b->use_egldevice) { > > + b->egldevice = create_egldevice(b->drm.filename); > > + if (b->egldevice == EGL_NO_DEVICE_EXT) > > + return -1; > > + } else { > > + b->gbm = create_gbm_device(b->drm.fd); > > + if (!b->gbm) > > + return -1; > > + } > > + > > if (drm_backend_create_gl_renderer(b) < 0) { > > - gbm_device_destroy(b->gbm); > > + if (b->gbm) > > + gbm_device_destroy(b->gbm); > > Shouldn't the egldevice be destroyed here too? EGLDevice is only used for enumeration purposes, no connection with the native device is actually opened, so no need for destruction whatsoever. I see now that "create_egldevice()" might be a misleading name for that function. Probably something like "find_egldevice()" would be better. > > > + if (b->use_egldevice) { > > + int w = output->base.current_mode->width; > > + int h = output->base.current_mode->height; > > > > - if (format[1]) > > - n_formats = 2; > > - if (gl_renderer->output_window_create(&output->base, > > - > > (EGLNativeWindowType)output->gbm_surface, > > - output->gbm_surface, > > - gl_renderer->opaque_attribs, > > - format, > > - n_formats) < 0) { > > - weston_log("failed to create gl renderer output > > state\n"); > > - gbm_surface_destroy(output->gbm_surface); > > - return -1; > > - } > > + /* Create a dumb fb for modesetting */ > > + output->dumb[0] = drm_fb_create_dumb(b, w, h); > > + if (!output->dumb[0]) { > > + weston_log("failed to create dumb > > framebuffer\n"); > > + return -1; > > + } > > Seems I was correct earlier - if it's being passed to drmModeSetCrtc > though, this will mean that we flash up some completely unknown > content when starting. Not pretty. Agree. As we discussed over IRC, we expect to solve this issue as soon as we have atomic support. > > > + } else { > > + EGLint format[2] = { > > + output->gbm_format, > > + fallback_format_for(output->gbm_format), > > + }; > > + int i, flags, n_formats = 1; > > + > > + output->gbm_surface = gbm_surface_create(b->gbm, > > + > > output->base.current_mode->width, > > + > > output->base.current_mode->height, > > + format[0], > > + GBM_BO_USE_SCANOUT | > > + GBM_BO_USE_RENDERING); > > + if (!output->gbm_surface) { > > + weston_log("failed to create gbm > > surface\n"); > > + return -1; > > + } > > > > - output->gbm_cursor_bo[i] = > > - gbm_bo_create(b->gbm, b->cursor_width, > > b->cursor_height, > > - GBM_FORMAT_ARGB8888, flags); > > - } > > + if (format[1]) > > + n_formats = 2; > > This needs to be rebased; 6d556374 changed the format-selection logic. Sure. Thanks. > > Cheers, > Daniel -- Miguel NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361 Managing Director: Karen Theresa Burns ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
