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

[Stanimir Varbanov]

Hi,

On 05/17/2016 10:31 AM, Christian König wrote:
> 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.
> 

My fault, I had to be more careful. Will look deeper in radeon and
amdgpu as well as vdpau. Thanks for catching the mistake.


-- 
regards,
Stan