Re: [PATCH v3 4/4] drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

[Yongbang Shi <shiyongbang@huawei.com>]

Hi Thomas.

>  
>  #include "hibmc_drm_drv.h"
> @@ -100,28 +102,41 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
>  static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_atomic_commit *state)
>  {
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(new_state);
>  	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	u32 gpu_addr = 0;
>  	u32 reg;
> -	s64 gpu_addr = 0;
>  	u32 line_l;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
> -	struct drm_gem_vram_object *gbo;
>  
> -	if (!new_state->fb)
> +	if (!fb)
>  		return;
>  
> -	gbo = drm_gem_vram_of_gem(new_state->fb->obj[0]);
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) == 0) {
> +		struct drm_rect damage;
> +		struct drm_atomic_helper_damage_iter iter;
> +
> +		drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
> +		drm_atomic_for_each_plane_damage(&iter, &damage) {
> +			struct iosys_map dst[DRM_FORMAT_MAX_PLANES] = {
> +				IOSYS_MAP_INIT_VADDR_IOMEM(priv->vram + gpu_addr),
> +			};
>  
> -	gpu_addr = drm_gem_vram_offset(gbo);
> -	if (WARN_ON_ONCE(gpu_addr < 0))
> -		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
> +			iosys_map_incr(&dst[0],
> +				       drm_fb_clip_offset(fb->pitches[0], fb->format, &damage));
> +			drm_fb_memcpy(dst, fb->pitches, shadow_plane_state->data, fb, &damage);
> +		}
> +
> +		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +	}

This implementation uses shadow planes for damage rect updates. If the entire plane is updated frequently, could this
lead to a performance drop or frame drops?

Originally, the window system(X11/Wayland) drew directly to the hardware framebuffer, but now there is an extra copy in
`atomic_update`.

