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.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.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varba...@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. 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?
Meanwhile I ran some ext_image_dma_buf_import piglit tests, so I can add
testcase for offset != 0 there.
>
>> ---
>> src/gallium/drivers/nouveau/nouveau_screen.c | 6 ++++++
>> src/gallium/drivers/vc4/vc4_screen.c | 7 +++++++
>> src/gallium/state_trackers/dri/dri2.c | 7 ++-----
>> src/gallium/winsys/i915/drm/i915_drm_buffer.c | 3 +++
>> src/gallium/winsys/intel/drm/intel_drm_winsys.c | 3 +++
>> src/gallium/winsys/svga/drm/vmw_screen_dri.c | 12 ++++++++++++
>> src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 2 ++
>> src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 6 ++++++
>> 8 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
>> b/src/gallium/drivers/nouveau/nouveau_screen.c
>> index 4ca9e5c06cde..2c421cc748c1 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
>> @@ -89,6 +89,12 @@ nouveau_screen_bo_from_handle(struct pipe_screen *pscreen,
>> struct nouveau_bo *bo = 0;
>> int ret;
>>
>> + if (whandle->offset != 0) {
>> + debug_printf("%s: attempt to import unsupported winsys offset %d\n",
>> + __FUNCTION__, whandle->offset);
> Nicely done on adding these warnings. This way devs/users will have
> nice feedback where/how to fix things.
>
>
>> 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
>
>> @@ -1056,8 +1054,6 @@ dri2_from_names(__DRIscreen *screen, int width, int
>> height, int format,
>>
>> if (num_names != 1)
>> return NULL;
>> - if (offsets[0] != 0)
>> - return NULL;
>>
>> format = convert_fourcc(format, &dri_components);
>> if (format == -1)
>> @@ -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.
>
>
>> --- a/src/gallium/winsys/i915/drm/i915_drm_buffer.c
>> +++ b/src/gallium/winsys/i915/drm/i915_drm_buffer.c
>> @@ -101,6 +101,9 @@ i915_drm_buffer_from_handle(struct i915_winsys *iws,
>> if ((whandle->type != DRM_API_HANDLE_TYPE_SHARED) && (whandle->type !=
>> DRM_API_HANDLE_TYPE_FD))
>> return NULL;
>>
>> + if (whandle->offset != 0)
>> + return NULL;
>> +
>> buf = CALLOC_STRUCT(i915_drm_buffer);
>> if (!buf)
>> return NULL;
>> diff --git a/src/gallium/winsys/intel/drm/intel_drm_winsys.c
>> b/src/gallium/winsys/intel/drm/intel_drm_winsys.c
>> index 1c5aabe33044..d4fe88ff73d5 100644
>> --- a/src/gallium/winsys/intel/drm/intel_drm_winsys.c
>> +++ b/src/gallium/winsys/intel/drm/intel_drm_winsys.c
>> @@ -313,6 +313,9 @@ intel_winsys_import_handle(struct intel_winsys *winsys,
>> drm_intel_bo *bo;
>> int err;
>>
>> + if (handle->offset != 0)
>> + return NULL;
>> +
> Worth adding the warning message for these two (intel and i915) ?
Sure, I just tried to keep the codding style on every file, and this one
is not so verbose as the others.
>
>
>> --- 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;
}
t
>
> 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.
>
>
> --- 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.
>
> Thanks
> Emil
>
regards,
Stan
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
