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: <5dd80287-1b09-d02c-9f67-5a0bb0a4566c@redhat.com>
Date:   Mon, 2 May 2022 18:55:10 +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 1/3] drm: Remove superfluous arg when calling to
 drm_fbdev_generic_setup()
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,
        virtualization@lists.linux-foundation.org
References: <20220502153900.408522-1-javierm@redhat.com>
 <20220502153900.408522-2-javierm@redhat.com>
 <YnABjdpGC166yIY7@pendragon.ideasonboard.com>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <YnABjdpGC166yIY7@pendragon.ideasonboard.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Laurent,

On 5/2/22 18:06, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Mon, May 02, 2022 at 05:38:58PM +0200, Javier Martinez Canillas wrote:
>> The drm_fbdev_generic_setup() function already sets the preferred bits per
>> pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp
>> value is zero.
>>
>> Passing the same value to the function is unnecessary. Let's cleanup that
>> in the two drivers that do it.
> 
> This looks fine, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> but why do we have two different mechanisms to set the preferred depth ?
> Could we get all drivers to set dev->mode_config.preferred_depth and

Yes, that's the plan and the reason why when we were discussing with Thomas
about how to pass this option to the FB helper layer, we agreed on reusing
the @preferred_bpp parameter rather than adding a third parameter to
drm_fbdev_generic_setup(). Since in the future drivers shouldn't pass that
information to the FB helper and just get it from the default mode config.

But doing that would require more auditing to all drivers and it could add
regressions while patches 1/2 and 2/2 in this series shouldn't cause any
behavioral changes.

> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
> comment in drm_fbdev_generic_setup() that could be related.
>

A FIXME makes sense, I'll add that to when posting a v3.
 Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat