[Mesa-dev] [PATCH v2 3/3] gallium: push offset down to driver

[Emil Velikov]

On 24 May 2016 at 11:33, Stanimir Varbanov <stanimir.varbanov at linaro.org> wrote:
> Hi Emil,
>
> On 05/21/2016 11:20 AM, Emil Velikov wrote:
>> Hi Stan,
>>
>> First, thanks for re-spinning these according to my suggestions.
>
> Thanks for the comments!
>
>>
>> On 20 May 2016 at 12:17, Stanimir Varbanov <stanimir.varbanov at linaro.org> wrote:
>>> Push offset down to drivers when importing dmabuf. This is needed
>>> to more fully support EGL_EXT_image_dma_buf_import when a non-zero
>>> offset is specified.
>>>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov at linaro.org>
>> Please reiterate (repeat) how you've tested the patch in the commit message.
>>
>> While mesa devs are split about the location of the revision log
>> (before or after the --- line) one thing we all agree upon,
>> adding/copying this information within the specific patch is highly
>> preferable.
>
> I mentioned in the cover letter that I tested the patch set on freedreno
> only, the other drivers are compile tested only.
Sadly the cover letter is not visible via git log which is why I
suggested adding/copying that information here.
And yes there is the argument that one find the cover letter in X
months/years, yet it's not as easy/convenient.

> I'm still searching a
> way to test the patch set on my laptop with i965. As can be expected the
> Ubuntu 16.04 loads the classical i965_dri driver, so I need a way to
> load the Gallium one if it has support for it of course. Do you have
> some pointers how this could be done?)
>
Personally I won't bother. With the commit message being clear "tested
on XX" everyone else can follow up and check their own driver. When
they get around to it ;-)

> Meanwhile I ran some ext_image_dma_buf_import piglit tests, so I can add
> testcase for offset != 0 there.
>
Please send any patches that you have to the piglit ML, if you haven't already.

>>> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
>>> index 5c2c01dca526..6ff3fce51222 100644
>>> --- a/src/gallium/state_trackers/dri/dri2.c
>>> +++ b/src/gallium/state_trackers/dri/dri2.c

>>> @@ -1067,6 +1063,7 @@ dri2_from_names(__DRIscreen *screen, int width, int height, int format,
>>>     whandle.type = DRM_API_HANDLE_TYPE_SHARED;
>>>     whandle.handle = names[0];
>>>     whandle.stride = strides[0];
>>> +   whandle.offset = offsets[0];
>>>
>> This feels like a new addition. I take it that not all the corner
>> cases were tested with v1 and it has gone unnoticed ?
>
> Yep, it is new addition compared with the first version of the patch
> set, I add it for completeness, but now I think it should be subject of
> separate patch.
>
What I'm wondering is how anything was working without it. Afaics the
offset was set (implicitly via memset) to zero so from a driver POV
nothing was changed.


>>> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>> @@ -266,6 +266,8 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
>>>     assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
>>>            whandle->type == DRM_API_HANDLE_TYPE_FD);
>>>
>>> +   assert(whandle->offset == 0);
>>> +
>> Please make this an if statement so that things don't crash/misrender
>> for the gnome (and other) folk.
>
> Did you mean:
>
> if (whandle->offset != 0) {
>         debug_print();
>         return NULL;
> }
>
Yes. There is a real chance of hitting this, so we should return
instead of assert.


>>
>> And last but not least you need a hunk for r300.
>
> I had the impression that the offset != 0 shouldn't be an issue for
> r300, r600 and radeon from the comment from Christian König on the v1. I
> might be wrong of course.
>
Almost... if you take a closer look - r600 and radeonsi share a lot of
code (drivers/radeon). In there (r600_texture_from_handle), the winsys
call (buffer_from_handle) provides storage for the offset value as it
makes use of it. In the r300 case (r300_texture_from_handle) there is
none, as it lacks the implementation (handling of offset != 0). One
day as r300 becomes capable we can drop the proposed hunk (below).
Until then it should stay imho.

>>
>>
>> --- a/src/gallium/winsys/radeon/drm/radeon_drm_bo.c
>> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_bo.c
>> @@ -859,6 +859,12 @@ static struct pb_buffer
>> *radeon_winsys_bo_from_handle(struct radeon_winsys *rws,
>>     unsigned handle;
>>     uint64_t size = 0;
>>
>> +    if (!offset && whandle->offset != 0) {
>> +        fprintf(stderr, "attempt to import unsupported winsys offset %u\n",
>> +                whandle->offset);
>> +        return NULL;
>> +    }
>> +
>>
>>
>> AMDGPU could in theory use the same, although since all its callers
>> explicitly provide 'offset' it's not a big deal either way. So I won't
>> bother with it for now.
>>
Adding the above for (in) AMDGPU is a paranoia check. So unless
you/others insist, just don't bother. I've mentioned it only for
completeness.

Regards,
Emil