Re: [RESEND] drm/edid/firmware: stop using a throwaway platform device

[Matthieu CHARETTE <matthieu.charette@gmail.com>]

Thank you everyone for your work!

Matthieu.

On Wed, Nov 16 2022 at 03:32:01 PM +0200, Jani Nikula 
<jani.nikula@intel.com> wrote:
> On Wed, 16 Nov 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>  Hi
>> 
>>  Am 14.11.22 um 12:17 schrieb Jani Nikula:
>>>  We've used a temporary platform device for firmware EDID loading 
>>> since
>>>  it was introduced in commit da0df92b5731 ("drm: allow loading an 
>>> EDID as
>>>  firmware to override broken monitor"), but there's no explanation 
>>> why.
>>> 
>>>  Using a temporary device does not play well with CONFIG_FW_CACHE=y,
>>>  which caches firmware images (e.g. on suspend) so that drivers can
>>>  request firmware when the system is not ready for it, and return 
>>> the
>>>  images from the cache (e.g. during resume). This works 
>>> automatically for
>>>  regular devices, but obviously not for a temporarily created 
>>> device.
>>> 
>>>  Stop using the throwaway platform device, and use the drm device
>>>  instead.
>>> 
>>>  Note that this may still be problematic for cases where the 
>>> display was
>>>  plugged in during suspend, and the firmware wasn't loaded and 
>>> therefore
>>>  not cached before suspend.
>>> 
>>>  References: 
>>> https://lore.kernel.org/r/20220727074152.43059-1-matthieu.charette@gmail.com
>>>  Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
>>>  Reported-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>>  Tested-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>>  Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>  Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>>  Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> 
>>  I looked through request_firmware() but did not see any signs that 
>> it
>>  somehow depends on a platform device. I assume that this might only
>>  affect the device name in the error message.
> 
> Thanks, pushed to drm-misc-next.
> 
> Matthieu, thanks for you patience and the report as well!
> 
> BR,
> Jani.
> 
> 
>> 
>>  Best regards
>>  Thomas
>> 
>>> 
>>>  ---
>>> 
>>>  Resend with a proper commit message; patch itself is unchanged.
>>>  ---
>>>    drivers/gpu/drm/drm_edid_load.c | 13 +------------
>>>    1 file changed, 1 insertion(+), 12 deletions(-)
>>> 
>>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>>> b/drivers/gpu/drm/drm_edid_load.c
>>>  index ef4ab59d6935..5d9ef267ebb3 100644
>>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>>  @@ -172,20 +172,9 @@ static const struct drm_edid 
>>> *edid_load(struct drm_connector *connector, const c
>>>    		fwdata = generic_edid[builtin];
>>>    		fwsize = sizeof(generic_edid[builtin]);
>>>    	} else {
>>>  -		struct platform_device *pdev;
>>>    		int err;
>>> 
>>>  -		pdev = platform_device_register_simple(connector->name, -1, 
>>> NULL, 0);
>>>  -		if (IS_ERR(pdev)) {
>>>  -			drm_err(connector->dev,
>>>  -				"[CONNECTOR:%d:%s] Failed to register EDID firmware platform 
>>> device for connector \"%s\"\n",
>>>  -				connector->base.id, connector->name,
>>>  -				connector->name);
>>>  -			return ERR_CAST(pdev);
>>>  -		}
>>>  -
>>>  -		err = request_firmware(&fw, name, &pdev->dev);
>>>  -		platform_device_unregister(pdev);
>>>  +		err = request_firmware(&fw, name, connector->dev->dev);
>>>    		if (err) {
>>>    			drm_err(connector->dev,
>>>    				"[CONNECTOR:%d:%s] Requesting EDID firmware \"%s\" failed 
>>> (err=%d)\n",
> 
> --
> Jani Nikula, Intel Open Source Graphics Center