Hi,
On 15 February 2018 at 22:44, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Thu, Feb 15, 2018 at 7:57 AM, Daniel Stone <dani...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
