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

[Emil Velikov]

Hi Stan,

First, thanks for re-spinning these according to my suggestions.

On 20 May 2016 at 12:17, 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.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov at 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.

> ---
>  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 ?


> --- 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) ?


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


And last but not least you need a hunk for r300.


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