Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Jani Nikula <jani.nikula@linux.intel.com>
To:     bbaa <bbaa@bbaa.fun>, maarten.lankhorst@linux.intel.com,
        mripard@kernel.org, tzimmermann@suse.de, airlied@gmail.com,
        daniel@ffwll.ch
Cc:     linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [Bug Report] drm/edid: drm_edid_override_connector_update
 returns a incorrect value
In-Reply-To: <930E9B4C7D91FDFF+29b34d89-8658-4910-966a-c772f320ea03@bbaa.fun>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <930E9B4C7D91FDFF+29b34d89-8658-4910-966a-c772f320ea03@bbaa.fun>
Date:   Thu, 07 Dec 2023 11:54:34 +0200
Message-ID: <87jzpq1go5.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 07 Dec 2023, bbaa <bbaa@bbaa.fun> wrote:
> Hello everyone,
>
> drm_edid_override_connector_update seem return a incorrect value.
>
> drivers/gpu/drm/drm_edid.c (Linux 6.7-rc4)
>    2294	/**
>    2295	 * drm_edid_override_connector_update - add modes from override/f=
irmware EDID
>    2296	 * @connector: connector we're probing
>    2297	 *
>    2298	 * Add modes from the override/firmware EDID, if available. Only =
to be used from
>    2299	 * drm_helper_probe_single_connector_modes() as a fallback for wh=
en DDC probe
>    2300	 * failed during drm_get_edid() and caused the override/firmware =
EDID to be
>    2301	 * skipped.
>    2302	 *
>    2303	 * Return: The number of modes added or 0 if we couldn't find any.
>    2304	 */

Thanks for the report. I've sent a patch to hopefully fix this [1].

[1] https://patchwork.freedesktop.org/patch/msgid/20231207093821.2654267-1-=
jani.nikula@intel.com

However, please read the documentation comment above: "Only to be used
from drm_helper_probe_single_connector_modes() ..."

The function is a fallback for a *very* specific and rare scenario.

> This will break the EDID override behavior on Nvidia graphics cards.
>
> NVIDIA/open-gpu-kernel-modules:
> kernel-open/nvidia-drm/nvidia-drm-connector.c:
>  =C2=A0=C2=A0103 =C2=A0#if defined(NV_DRM_CONNECTOR_HAS_OVERRIDE_EDID) =
=C2=A0=C2=A0104 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if=20
> (connector->override_edid) { =C2=A0=C2=A0105 =C2=A0#else =C2=A0=C2=A0106 =
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if=20
> (drm_edid_override_connector_update(connector) > 0) { =C2=A0=C2=A0107 =C2=
=A0#endif=20
>  =C2=A0=C2=A0108 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0co=
nst struct drm_property_blob *edid =3D=20
> connector->edid_blob_ptr; =C2=A0=C2=A0109=20
> drm_edid_override_connector_update(connector)  will return zero here.

That's an out-of-tree driver that doesn't follow the documentation
above. Drivers have no business calling the function.

All of the override/firmware EDID handling should be covered
transparently via the drm_edid_read*() and drm_get_edid() functions, and
the drivers shouldn't have to ever care about overrides, at all. Drivers
shouldn't really use connector->edid_blob_ptr directly either.

Please report and get that fixed downstream.


BR,
Jani.


--=20
Jani Nikula, Intel