[Mesa-dev] [PATCH 10/11] egl/x11: Support DRI3 v1.1

[Daniel Stone]

Hi,

On 15 February 2018 at 22:44, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Thu, Feb 15, 2018 at 7:57 AM, Daniel Stone <daniels at collabora.com> wrote:
>> +   draw->ext->image->queryDmaBufModifiers(draw->dri_screen, format,
>> +                                          supported_modifiers_count,
>> +                                          supported_modifiers, NULL,
>> +                                          &supported_modifiers_count);
>> +
>> +   for (i = 0; i < supported_modifiers_count; i++) {
>> +      for (j = 0; j < count; j++) {
>> +         if (supported_modifiers[i] == modifiers[j]) {
>> +            free(supported_modifiers);
>> +            return true;
>> +         }
>> +      }
>> +   }
>> +
>> +   free(supported_modifiers);
>> +   return false;
>
>
> We could make the cleanup path a bit nicer if we did something like this:
>
> bool found = false;
> for (...) {
>    if (...) {
>       found = true;
>       break;
>    }
> }
>
> free(...);
> return found;
>
> That would mean we only have one free.  I don't really care all that much
> though as the current code is correct.

Seems an obvious and good cleanup.

>> +         if (mod_reply->num_drawable_modifiers) {
>> +            count = mod_reply->num_drawable_modifiers;
>> +            modifiers = malloc(count * sizeof(uint64_t));
>> +            if (!modifiers) {
>> +               free(mod_reply);
>> +               goto no_image;
>> +            }
>> +
>> +            memcpy(modifiers,
>> +
>> xcb_dri3_get_supported_modifiers_drawable_modifiers(mod_reply),
>> +                   count * sizeof(uint64_t));
>
>
> Dumb question, but why do we need to memcpy?  Can't we just pass these
> directly into createImageWithModifiers so long as we don't free mod_reply
> until after it returns?

Let me see if I can make that work without making the cleanup too
finicky. If so, no problem.

>> +#if XCB_DRI3_MAJOR_VERSION > 1 || (XCB_DRI3_MAJOR_VERSION == 1 &&
>> XCB_DRI3_MINOR_VERSION >= 1)
>> +   if (draw->multiplanes_available &&
>> +       buffer->modifier != DRM_FORMAT_MOD_INVALID) {
>
> I made a similar comment on the Wayland one but buffer->modifier != INVALID
> should imply multiplanes_available.  We should make multiplanes_available an
> assert.

I don't think that's true. If your winsys is not modifier-aware but
your driver is, you could, e.g., have !draw->multiplanes_available but
buffer->modifier == DRM_FORMAT_I915_MOD_X_TILED.

>> +#if XCB_DRI3_MAJOR_VERSION > 1 || (XCB_DRI3_MAJOR_VERSION == 1 &&
>> XCB_DRI3_MINOR_VERSION >= 1)
>> +   if (draw->multiplanes_available &&
>> +       draw->ext->image->base.version >= 15 &&
>> +       draw->ext->image->createImageFromDmaBufs2) {
>> +      xcb_dri3_buffers_from_pixmap_cookie_t bps_cookie;
>> +      xcb_dri3_buffers_from_pixmap_reply_t *bps_reply;
>> +
>> +      bps_cookie = xcb_dri3_buffers_from_pixmap(draw->conn, pixmap);
>> +      bps_reply = xcb_dri3_buffers_from_pixmap_reply(draw->conn,
>> bps_cookie,
>> +                                                     NULL);
>> +      if (!bps_reply)
>> +         goto no_image;
>> +      buffer->image =
>> +         loader_dri3_create_image_from_buffers(draw->conn, bps_reply,
>> format,
>> +                                               draw->dri_screen,
>> +                                               draw->ext->image,
>> +                                               buffer);
>> +      width = bps_reply->width;
>> +      height = bps_reply->height;
>> +      free(bps_reply);
>> +   } else
>> +#endif
>
>
> I really don't like mising preprocessor and C control-flow like this.  I
> made a suggestion on the previous version.  If there's nothing better to do,
> then I can live with it.

That's going to go before we land it. The #ifdefs are mainly there now
so that people can have these patches in their tree but not have a
hard-fail when they build without the experimental proto. When it
lands we'll just up the build deps.

Cheers,
Daniel