[Mesa-dev] [RFC 01/22] RFC: egl/x11: Support DRI3 v1.1

[Louis-Francis Ratté-Boulianne]

Hi,

On Tue, 2017-06-20 at 15:19 +0100, Emil Velikov wrote:
> > +            for (i = 0; i < count; i++) {
> > +               modifiers[i] = (uint64_t) mod_parts[i * 2] << 32;
> > +               modifiers[i] |= (uint64_t) mod_parts[i * 2 + 1] &
> > 0xffffff;
> > +            }
> > +         }
> > +
> > +         free(mod_reply);
> > +
> > +         buffer->image = draw->ext->image-
> > >createImageWithModifiers(draw->dri_screen,
> > +                                                                  
> >   width, height,
> > +                                                                  
> >   format,
> > +                                                                  
> >   modifiers, count,
> > +                                                                  
> >   buffer);
> > +         free(modifiers);
> > +      }
> > +#endif
> > +
> > +      if (!buffer->image)
> 
> Does not align with all the error paths above. There we bail out, yet
> here we fall-back to the old extension.
> Perhaps change the former to come here? One could even more that
> whole
> hunk into a separate function.

The rationale is that we only try to create a buffer with the supported
modifiers. If it doesn't work, it's still sensible to try the old path
as it's better to have a DRI image without any optimization than none.

> +   }
> > 
> > -   xcb_dri3_pixmap_from_buffer(draw->conn,
> > -                               (pixmap = xcb_generate_id(draw-
> > >conn)),
> > -                               draw->drawable,
> > -                               buffer->size,
> > -                               width, height, buffer->pitch,
> > -                               depth, buffer->cpp * 8,
> > -                               buffer_fd);
> > +#if XCB_DRI3_MAJOR_VERSION > 1 || XCB_DRI3_MINOR_VERSION >= 1
> > +   if (draw->multiplanes_available) {
> 
> This else looks a bit odd. If we fail to manage multiple buffers
> above, multiplanes_available will still be true, yet we could have a
> DRIImage.
> We should track that (modify multiplanes_available/other) and act
> accordingly here.

What can fail above (and not be critical) is creating an image with
modifiers. I grant you that it means the resulting image will only have
one plane, but that doesn't make it "bad" to use
xcb_dri3_pixmap_from_buffers. multiplanes_available simply means that
the X server actually supports using multiple planes.

The rest of your comments have been addressed and I will post the new
RFC (v2) soon. Thank you for the review.

--
Louis-Francis