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;
Date:   Fri, 15 Jul 2022 11:03:23 +0200
From:   Matthieu CHARETTE <matthieu.charette@gmail.com>
Subject: Re: [PATCH] drm: Fix EDID firmware load on resume
To:     =?iso-8859-1?b?QW5kcuk=?= Almeida <andrealmeid@igalia.com>
Cc:     maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, airlied@linux.ie, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Message-Id: <N512FR.B1R49X9JM9FW2@gmail.com>
In-Reply-To: <120a8dc1-7e5c-85da-e087-10d21fedbf20@igalia.com>
References: <WKTLER.NFH5CX2O259Q@gmail.com>
        <120a8dc1-7e5c-85da-e087-10d21fedbf20@igalia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

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

Thanks.

Matthieu

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