| Message ID | 20191122083044.6627-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace hibmc code with generic implmentation | expand |
Hi Thomas. Change looks good. If you spin this could you move the changes to generic drm code to a separate patch? As it is now it is hidden for most. But then there are also no users of drm_gem_vram_fill_create_dumb() One small nit below that you can safely ignore :-) Sam 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> > --- > 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 > * @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); Semi related change... > + if (pitch_align) > + pitch = ALIGN(pitch, pitch_align); > 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
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. > * @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 >
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. BTW What's the DRM interface's behavior wrt interruption? For example, can a ioctl call like CREATE_DUMB return EINTR to userspace? 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 >> >
Hi thanks for the review. Am 23.11.19 um 09:56 schrieb Sam Ravnborg: > Hi Thomas. > > Change looks good. If you spin this could you move the > changes to generic drm code to a separate patch? > As it is now it is hidden for most. > But then there are also no users of drm_gem_vram_fill_create_dumb() I just posted a patchset for mgag200 that uses this function. Maybe it'll have to stay. Best regards Thomas > > One small nit below that you can safely ignore :-) > > > Sam > > > 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> >> --- >> 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 >> * @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); > Semi related change... > >> + if (pitch_align) >> + pitch = ALIGN(pitch, pitch_align); >> 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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 >
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 * @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); 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);
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> --- 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(-)