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

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

[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>

---

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",
-- 
2.34.1

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3065 bytes --]

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.

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",

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Jani Nikula]

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

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

[Matthieu CHARETTE]

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