Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <9556f080-d309-f396-6d28-1190bc94cc38@redhat.com>
Date:   Mon, 2 May 2022 19:09:17 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v2 3/3] drm: Allow simpledrm to setup its emulated FB as
 firmware provided
Content-Language: en-US
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     linux-kernel@vger.kernel.org,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        dri-devel@lists.freedesktop.org
References: <20220502153900.408522-1-javierm@redhat.com>
 <20220502153900.408522-4-javierm@redhat.com>
 <YnAEDlikr+d8cvy4@pendragon.ideasonboard.com>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <YnAEDlikr+d8cvy4@pendragon.ideasonboard.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Laurent,

On 5/2/22 18:17, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> 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@suse.de>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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. -- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat