On Fri, 15 Sep 2023, Alex Hung <alex.hung@xxxxxxx> wrote:
> [WHY]
> edid_override and drm_edid_override_connector_update, according to drm
> documentation, should not be referred outside drm_edid.
>
> [HOW]
> Remove and replace them accordingly. This can tested by IGT's
> kms_hdmi_inject test.
>
> Signed-off-by: Alex Hung <alex.hung@xxxxxxx>
> ---
>
> V2 - add comments for drm_get_edid and check edid before use.
>
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++++++++++--------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5efebc06296b..b29ef87c27a9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6444,15 +6444,24 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
> static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
> {
> struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
> struct dc_link *dc_link = aconnector->dc_link;
> struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> struct edid *edid;
>
> - if (!connector->edid_override)
> + /*
> + * Note: drm_get_edid gets edid in the following order:
> + * 1) override EDID if set via edid_override debugfs,
> + * 2) firmware EDID if set via edid_firmware module parameter
> + * 3) regular DDC read.
> + */
> + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
> + if (!edid) {
> + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name);
> + connector->force = DRM_FORCE_OFF;
Drivers aren't supposed to modify connector->force.
Why would you do that anyway? This connects EDID probe success with
connector forcing, and I've been repeatedly saying these are two
separate things that should not be conflated.
Maybe the user has set connector->force, because the display probe is
flaky. This switches connector->force off if the display does not
respond, undermining one of the main purposes of the whole thing.
> return;
> + }
>
> - drm_edid_override_connector_update(&aconnector->base);
> - edid = aconnector->base.edid_blob_ptr->data;
> aconnector->edid = edid;
>
> /* Update emulated (virtual) sink's EDID */
> @@ -6487,30 +6496,27 @@ static int get_modes(struct drm_connector *connector)
>
> static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> {
> + struct drm_connector *connector = &aconnector->base;
> + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(&aconnector->base);
> struct dc_sink_init_data init_params = {
> .link = aconnector->dc_link,
> .sink_signal = SIGNAL_TYPE_VIRTUAL
> };
> struct edid *edid;
>
> - if (!aconnector->base.edid_blob_ptr) {
> - /* if connector->edid_override valid, pass
> - * it to edid_override to edid_blob_ptr
> - */
> -
> - drm_edid_override_connector_update(&aconnector->base);
> -
> - if (!aconnector->base.edid_blob_ptr) {
> - DRM_ERROR("No EDID firmware found on connector: %s ,forcing to OFF!\n",
> - aconnector->base.name);
> -
> - aconnector->base.force = DRM_FORCE_OFF;
> - return;
> - }
> + /*
> + * Note: drm_get_edid gets edid in the following order:
> + * 1) override EDID if set via edid_override debugfs,
> + * 2) firmware EDID if set via edid_firmware module parameter
> + * 3) regular DDC read.
> + */
> + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
> + if (!edid) {
> + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name);
> + connector->force = DRM_FORCE_OFF;
Ditto.
BR,
Jani.
> + return;
> }
>
> - edid = (struct edid *) aconnector->base.edid_blob_ptr->data;
> -
> aconnector->edid = edid;
>
> aconnector->dc_em_sink = dc_link_add_remote_sink(
--
Jani Nikula, Intel
