Re: [PATCH 3/4] drm/hisilicon/hibmc: Implement hibmc_dumb_create() with generic helpers

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 26, 2019 at 08:40:27AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.11.19 um 10:14 schrieb Daniel Vetter:
> > On Fri, Nov 22, 2019 at 09:30:43AM +0100, Thomas Zimmermann wrote:
> >> The hibmc driver aligns scanlines to 16 bytes. Adding the pitch alignment
> >> as an argument to drm_gem_vram_fill_create_dumb() allows to use the generic
> >> implementation with hibmc. A value of 0 disables scanline pitches.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > I concur with Sam, the vram change should be split out.
> > 
> >> ---
> >>  drivers/gpu/drm/drm_gem_vram_helper.c         | 10 ++--
> >>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  4 --
> >>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 48 +------------------
> >>  include/drm/drm_gem_vram_helper.h             |  1 +
> >>  4 files changed, 10 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> >> index 666cb4c22bb9..f098784e7dfd 100644
> >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> >> @@ -485,6 +485,7 @@ EXPORT_SYMBOL(drm_gem_vram_vunmap);
> >>   * @dev:		the DRM device
> >>   * @bdev:		the TTM BO device managing the buffer object
> >>   * @pg_align:		the buffer's alignment in multiples of the page size
> >> + * @pitch_align:	the scanline's alignment in powers of 2
> >>   * @interruptible:	sleep interruptible if waiting for memory
> > 
> > I also noticed that no one sets this to true, neither here nor in
> > drm_gem_vram_create(). Maybe remove that too? Otherwise the argument list
> > becomes very unwielding. And you're already touching the (few) callers.
> 
> OK, I'll add this as a separate patch.

Yeah makes sense.

> BTW What's the DRM interface's behavior wrt interruption? For example,
> can a ioctl call like CREATE_DUMB return EINTR to userspace?

Yup. Everyone is required to use drmIoctl() for all drm ioctls, which
auto-restarts all syscalls when userspace sees a EINTR. We also generally
test that in igt (but maybe not for all the kms ioctls, at least not for
the dumb ones).

interruptible + igts using igt_while_interruptible is a fairly effective
way to exercise error paths in all kinds of places. Only trouble is that
if we introduce a new interruptible point somewhere we might run into
userspace that gets it wrong (e.g. dumb ioctls I think aren't
interruptible on most x86 drivers right now, so there might be a surprise
and we need to audit userspaces, including plymouth and all those).
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> >>   * @args:		the arguments as provided to \
> >>  				&struct drm_driver.dumb_create
> >> @@ -502,6 +503,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> >>  				  struct drm_device *dev,
> >>  				  struct ttm_bo_device *bdev,
> >>  				  unsigned long pg_align,
> >> +				  unsigned long pitch_align,
> >>  				  bool interruptible,
> >>  				  struct drm_mode_create_dumb *args)
> >>  {
> >> @@ -510,7 +512,9 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> >>  	int ret;
> >>  	u32 handle;
> >>  
> >> -	pitch = args->width * ((args->bpp + 7) / 8);
> >> +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> >> +	if (pitch_align)
> >> +		pitch = ALIGN(pitch, pitch_align);
> > 
> > Maybe throw a WARN_IS(is_pot(align)) in here?
> > 
> > Cheers, Daniel
> > 
> >>  	size = pitch * args->height;
> >>  
> >>  	size = roundup(size, PAGE_SIZE);
> >> @@ -612,8 +616,8 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
> >>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
> >>  		return -EINVAL;
> >>  
> >> -	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev, 0,
> >> -					     false, args);
> >> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> >> +					     0, 0, false, args);
> >>  }
> >>  EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
> >>  
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> >> index 8eb7258b236a..50a0c1f9d211 100644
> >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> >> @@ -18,7 +18,6 @@
> >>  #include <drm/drm_framebuffer.h>
> >>  
> >>  struct drm_device;
> >> -struct drm_gem_object;
> >>  
> >>  struct hibmc_drm_private {
> >>  	/* hw */
> >> @@ -41,9 +40,6 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
> >>  int hibmc_de_init(struct hibmc_drm_private *priv);
> >>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
> >>  
> >> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> >> -		     struct drm_gem_object **obj);
> >> -
> >>  int hibmc_mm_init(struct hibmc_drm_private *hibmc);
> >>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
> >>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> >> index f6d25b85c209..0af5d966a480 100644
> >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> >> @@ -47,55 +47,11 @@ void hibmc_mm_fini(struct hibmc_drm_private *hibmc)
> >>  	drm_vram_helper_release_mm(hibmc->dev);
> >>  }
> >>  
> >> -int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> >> -		     struct drm_gem_object **obj)
> >> -{
> >> -	struct drm_gem_vram_object *gbo;
> >> -	int ret;
> >> -
> >> -	*obj = NULL;
> >> -
> >> -	size = roundup(size, PAGE_SIZE);
> >> -	if (size == 0)
> >> -		return -EINVAL;
> >> -
> >> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
> >> -	if (IS_ERR(gbo)) {
> >> -		ret = PTR_ERR(gbo);
> >> -		if (ret != -ERESTARTSYS)
> >> -			DRM_ERROR("failed to allocate GEM object: %d\n", ret);
> >> -		return ret;
> >> -	}
> >> -	*obj = &gbo->bo.base;
> >> -	return 0;
> >> -}
> >> -
> >>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
> >>  		      struct drm_mode_create_dumb *args)
> >>  {
> >> -	struct drm_gem_object *gobj;
> >> -	u32 handle;
> >> -	int ret;
> >> -
> >> -	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 16);
> >> -	args->size = args->pitch * args->height;
> >> -
> >> -	ret = hibmc_gem_create(dev, args->size, false,
> >> -			       &gobj);
> >> -	if (ret) {
> >> -		DRM_ERROR("failed to create GEM object: %d\n", ret);
> >> -		return ret;
> >> -	}
> >> -
> >> -	ret = drm_gem_handle_create(file, gobj, &handle);
> >> -	drm_gem_object_put_unlocked(gobj);
> >> -	if (ret) {
> >> -		DRM_ERROR("failed to unreference GEM object: %d\n", ret);
> >> -		return ret;
> >> -	}
> >> -
> >> -	args->handle = handle;
> >> -	return 0;
> >> +	return drm_gem_vram_fill_create_dumb(file, dev, &dev->vram_mm->bdev,
> >> +					     0, 16, false, args);
> >>  }
> >>  
> >>  const struct drm_mode_config_funcs hibmc_mode_funcs = {
> >> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> >> index e040541a105f..c642b4cb6600 100644
> >> --- a/include/drm/drm_gem_vram_helper.h
> >> +++ b/include/drm/drm_gem_vram_helper.h
> >> @@ -113,6 +113,7 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> >>  				  struct drm_device *dev,
> >>  				  struct ttm_bo_device *bdev,
> >>  				  unsigned long pg_align,
> >> +				  unsigned long pitch_align,
> >>  				  bool interruptible,
> >>  				  struct drm_mode_create_dumb *args);
> >>  
> >> -- 
> >> 2.23.0
> >>
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel