| Message ID | 20200120164343.2262-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm] intel: drm_intel_bo_gem_create_from_* on platforms w/o HW tiling | expand |
On Mon, 20 Jan 2020 at 16:43, Imre Deak <imre.deak@intel.com> wrote: > > Platforms without a HW detiler doesn't support the get_tiling IOCTL. > Fix the drm_intel_bo_gem_create_from_* functions assuming the default > no-tiling, no-swizzling setting for the GEM buffer in this case. > > Signed-off-by: Imre Deak <imre.deak@intel.com> Seems like it this can happen only in vgpu cases.... which explains why it was not raised earlier. Regardless: Reviewed-by: Emil Velikov <emil.velikov@collabora.com> Aside: might want to do similar patch for mesa. Be that for classic/i965, gallium/iris and/or the Vulkan ANV driver. -Emil
On Monday, 2020-01-20 18:43:43 +0200, Imre Deak wrote: > Platforms without a HW detiler doesn't support the get_tiling IOCTL. > Fix the drm_intel_bo_gem_create_from_* functions assuming the default > no-tiling, no-swizzling setting for the GEM buffer in this case. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > intel/intel_bufmgr_gem.c | 42 +++++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > index fbf48730..fc249ef1 100644 > --- a/intel/intel_bufmgr_gem.c > +++ b/intel/intel_bufmgr_gem.c > @@ -1069,6 +1069,27 @@ check_bo_alloc_userptr(drm_intel_bufmgr *bufmgr, > tiling_mode, stride, size, flags); > } > > +static int get_tiling_mode(drm_intel_bufmgr_gem *bufmgr_gem, > + uint32_t gem_handle, > + uint32_t *tiling_mode, > + uint32_t *swizzle_mode) > +{ > + struct drm_i915_gem_get_tiling get_tiling; > + int ret; > + > + memclear(get_tiling); > + ret = drmIoctl(bufmgr_gem->fd, > + DRM_IOCTL_I915_GEM_GET_TILING, > + &get_tiling); You're missing `get_tiling.handle = gem_handle;` Or better yet, just initialise `get_tiling` and get rid of the memclear(): struct drm_i915_gem_get_tiling get_tiling = { .handle = gem_handle, }; With either fix: Reviewed-by: Eric Engestrom <eric@engestrom.ch> FYI, I've posted the following MR for the equivalent Mesa changes: https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3497 > + if (ret != 0 && errno != EOPNOTSUPP) > + return ret; > + > + *tiling_mode = get_tiling.tiling_mode; > + *swizzle_mode = get_tiling.swizzle_mode; > + > + return 0; > +} > + > /** > * Returns a drm_intel_bo wrapping the given buffer object handle. > * > @@ -1084,7 +1105,6 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, > drm_intel_bo_gem *bo_gem; > int ret; > struct drm_gem_open open_arg; > - struct drm_i915_gem_get_tiling get_tiling; > > /* At the moment most applications only have a few named bo. > * For instance, in a DRI client only the render buffers passed > @@ -1146,16 +1166,11 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, > HASH_ADD(name_hh, bufmgr_gem->name_table, > global_name, sizeof(bo_gem->global_name), bo_gem); > > - memclear(get_tiling); > - get_tiling.handle = bo_gem->gem_handle; > - ret = drmIoctl(bufmgr_gem->fd, > - DRM_IOCTL_I915_GEM_GET_TILING, > - &get_tiling); > + ret = get_tiling_mode(bufmgr_gem, bo_gem->gem_handle, > + &bo_gem->tiling_mode, &bo_gem->swizzle_mode); > if (ret != 0) > goto err_unref; > > - bo_gem->tiling_mode = get_tiling.tiling_mode; > - bo_gem->swizzle_mode = get_tiling.swizzle_mode; > /* XXX stride is unknown */ > drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0); > DBG("bo_create_from_handle: %d (%s)\n", handle, bo_gem->name); > @@ -2634,7 +2649,6 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s > int ret; > uint32_t handle; > drm_intel_bo_gem *bo_gem; > - struct drm_i915_gem_get_tiling get_tiling; > > pthread_mutex_lock(&bufmgr_gem->lock); > ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle); > @@ -2688,15 +2702,11 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s > bo_gem->has_error = false; > bo_gem->reusable = false; > > - memclear(get_tiling); > - get_tiling.handle = bo_gem->gem_handle; > - if (drmIoctl(bufmgr_gem->fd, > - DRM_IOCTL_I915_GEM_GET_TILING, > - &get_tiling)) > + ret = get_tiling_mode(bufmgr_gem, handle, > + &bo_gem->tiling_mode, &bo_gem->swizzle_mode); > + if (ret) > goto err; > > - bo_gem->tiling_mode = get_tiling.tiling_mode; > - bo_gem->swizzle_mode = get_tiling.swizzle_mode; > /* XXX stride is unknown */ > drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0); >
On Tue, Jan 21, 2020 at 10:40:48PM +0000, Eric Engestrom wrote: > On Monday, 2020-01-20 18:43:43 +0200, Imre Deak wrote: > > Platforms without a HW detiler doesn't support the get_tiling IOCTL. > > Fix the drm_intel_bo_gem_create_from_* functions assuming the default > > no-tiling, no-swizzling setting for the GEM buffer in this case. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > intel/intel_bufmgr_gem.c | 42 +++++++++++++++++++++++++--------------- > > 1 file changed, 26 insertions(+), 16 deletions(-) > > > > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > > index fbf48730..fc249ef1 100644 > > --- a/intel/intel_bufmgr_gem.c > > +++ b/intel/intel_bufmgr_gem.c > > @@ -1069,6 +1069,27 @@ check_bo_alloc_userptr(drm_intel_bufmgr *bufmgr, > > tiling_mode, stride, size, flags); > > } > > > > +static int get_tiling_mode(drm_intel_bufmgr_gem *bufmgr_gem, > > + uint32_t gem_handle, > > + uint32_t *tiling_mode, > > + uint32_t *swizzle_mode) > > +{ > > + struct drm_i915_gem_get_tiling get_tiling; > > + int ret; > > + > > + memclear(get_tiling); > > + ret = drmIoctl(bufmgr_gem->fd, > > + DRM_IOCTL_I915_GEM_GET_TILING, > > + &get_tiling); > > You're missing `get_tiling.handle = gem_handle;` Err, thanks for catching that, will resend fixing it. > Or better yet, just initialise `get_tiling` and get rid of the memclear(): > struct drm_i915_gem_get_tiling get_tiling = { > .handle = gem_handle, > }; > > With either fix: > Reviewed-by: Eric Engestrom <eric@engestrom.ch> > > FYI, I've posted the following MR for the equivalent Mesa changes: > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3497 > > > + if (ret != 0 && errno != EOPNOTSUPP) > > + return ret; > > + > > + *tiling_mode = get_tiling.tiling_mode; > > + *swizzle_mode = get_tiling.swizzle_mode; > > + > > + return 0; > > +} > > + > > /** > > * Returns a drm_intel_bo wrapping the given buffer object handle. > > * > > @@ -1084,7 +1105,6 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, > > drm_intel_bo_gem *bo_gem; > > int ret; > > struct drm_gem_open open_arg; > > - struct drm_i915_gem_get_tiling get_tiling; > > > > /* At the moment most applications only have a few named bo. > > * For instance, in a DRI client only the render buffers passed > > @@ -1146,16 +1166,11 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, > > HASH_ADD(name_hh, bufmgr_gem->name_table, > > global_name, sizeof(bo_gem->global_name), bo_gem); > > > > - memclear(get_tiling); > > - get_tiling.handle = bo_gem->gem_handle; > > - ret = drmIoctl(bufmgr_gem->fd, > > - DRM_IOCTL_I915_GEM_GET_TILING, > > - &get_tiling); > > + ret = get_tiling_mode(bufmgr_gem, bo_gem->gem_handle, > > + &bo_gem->tiling_mode, &bo_gem->swizzle_mode); > > if (ret != 0) > > goto err_unref; > > > > - bo_gem->tiling_mode = get_tiling.tiling_mode; > > - bo_gem->swizzle_mode = get_tiling.swizzle_mode; > > /* XXX stride is unknown */ > > drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0); > > DBG("bo_create_from_handle: %d (%s)\n", handle, bo_gem->name); > > @@ -2634,7 +2649,6 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s > > int ret; > > uint32_t handle; > > drm_intel_bo_gem *bo_gem; > > - struct drm_i915_gem_get_tiling get_tiling; > > > > pthread_mutex_lock(&bufmgr_gem->lock); > > ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle); > > @@ -2688,15 +2702,11 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s > > bo_gem->has_error = false; > > bo_gem->reusable = false; > > > > - memclear(get_tiling); > > - get_tiling.handle = bo_gem->gem_handle; > > - if (drmIoctl(bufmgr_gem->fd, > > - DRM_IOCTL_I915_GEM_GET_TILING, > > - &get_tiling)) > > + ret = get_tiling_mode(bufmgr_gem, handle, > > + &bo_gem->tiling_mode, &bo_gem->swizzle_mode); > > + if (ret) > > goto err; > > > > - bo_gem->tiling_mode = get_tiling.tiling_mode; > > - bo_gem->swizzle_mode = get_tiling.swizzle_mode; > > /* XXX stride is unknown */ > > drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0); > >
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index fbf48730..fc249ef1 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -1069,6 +1069,27 @@ check_bo_alloc_userptr(drm_intel_bufmgr *bufmgr, tiling_mode, stride, size, flags); } +static int get_tiling_mode(drm_intel_bufmgr_gem *bufmgr_gem, + uint32_t gem_handle, + uint32_t *tiling_mode, + uint32_t *swizzle_mode) +{ + struct drm_i915_gem_get_tiling get_tiling; + int ret; + + memclear(get_tiling); + ret = drmIoctl(bufmgr_gem->fd, + DRM_IOCTL_I915_GEM_GET_TILING, + &get_tiling); + if (ret != 0 && errno != EOPNOTSUPP) + return ret; + + *tiling_mode = get_tiling.tiling_mode; + *swizzle_mode = get_tiling.swizzle_mode; + + return 0; +} + /** * Returns a drm_intel_bo wrapping the given buffer object handle. * @@ -1084,7 +1105,6 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, drm_intel_bo_gem *bo_gem; int ret; struct drm_gem_open open_arg; - struct drm_i915_gem_get_tiling get_tiling; /* At the moment most applications only have a few named bo. * For instance, in a DRI client only the render buffers passed @@ -1146,16 +1166,11 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, HASH_ADD(name_hh, bufmgr_gem->name_table, global_name, sizeof(bo_gem->global_name), bo_gem); - memclear(get_tiling); - get_tiling.handle = bo_gem->gem_handle; - ret = drmIoctl(bufmgr_gem->fd, - DRM_IOCTL_I915_GEM_GET_TILING, - &get_tiling); + ret = get_tiling_mode(bufmgr_gem, bo_gem->gem_handle, + &bo_gem->tiling_mode, &bo_gem->swizzle_mode); if (ret != 0) goto err_unref; - bo_gem->tiling_mode = get_tiling.tiling_mode; - bo_gem->swizzle_mode = get_tiling.swizzle_mode; /* XXX stride is unknown */ drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0); DBG("bo_create_from_handle: %d (%s)\n", handle, bo_gem->name); @@ -2634,7 +2649,6 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s int ret; uint32_t handle; drm_intel_bo_gem *bo_gem; - struct drm_i915_gem_get_tiling get_tiling; pthread_mutex_lock(&bufmgr_gem->lock); ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle); @@ -2688,15 +2702,11 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s bo_gem->has_error = false; bo_gem->reusable = false; - memclear(get_tiling); - get_tiling.handle = bo_gem->gem_handle; - if (drmIoctl(bufmgr_gem->fd, - DRM_IOCTL_I915_GEM_GET_TILING, - &get_tiling)) + ret = get_tiling_mode(bufmgr_gem, handle, + &bo_gem->tiling_mode, &bo_gem->swizzle_mode); + if (ret) goto err; - bo_gem->tiling_mode = get_tiling.tiling_mode; - bo_gem->swizzle_mode = get_tiling.swizzle_mode; /* XXX stride is unknown */ drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0);
Platforms without a HW detiler doesn't support the get_tiling IOCTL. Fix the drm_intel_bo_gem_create_from_* functions assuming the default no-tiling, no-swizzling setting for the GEM buffer in this case. Signed-off-by: Imre Deak <imre.deak@intel.com> --- intel/intel_bufmgr_gem.c | 42 +++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-)