Re: [PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

Hi,

Sorry, my email client removed every tab. I will send the v2 in a new 
thread.

Thanks.

Matthieu

On Thu, Jul 14 2022 at 11:23:10 AM -0300, André Almeida 
<andrealmeid@xxxxxxxxxx> wrote:
> Hi Matthieu,
>
> Thanks for your patch.
>
> Às 11:58 de 06/07/22, Matthieu CHARETTE escreveu:
> >  Loading an EDID using drm.edid_firmware parameter makes resume to 
> > fail
> >  after firmware cache is being cleaned. This is because edid_load() 
> > use a
> >  temporary device to request the firmware. This cause the EDID 
> > firmware
> >  not to be cached from suspend. And, requesting the EDID firmware 
> > return
> >  an error during resume.
> >  So the request_firmware() call should use a permanent device for 
> > each
> >  connector. Also, we should cache the EDID even if no monitor is
> >  connected, in case it's plugged while suspended.
> >
> >  Signed-off-by: Matthieu CHARETTE <matthieu.charette@xxxxxxxxx>
> >  ---
> >  drivers/gpu/drm/drm_connector.c | 9 ++++
> >  drivers/gpu/drm/drm_edid_load.c | 81 
> > ++++++++++++++++++++++++++++-----
> >  include/drm/drm_connector.h | 12 +++++
> >  include/drm/drm_edid.h | 3 ++
> >  4 files changed, 94 insertions(+), 11 deletions(-)
> >
> >  diff --git a/drivers/gpu/drm/drm_connector.c
> >  b/drivers/gpu/drm/drm_connector.c
> >  index 1c48d162c77e..e8819ebf1c4b 100644
> >  --- a/drivers/gpu/drm/drm_connector.c
> >  +++ b/drivers/gpu/drm/drm_connector.c
> >  @@ -31,6 +31,7 @@
> >  #include <drm/drm_privacy_screen_consumer.h>
> >  #include <drm/drm_sysfs.h>
> >
> >  +#include <linux/platform_device.h>
> >  #include <linux/uaccess.h>
> >
> >  #include "drm_crtc_internal.h"
> >  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
> >
> >   drm_connector_get_cmdline_mode(connector);
> >
> >  + connector->edid_load_pdev = NULL;
> >  + drm_cache_edid_firmware(connector);
> >  +
> >   /* We should add connectors at the end to avoid upsetting the 
> > connector
> >    * index too much.
> >    */
> >  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector
> >  *connector)
> >    connector->tile_group = NULL;
> >   }
> >
> >  + if (connector->edid_load_pdev) {
> >  + platform_device_unregister(connector->edid_load_pdev);
> >  + connector->edid_load_pdev = NULL;
> >  + }
> >  +
>
> The indentation of your patch is wrong in different places, like in 
> this
> if here. It should be like
>
> + if (connector->edid_load_pdev) {
> + 	platform_device_unregister(connector->edid_load_pdev);
> + 	connector->edid_load_pdev = NULL;
> + }
>
> ./scripts/checkpatch.pl can help you detect those issues for your v2
>
> Thanks,
> 	André