Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 4 May 2022 12:58:49 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     linux-kernel@vger.kernel.org,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 3/3] drm: Allow simpledrm to setup its emulated FB as
 firmware provided
Message-ID: <YnJcaaDcIsJKhSwQ@phenom.ffwll.local>
Mail-Followup-To: Javier Martinez Canillas <javierm@redhat.com>,
        linux-kernel@vger.kernel.org,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        dri-devel@lists.freedesktop.org
References: <20220503071540.471667-1-javierm@redhat.com>
 <20220503071540.471667-4-javierm@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220503071540.471667-4-javierm@redhat.com>
Precedence: bulk

On Tue, May 03, 2022 at 09:15:40AM +0200, Javier Martinez Canillas wrote:
> Indicate to fbdev subsystem that the registered framebuffer is provided by
> the system firmware, so that it can handle accordingly. For example, would
> unregister the FB devices if asked to remove the conflicting framebuffers.
> 
> Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter.
> Drivers can use this to indicate the FB helper initialization that the FB
> registered is provided by the firmware, so it can be configured as such.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> Changes in v3:
> - Drop the firmware local variable (Laurent Pinchart).
> - Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test robot).

Just for the record what I brought up on irc already:

FBINFO_MISC_FIRMWARE is purely an internal flag with no uapi impact, and
it's only to control whether we nuke this from
remove_conflicting_framebuffer or not. Since simpledrm only ever binds
against sysfb I think it'd be cleaner to only rely on that, and relegate
that entire FBINFO_MISC_FIRMWARE misc hack to the fbdev dungeons and let
it quietly wither away there.

Also I'm not a huge fan of these midlayer flags in general :-)
-Daniel

> 
>  drivers/gpu/drm/drm_fb_helper.c  |  8 ++++++++
>  drivers/gpu/drm/tiny/simpledrm.c |  2 +-
>  include/drm/drm_fb_helper.h      | 10 ++++++++++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 52042ba1e4cf..28b21858b726 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
>  		/* don't leak any physical addresses to userspace */
>  		info->flags |= FBINFO_HIDE_SMEM_START;
>  
> +	/* Indicate that the framebuffer is provided by the firmware */
> +	if (fb_helper->firmware)
> +		info->flags |= FBINFO_MISC_FIRMWARE;
> +
>  	/* Need to drop locks to avoid recursive deadlock in
>  	 * register_framebuffer. This is ok because the only thing left to do is
>  	 * register the fbdev emulation instance in kernel_fb_helper_list. */
> @@ -2512,6 +2516,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   *
>   * * DRM_FB_BPP: bits per pixel for the device. If the field is not set,
>   *   @dev->mode_config.preferred_depth is used instead.
> + * * DRM_FB_FW: if the framebuffer for the device is provided by the
> + *   system firmware.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
>   * dumb buffers with a virtual address and that can be mmap'ed.
> @@ -2569,6 +2575,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, const unsigned int options)
>  	if (!fb_helper->preferred_bpp)
>  		fb_helper->preferred_bpp = 32;
>  
> +	fb_helper->firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options);
> +
>  	ret = drm_fbdev_client_hotplug(&fb_helper->client);
>  	if (ret)
>  		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index f5b8e864a5cd..f6f1c5e108b2 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -901,7 +901,7 @@ static int simpledrm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	drm_fbdev_generic_setup(dev, 0);
> +	drm_fbdev_generic_setup(dev, DRM_FB_OPTION(DRM_FB_FW, 1));
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 5fc41cf0c987..5a17af423944 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -44,6 +44,7 @@ enum mode_set_atomic {
>  };
>  
>  #define DRM_FB_BPP_MASK GENMASK(7, 0)
> +#define DRM_FB_FW_MASK GENMASK(8, 8)
>  
>  /* Using the GNU statement expression extension */
>  #define DRM_FB_OPTION(option, value)				\
> @@ -197,6 +198,15 @@ struct drm_fb_helper {
>  	 * See also: @deferred_setup
>  	 */
>  	int preferred_bpp;
> +
> +	/**
> +	 * @firmware:
> +	 *
> +	 * Set if the driver indicates to the FB helper initialization that the
> +	 * framebuffer for the device being registered is provided by firmware,
> +	 * so that it can pass this on when registering the framebuffer device.
> +	 */
> +	bool firmware;
>  };
>  
>  static inline struct drm_fb_helper *
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch