Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <120a8dc1-7e5c-85da-e087-10d21fedbf20@igalia.com>
Date:   Thu, 14 Jul 2022 11:23:10 -0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] drm: Fix EDID firmware load on resume
Content-Language: en-US
To:     Matthieu CHARETTE <matthieu.charette@gmail.com>
Cc:     tzimmermann@suse.de, airlied@linux.ie,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
References: <WKTLER.NFH5CX2O259Q@gmail.com>
From:   =?UTF-8?Q?Andr=c3=a9_Almeida?= <andrealmeid@igalia.com>
In-Reply-To: <WKTLER.NFH5CX2O259Q@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@gmail.com>
> ---
> 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é