linux-kernel - Re: [PATCH v2 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YnAcj9O3l/qLc5ss@pendragon.ideasonboard.com>
Date:   Mon, 2 May 2022 21:01:51 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 3/3] drm: Allow simpledrm to setup its emulated FB as
 firmware provided

Hi Javier,

On Mon, May 02, 2022 at 07:09:17PM +0200, Javier Martinez Canillas wrote:
> On 5/2/22 18:17, Laurent Pinchart wrote:
> > On Mon, May 02, 2022 at 05:39:00PM +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@...e.de>
> >> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  drivers/gpu/drm/drm_fb_helper.c  |  9 +++++++++
> >>  drivers/gpu/drm/tiny/simpledrm.c |  2 +-
> >>  include/drm/drm_fb_helper.h      | 10 ++++++++++
> >>  3 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index fd0084ad77c3..775e47c5de1f 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.
> >> @@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options)
> >>  {
> >>  	struct drm_fb_helper *fb_helper;
> >>  	unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options);
> >> +	bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options);
> >>  	int ret;
> >>  
> >>  	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
> >> @@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options)
> >>  		preferred_bpp = 32;
> >>  	fb_helper->preferred_bpp = preferred_bpp;
> >>  
> >> +	fb_helper->firmware = firmware;
> > 
> > I'd get rid of the local variable and write
> >
> 
> I actually considered that but then decided to add a local variable to
> have both options set in the same place, since I thought that would be
> easier to read and also consistent with how preferred_bpp is handled.
> 
> Maybe I could go the other way around and rework patch 2/3 to also not
> require a preferred_bpp local variable ? That patch won't be as small
> as it's now though. -- 

Up to you, or you could ignore the comment, it's minor. If you want to
keep the variable, you could also make it const, it's a good practice to
show it isn't intended to be modified.

-- 
Regards,

Laurent Pinchart