[Intel-gfx] [PATCH] drm/edid/firmware: stop using throwaway platform device

[Matthieu CHARETTE]

Currently the EDID is requested during the resume. But since it's
requested too early, this means before the filesystem is mounted, the
firmware request fails. This make the DRM driver crash when resuming.
This kind of issue should be prevented by the firmware caching process
which cache every firmware requested for the next resume. But since we
are using a temporary device, the firmware isn't cached on suspend
since the device doesn't work anymore.
When using a non temporary device to get the EDID, the firmware will
be cached on suspend for the next resume. So requesting the firmware
during resume will succeed.
But if the firmware has never been requested since the boot, this
means that the monitor isn't plugged since the boot. The kernel will
not be caching the EDID. So if we plug the monitor while the machine
is suspended. The resume will fail to load the firmware. And the DRM
driver will crash.
So basically, your fix should solve the issue except for the case
where the monitor hasn't been plugged since boot and is plugged while
the machine is suspended.
I hope I was clear. Tell me if I wasn't. I'm not really good at explaining.

Matthieu

On 10/11/22, Jani Nikula <jani.nikula at intel.com> wrote:
> On Tue, 11 Oct 2022, Matthieu CHARETTE <matthieu.charette at gmail.com> wrote:
>> It should fix the issue. Meanwhile, the system will still crash if a
>> new monitor is plugged while the machine is suspended. We might need to
>> precache the EDID to prevent that.
>
> Please elaborate.
>
> BR,
> Jani.
>
>
>>
>> Matthieu
>>
>> On Fri, Oct 7 2022 at 01:21:46 AM +0300, Jani Nikula
>> <jani.nikula at intel.com> wrote:
>>> 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.
>>>
>>> Do we need to?
>>>
>>> Maybe this fixes the suspend/resume issue?
>>>
>>> (Yes, I'll rewrite the commit message if this is the way to go ;)
>>>
>>> References:
>>> https://lore.kernel.org/r/20220727074152.43059-1-matthieu.charette@gmail.com
>>> Cc: Matthieu CHARETTE <matthieu.charette at gmail.com>
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>> ---
>>>  drivers/gpu/drm/drm_edid_load.c | 11 +----------
>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid_load.c
>>> b/drivers/gpu/drm/drm_edid_load.c
>>> index 37d8ba3ddb46..fbae12130234 100644
>>> --- a/drivers/gpu/drm/drm_edid_load.c
>>> +++ b/drivers/gpu/drm/drm_edid_load.c
>>> @@ -182,18 +182,9 @@ static void *edid_load(struct drm_connector
>>> *connector, const char *name,
>>>  		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_ERROR("Failed to register EDID firmware platform device "
>>> -				  "for connector \"%s\"\n", 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_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>>  				  name, err);
>>> --
>>> 2.34.1
>>>
>>
>>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
>