>  
>  	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
>  
>  	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
>  
> -	line_l = new_state->fb->pitches[0];
> +	line_l = fb->pitches[0];
>  	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
>  	       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
>  	       priv->mmio + HIBMC_CRT_FB_WIDTH);
> @@ -149,13 +164,11 @@ static const struct drm_plane_funcs hibmc_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
>  	.destroy = drm_plane_cleanup,
> -	.reset = drm_atomic_helper_plane_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +	DRM_GEM_SHADOW_PLANE_FUNCS,
>  };
>  
>  static const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
> -	DRM_GEM_VRAM_PLANE_HELPER_FUNCS,
> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>  	.atomic_check = hibmc_plane_atomic_check,
>  	.atomic_update = hibmc_plane_atomic_update,
>  };
> @@ -515,6 +528,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv)
>  	}
>  
>  	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
> +	drm_plane_enable_fb_damage_clips(plane);
>  
>  	ret = drm_crtc_init_with_planes(dev, crtc, plane,
>  					NULL, &hibmc_crtc_funcs, NULL);
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 99b36de1fe13..19d3193e2f76 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -18,9 +18,10 @@
>  #include <drm/clients/drm_client_setup.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_drv.h>
> -#include <drm/drm_fbdev_ttm.h>
> +#include <drm/drm_dumb_buffers.h>
> +#include <drm/drm_fbdev_shmem.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> -#include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_module.h>
>  #include <drm/drm_vblank.h>
> @@ -71,7 +72,13 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
>  static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  			     struct drm_mode_create_dumb *args)
>  {
> -	return drm_gem_vram_fill_create_dumb(file, dev, 0, 128, args);
> +	int ret;
> +
> +	ret = drm_mode_size_dumb(dev, args, SZ_128, 0);
> +	if (ret)
> +		return ret;
> +
> +	return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
>  }
>  
>  static const struct drm_driver hibmc_driver = {
> @@ -81,10 +88,9 @@ static const struct drm_driver hibmc_driver = {
>  	.desc			= "hibmc drm driver",
>  	.major			= 1,
>  	.minor			= 0,
> -	.debugfs_init		= drm_vram_mm_debugfs_init,
> -	.dumb_create            = hibmc_dumb_create,
> -	.dumb_map_offset        = drm_gem_ttm_dumb_map_offset,
> -	DRM_FBDEV_TTM_DRIVER_OPS,
> +	.gem_prime_import	= drm_gem_shmem_prime_import_no_map,
> +	.dumb_create		= hibmc_dumb_create,
> +	DRM_FBDEV_SHMEM_DRIVER_OPS,
>  };
>  
>  static int __maybe_unused hibmc_pm_suspend(struct device *dev)
> @@ -106,11 +112,32 @@ static const struct dev_pm_ops hibmc_pm_ops = {
>  				hibmc_pm_resume)
>  };
>  
> +static enum drm_mode_status hibmc_mode_config_mode_valid(struct drm_device *dev,
> +							 const struct drm_display_mode *mode)
> +{
> +	const struct drm_format_info *info =
> +		drm_get_format_info(dev, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR);
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	unsigned long max_fb_size = priv->vram_size;
> +	u64 pitch;
> +
> +	if (drm_WARN_ON_ONCE(dev, !info))
> +		return MODE_ERROR; /* driver bug */
> +
> +	pitch = drm_format_info_min_pitch(info, 0, mode->hdisplay);
> +	if (!pitch)
> +		return MODE_BAD_WIDTH;
> +	else if (pitch > max_fb_size / mode->vdisplay)
> +		return MODE_MEM;
> +

I reviewed the AST and Cirrus implementations, and they include checks for the maximum size supported by the hardware.

The hardware FB width register has only 14 bits, and the HIBMC_CRT_FB_WIDTH_WIDTH_MASK macro defines the maximum size as
0x3FFF. Would adding the following check make the code more rigorous?

Also, the pitch requires 128-byte alignment. Should we add a check here, or does the buffer created by
`hibmc_dumb_create` already ensure that the FB start address is aligned to 128 bytes via `SZ_128`? I’m not entirely sure
about this.

Of course, the original code used `drm_vram_helper_mode_valid` without strictly checking these hardware limitations.
Thank you for your modification.


Thanks,
Yongbang.

> +	return MODE_OK;
> +}
> +
>  static const struct drm_mode_config_funcs hibmc_mode_funcs = {
> -	.mode_valid = drm_vram_helper_mode_valid,
> +	.mode_valid = hibmc_mode_config_mode_valid,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
> -	.fb_create = drm_gem_fb_create,
> +	.fb_create = drm_gem_fb_create_with_dirty,
>  };
>  
>  static int hibmc_kms_init(struct hibmc_drm_private *priv)
> @@ -130,7 +157,6 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>  	dev->mode_config.max_height = 1200;
>  
>  	dev->mode_config.preferred_depth = 24;
> -	dev->mode_config.prefer_shadow = 1;
>  
>  	dev->mode_config.funcs = (void *)&hibmc_mode_funcs;
>  
> @@ -335,18 +361,22 @@ static int hibmc_load(struct drm_device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	resource_size_t vram_base, vram_size;
>  	int ret;
>  
>  	ret = hibmc_hw_init(priv);
>  	if (ret)
>  		return ret;
>  
> -	ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
> -				    pci_resource_len(pdev, 0));
> -	if (ret) {
> -		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
> -		return ret;
> -	}
> +	vram_base = pci_resource_start(pdev, 0);
> +	vram_size = pci_resource_len(pdev, 0);
> +
> +	priv->vram = devm_ioremap_wc(dev->dev, vram_base, vram_size);
> +	if (!priv->vram)
> +		return -ENOMEM;
> +
> +	priv->vram_base = vram_base;
> +	priv->vram_size = vram_size;
>  
>  	ret = hibmc_kms_init(priv);
>  	if (ret)
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index cd3a3fca1fe6..dce8572bf63e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -38,6 +38,11 @@ struct hibmc_drm_private {
>  	/* hw */
>  	void __iomem   *mmio;
>  
> +	/* vram */
> +	void __iomem *vram;
> +	resource_size_t	vram_base;
> +	resource_size_t	vram_size;
> +
>  	/* drm */
>  	struct drm_device dev;
>  	struct drm_plane primary_plane;
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 5ccdae21b94a..86b967174b60 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -141,6 +141,10 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>  void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
>  			      struct drm_printer *p, unsigned int indent);
>  
> +int drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
> +				     struct drm_device *dev, size_t size,
> +				     uint32_t *handle);
> +
>  extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
>  
>  /*