[Cogl] Feedback of wip/wayland-for-demo

[Jasper St. Pierre]

On Wed, Mar 12, 2014 at 11:55 AM, Neil Roberts <neil at linux.intel.com> wrote:

> Hi Jasper,
>
> Thanks for the patches on the wip/wayland-for-demo branch. I've pushed
> the first patch to the cogl-1.18 and master branches.
>
> https://git.gnome.org/browse/cogl/commit/?id=af9057d35f331e2c9509958f
>
> I have a couple of minor comments about the second patch:
>
> > diff --git a/cogl/winsys/cogl-winsys-egl-kms.c
> b/cogl/winsys/cogl-winsys-egl-kms.c
> > index 4af9848..41954ef 100644
> > --- a/cogl/winsys/cogl-winsys-egl-kms.c
> > +++ b/cogl/winsys/cogl-winsys-egl-kms.c
> > @@ -285,14 +285,21 @@ _cogl_winsys_renderer_connect (CoglRenderer
> *renderer,
> >     * we're doing here... */
> >    g_setenv ("EGL_PLATFORM", "drm", 1);
> >
> > -  kms_renderer->fd = open (device_name, O_RDWR);
> > -  if (kms_renderer->fd < 0)
> > +  if (renderer->kms_fd > 0)
>
> Shouldn't this be '>= 0'? I could imagine something potentially using
> fd 0 if it has closed its stdin.
>
> It would be nice if you could make it avoid closing the user's fd if it
> hits the close_fd label. I was going to say that it should also avoid
> closing the fd when the renderer is destroyed, but it looks that is
> missing anyway even if Cogl opens the device so we can leave that to a
> separate patch.
>

Good catches.


> Have you been able to test the last patch? When it returns from
> flip_all_crtcs it looks like flip->pending would still be zero which
> would make it immediately proceed to delete the framebuffer that it just
> set to scan out from. Won't that break?
>

I haven't ever tested it, as the only driver that doesn't have a native
flip is simpledrm, which is only used in pathological cases in virt. We
can't unconditionally bump flip->pending, since then we'll be stuck waiting
for a page flip event that will never come. I'll look into extracting the
"flip" logic into something we can use in both paths.

Anyway, the patch isn't too important, the really important one is the
ability to set a native KMS FD. So let's defer on this for now.

I think we would also need to make it queue the swap_notify_idle because
> Cogl is meant to have the invariant that whenever you call
> cogl_onscreen_swap_buffers you will always get a swap notification.
>

Yeah.


> Regards,
> - Neil
>



-- 
  Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/cogl/attachments/20140312/f3700f01/attachment.html>