Re: [PULL 05/11] ui/console: Use qemu_dmabuf_get_..() helpers instead

[Richard Henderson]

On 5/15/24 14:42, Cédric Le Goater wrote:
> On 5/15/24 14:38, Richard Henderson wrote:
> > On 5/14/24 15:17, marcandre.lureau@redhat.com wrote:
> > > --- a/ui/gtk-egl.c
> > > +++ b/ui/gtk-egl.c
> > > @@ -70,6 +70,7 @@ void gd_egl_draw(VirtualConsole *vc)
> > >       QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> > >   #endif
> > >       int ww, wh, ws;
> > > +    int fence_fd;
> > >       if (!vc->gfx.gls) {
> > >           return;
> > > @@ -83,7 +84,7 @@ void gd_egl_draw(VirtualConsole *vc)
> > >       if (vc->gfx.scanout_mode) {
> > >   #ifdef CONFIG_GBM
> > >           if (dmabuf) {
> > > -            if (!dmabuf->draw_submitted) {
> > > +            if (!qemu_dmabuf_get_draw_submitted(dmabuf)) {
> > >                   return;
> > >               } else {
> > >                   dmabuf->draw_submitted = false;
> > > @@ -99,8 +100,9 @@ void gd_egl_draw(VirtualConsole *vc)
> > >   #ifdef CONFIG_GBM
> > >           if (dmabuf) {
> > >               egl_dmabuf_create_fence(dmabuf);
> > > -            if (dmabuf->fence_fd >= 0) {
> > > -                qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, 
> > > vc);
> > > +            fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
> > > +            if (fence_fd >= 0) {
> > > +                qemu_set_fd_handler(fence_fd, gd_hw_gl_flushed, NULL, vc);
> > >                   return;
> > >               }
> > >               graphic_hw_gl_block(vc->gfx.dcl.con, false);
> >
> > If !CONFIG_GBM, this causes
> >
> > ../src/ui/gtk-egl.c: In function ‘gd_egl_draw’:
> > ../src/ui/gtk-egl.c:73:9: error: unused variable ‘fence_fd’ 
> > [-Werror=unused-variable]
> >     73 |     int fence_fd;
> >        |         ^~~~~~~~
> > cc1: all warnings being treated as errors
>
> I sent :
>
> 20240515100520.574383-1-clg@redhat.com/">https://lore.kernel.org/qemu-devel/20240515100520.574383-1-clg@redhat.com/

Ah, missed that.  Thanks.

> > There is no reason to have the declaration at the top, rather than here in this 
> > block.
>
> It could be done that way too but since we need dmabuf, it might
> as well be declared at the top.

Surely it is clearly better to limit a variable's scope to it's use, even if there is a 
convenient ifdef nearby at the top of the function...


r~