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

[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.

BR,
-R


>
> Thanks
> Emil
>
> [1] src/mesa/state_tracker/st_vdpau.c