On Mon, May 16, 2016 at 3:53 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 15 May 2016 at 12:34, Stanimir Varbanov <stanimir.varba...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
