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

[Christian König]

Am 16.05.2016 um 22:16 schrieb Rob Clark:
> On Mon, May 16, 2016 at 3:53 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 15 May 2016 at 12:34, 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.
>>>
>> How did you test this, on which hardware ? Please add such information
>> to the commit message.
>>
>> My knowledge in the area is far from perfect, although it seems that
>> you're pushing the offset check as opposed to the actual offset.
>> Afaics there is nothing in between that makes use of it (the offset)
>> so this patch should be mostly a no-op. See below why "mostly".
>>
>> Perhaps you meant to nuke the in-drivers offset checking, or there's
>> some more work needed in the area ?
> jfwiw, freedreno already handles the offset, egl just needs to pass it
> down.  This is why you aren't seeing a hunk for freedreno.
>
> The checks to other drivers for offset!=0 would presumably be removed
> when they add support for non-zero offsets.  So IMO this change is
> correct, at least for the drivers the ignore the offset.
>
>>> --- a/src/gallium/drivers/radeon/r600_texture.c
>>> +++ b/src/gallium/drivers/radeon/r600_texture.c
>>> @@ -29,6 +29,7 @@
>>>   #include "util/u_format.h"
>>>   #include "util/u_memory.h"
>>>   #include "util/u_pack_color.h"
>>> +#include "state_tracker/drm_driver.h"
>> This does not look great. struct winsys_handle is supposed to stay
>> opaque for the driver. radeon/amdgpu is a nice example of
>> winsys/driver separation (grep for "_from_handle\>" to see how they've
>> done it) although I wonder if this is correct... read below.
>>
>> [And yes, we might want to address freedreno and vc4 and move the
>> winsys code from $driver_screen.c to gallium/winsys/$driver/ at some
>> point.]
> tbh, being portable to use freedreno (or probably vc4) on windows
> doesn't seem terribly important ;-)
>
> For r600/amdgpu we don't want to add this include.  For
> nouveau/vc4/freedreno it is ok.. they already ignore the winsys
> abstraction.
>
> (fwiw, for ilo the offset!=0 check should probably go in
> intel_winsys_import_handle())
>
>>>   #include <errno.h>
>>>   #include <inttypes.h>
>>>
>>> @@ -1113,6 +1114,9 @@ static struct pipe_resource *r600_texture_from_handle(struct pipe_screen *screen
>>>          struct radeon_bo_metadata metadata = {};
>>>          struct r600_texture *rtex;
>>>
>>> +       if (whandle->offset != 0)
>>> +               return NULL;
>>> +
>> Afaics some state trackers [1] can provide a a non-zero value and
>> things are likely to explode there.
>> Christian, is VDPAU dmabuf going to work with this in place ?
> yeah, looks like r600 does already handle the offset, so I think we
> could drop this hunk.  It might be the same for amdgpu, not sure.

Yes, both amdgpu as well as radeon already handle that.

And yes the offset is used for VDPAU dma-buf based interop and adding 
those checks would break that.

Christian.

>
> BR,
> -R
>
>
>> Thanks
>> Emil
>>
>> [1] src/mesa/state_tracker/st_vdpau.c