Re: [v2,7/8] drm/fb-helper: Consolidate CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM

[Sui jingfeng <suijingfeng@loongson.cn>]

Tested-by: Sui Jingfeng<suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Consolidate all handling of CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM by
> making the module parameter optional in drm_fb_helper.c.
>
> Without the config option, modules can set smem_start in struct
> fb_info for internal usage, but not export if to userspace. The
> address can only be exported by enabling the option and setting
> the module parameter. Also update the comment.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++--------------
>   drivers/gpu/drm/drm_fbdev_dma.c |  9 +--------
>   include/drm/drm_fb_helper.h     |  9 ---------
>   3 files changed, 9 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bb0b25209b3e..63ec95e86d0e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -60,16 +60,17 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
>    * In order to keep user-space compatibility, we want in certain use-cases
>    * to keep leaking the fbdev physical address to the user-space program
>    * handling the fbdev buffer.
> - * This is a bad habit essentially kept into closed source opengl driver
> - * that should really be moved into open-source upstream projects instead
> - * of using legacy physical addresses in user space to communicate with
> - * other out-of-tree kernel modules.
> + *
> + * This is a bad habit, essentially kept to support closed-source OpenGL
> + * drivers that should really be moved into open-source upstream projects
> + * instead of using legacy physical addresses in user space to communicate
> + * with other out-of-tree kernel modules.
>    *
>    * This module_param *should* be removed as soon as possible and be
>    * considered as a broken and legacy behaviour from a modern fbdev device.
>    */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>   static bool drm_leak_fbdev_smem;
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>   module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
>   MODULE_PARM_DESC(drm_leak_fbdev_smem,
>   		 "Allow unsafe leaking fbdev physical smem address [default=false]");
> @@ -1983,10 +1984,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
>   		return ret;
>   	}
>   
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -	fb_helper->hint_leak_smem_start = drm_leak_fbdev_smem;
> -#endif
> -
>   	/* push down into drivers */
>   	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
>   	if (ret < 0)
> @@ -2185,11 +2182,8 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper)
>   
>   	info = fb_helper->info;
>   	info->var.pixclock = 0;
> -	/* Shamelessly allow physical address leaking to userspace */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -	if (!fb_helper->hint_leak_smem_start)
> -#endif
> -		/* don't leak any physical addresses to userspace */
> +
> +	if (!drm_leak_fbdev_smem)
>   		info->flags |= FBINFO_HIDE_SMEM_START;
>   
>   	/* Need to drop locks to avoid recursive deadlock in
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index cf553ac12a0f..728deffcc0d9 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -136,16 +136,9 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
>   		info->flags |= FBINFO_READS_FAST; /* signal caching */
>   	info->screen_size = sizes->surface_height * fb->pitches[0];
>   	info->screen_buffer = map.vaddr;
> +	info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
>   	info->fix.smem_len = info->screen_size;
>   
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -	/*
> -	 * Shamelessly leak the physical address to user-space.
> -	 */
> -	if (fb_helper->hint_leak_smem_start && !info->fix.smem_start)
> -		info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
> -#endif
> -
>   	return 0;
>   
>   err_drm_client_buffer_vunmap:
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index c5822ec2fdd1..72032c354a30 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -195,15 +195,6 @@ struct drm_fb_helper {
>   	 */
>   	int preferred_bpp;
>   
> -	/**
> -	 * @hint_leak_smem_start:
> -	 *
> -	 * Hint to the fbdev emulation to store the framebuffer's physical
> -	 * address in struct &fb_info.fix.smem_start. If the hint is unset,
> -	 * the smem_start field should always be cleared to zero.
> -	 */
> -	bool hint_leak_smem_start;
> -
>   #ifdef CONFIG_FB_DEFERRED_IO
>   	/**
>   	 * @fbdefio: