* [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid @ 2024-03-27 21:44 Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 1/6] drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector Melissa Wen ` (6 more replies) 0 siblings, 7 replies; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel Hi, After finding a null-pointer dereference when running igt@kms_connector_force_edid (fixed by [1]), I started migrating raw edid handles in amdgpu connectors to use opaque drm_edid and its helpers. However, I'm struggling to remove all drm_edid_raw() occurrences for different reasons and, with this RFC, I'm looking for suggestions to overcome them: 1. `dc_link` ops need a copy of edid data and size via `dc_sink` (basically, dc_link_add_remote_sink()). I'm unsure what a possible approach to clean dc_link helpers should be since it seems part of the shared code. Any suggestions? 2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are in drm_edid halpers, but there are still a few that are not managed by them yet. For example: ``` edid_caps->serial_number = edid_buf->serial; edid_caps->manufacture_week = edid_buf->mfg_week; edid_caps->manufacture_year = edid_buf->mfg_year; ``` AFAIU I see the same pending issue in i915/pnpid_get_panel_type() - that still uses drm_edid_raw(). My suggestion is to keep the drm_edid_raw() around and verify if there are no regressions or functional changes after this switch of amdgpu_dm_connector to struct drm_edid. I added FIXME comments to all drm_edid_raw occurrences to keep these pending issues on our radar. So we can continue removing driver-specific code in favor of drm_edid common code. What do you think? This is the current status of drm_edid migration in this patchset: - patch 1-2: remove unused variables found in this process. - patch 3-4: migration of amdgpu_dm_connector from struct edid to struct drm_edid and its helpers. - patch 5-6: remove duplicated code for parsing vrr caps. WIP: require more validation. Additionally, it needs extensive testing and validation of a large variety of display caps and I don't have the required setup for it (perhaps you can test it in your CI to point issues?). I don't see how to reduce the scope of changes to mitigate the noise/disruption, but any suggestions are welcome. [1] https://lore.kernel.org/amd-gfx/20240216122401.216860-1-mwen@igalia.com/ Thanks, Melissa v1: https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/ - use const to fix compilation warnings (Alex Hung) - remove unused variables - remove driver-specific parser for connector info in favor of drm_edid common code v2: https://lore.kernel.org/amd-gfx/20240327165828.288792-1-mwen@igalia.com/ - fix general protection fault on mst Melissa Wen (6): drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector drm/amd/display: clean unused variables for hdmi freesync parser drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid drm/amd/display: switch to setting physical address directly drm/amd/display: always call connector_update when parsing freesync_caps drm/amd/display: remove redundant freesync parser for DP .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 187 +++++------------- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 5 +- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 +-- 4 files changed, 69 insertions(+), 155 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH v3 1/6] drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen @ 2024-03-27 21:44 ` Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 2/6] drm/amd/display: clean unused variables for hdmi freesync parser Melissa Wen ` (5 subsequent siblings) 6 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel We set pixel_clock_mhz when updating freesync caps, but it's not used anywhere. Remove it. Signed-off-by: Melissa Wen <mwen@igalia.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 - 2 files changed, 4 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 ef546932f6c9..05ffe844b1fc 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -11260,7 +11260,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->min_vfreq = 0; amdgpu_dm_connector->max_vfreq = 0; - amdgpu_dm_connector->pixel_clock_mhz = 0; connector->display_info.monitor_range.min_vfreq = 0; connector->display_info.monitor_range.max_vfreq = 0; freesync_capable = false; @@ -11324,8 +11323,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, connector->display_info.monitor_range.min_vfreq; amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq; - amdgpu_dm_connector->pixel_clock_mhz = - range->pixel_clock_mhz * 10; break; } diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index 09519b7abf67..67647bb5999b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -678,7 +678,6 @@ struct amdgpu_dm_connector { * value is set to zero when there is no FreeSync support. */ int max_vfreq ; - int pixel_clock_mhz; /* Audio instance - protected by audio_lock. */ int audio_inst; -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH v3 2/6] drm/amd/display: clean unused variables for hdmi freesync parser 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 1/6] drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector Melissa Wen @ 2024-03-27 21:44 ` Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen ` (4 subsequent siblings) 6 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel Signed-off-by: Melissa Wen <mwen@igalia.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 --- 1 file changed, 3 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 05ffe844b1fc..280562707cd0 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -11344,9 +11344,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, } else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info); if (i >= 0 && vsdb_info.freesync_supported) { - timing = &edid->detailed_timings[i]; - data = &timing->data.other_data; - amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz; amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz; if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10) -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 1/6] drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 2/6] drm/amd/display: clean unused variables for hdmi freesync parser Melissa Wen @ 2024-03-27 21:44 ` Melissa Wen 2024-03-28 9:33 ` Jani Nikula 2024-03-27 21:44 ` [RFC PATCH v3 4/6] drm/amd/display: switch to setting physical address directly Melissa Wen ` (3 subsequent siblings) 6 siblings, 1 reply; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel Replace raw edid handling (struct edid) with the opaque EDID type (struct drm_edid) on amdgpu_dm_connector for consistency. It may also prevent mismatch of approaches in different parts of the driver code. Working in progress. It was only exercised with IGT tests. v2: use const to fix warnings (Alex Hung) v3: fix general protection fault on mst Signed-off-by: Melissa Wen <mwen@igalia.com> --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +++++++++---------- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++--- 4 files changed, 67 insertions(+), 68 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 280562707cd0..bbbf9c9d40d5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect( aconnector->dc_sink = sink; dc_sink_retain(aconnector->dc_sink); if (sink->dc_edid.length == 0) { - aconnector->edid = NULL; + drm_edid_free(aconnector->edid); if (aconnector->dc_link->aux_mode) { drm_dp_cec_unset_edid( &aconnector->dm_dp_aux.aux); } } else { - aconnector->edid = - (struct edid *)sink->dc_edid.raw_edid; + const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid; + aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length); + /* FIXME: Get rid of drm_edid_raw() */ if (aconnector->dc_link->aux_mode) drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux, - aconnector->edid); + drm_edid_raw(aconnector->edid)); } if (!aconnector->timing_requested) { @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect( "failed to create aconnector->requested_timing\n"); } - drm_connector_update_edid_property(connector, aconnector->edid); + drm_edid_connector_update(connector, aconnector->edid); amdgpu_dm_update_freesync_caps(connector, aconnector->edid); update_connector_ext_caps(aconnector); } else { drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux); amdgpu_dm_update_freesync_caps(connector, NULL); - drm_connector_update_edid_property(connector, NULL); + drm_edid_connector_update(connector, NULL); aconnector->num_modes = 0; dc_sink_release(aconnector->dc_sink); aconnector->dc_sink = NULL; - aconnector->edid = NULL; + drm_edid_free(aconnector->edid); kfree(aconnector->timing_requested); aconnector->timing_requested = NULL; /* Set CP to DESIRED if it was ENABLED, so we can re-enable it again on hotplug */ @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); struct dc_link *dc_link = aconnector->dc_link; struct dc_sink *dc_em_sink = aconnector->dc_em_sink; - struct edid *edid; - struct i2c_adapter *ddc; - - if (dc_link && dc_link->aux_mode) - ddc = &aconnector->dm_dp_aux.aux.ddc; - else - ddc = &aconnector->i2c->base; + const struct drm_edid *drm_edid; /* * Note: drm_get_edid gets edid in the following order: @@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) * 2) firmware EDID if set via edid_firmware module parameter * 3) regular DDC read. */ - edid = drm_get_edid(connector, ddc); - if (!edid) { + drm_edid = drm_edid_read(connector); + + if (!drm_edid) { DRM_ERROR("No EDID found on connector: %s.\n", connector->name); return; } - - aconnector->edid = edid; - + aconnector->edid = drm_edid; + drm_edid_connector_update(connector, drm_edid); /* Update emulated (virtual) sink's EDID */ if (dc_em_sink && dc_link) { + const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() + memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps)); - memmove(dc_em_sink->dc_edid.raw_edid, edid, (edid->extensions + 1) * EDID_LENGTH); + memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH); dm_helpers_parse_edid_caps( dc_link, &dc_em_sink->dc_edid, @@ -6716,18 +6713,12 @@ 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 dc_link *dc_link = aconnector->dc_link; struct dc_sink_init_data init_params = { .link = aconnector->dc_link, .sink_signal = SIGNAL_TYPE_VIRTUAL }; - struct edid *edid; - struct i2c_adapter *ddc; - - if (dc_link->aux_mode) - ddc = &aconnector->dm_dp_aux.aux.ddc; - else - ddc = &aconnector->i2c->base; + const struct drm_edid *drm_edid; + const struct edid *edid; /* * Note: drm_get_edid gets edid in the following order: @@ -6735,17 +6726,19 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) * 2) firmware EDID if set via edid_firmware module parameter * 3) regular DDC read. */ - edid = drm_get_edid(connector, ddc); - if (!edid) { + drm_edid = drm_edid_read(connector); + if (!drm_edid) { DRM_ERROR("No EDID found on connector: %s.\n", connector->name); return; } - if (drm_detect_hdmi_monitor(edid)) + if (connector->display_info.is_hdmi) init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A; - aconnector->edid = edid; + aconnector->edid = drm_edid; + drm_edid_connector_update(connector, drm_edid); + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() aconnector->dc_em_sink = dc_link_add_remote_sink( aconnector->dc_link, (uint8_t *)edid, @@ -7428,16 +7421,16 @@ static void amdgpu_set_panel_orientation(struct drm_connector *connector) } static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector, - struct edid *edid) + const struct drm_edid *drm_edid) { struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector); - if (edid) { + if (drm_edid) { /* empty probed_modes */ INIT_LIST_HEAD(&connector->probed_modes); amdgpu_dm_connector->num_modes = - drm_add_edid_modes(connector, edid); + drm_edid_connector_add_modes(connector); /* sorting the probed modes before calling function * amdgpu_dm_get_native_mode() since EDID can have @@ -7451,10 +7444,10 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector, amdgpu_dm_get_native_mode(connector); /* Freesync capabilities are reset by calling - * drm_add_edid_modes() and need to be + * drm_edid_connector_add_modes() and need to be * restored here. */ - amdgpu_dm_update_freesync_caps(connector, edid); + amdgpu_dm_update_freesync_caps(connector, drm_edid); } else { amdgpu_dm_connector->num_modes = 0; } @@ -7550,12 +7543,12 @@ static uint add_fs_modes(struct amdgpu_dm_connector *aconnector) } static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector, - struct edid *edid) + const struct drm_edid *drm_edid) { struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector); - if (!(amdgpu_freesync_vid_mode && edid)) + if (!(amdgpu_freesync_vid_mode && drm_edid)) return; if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10) @@ -7568,23 +7561,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector) struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector); struct drm_encoder *encoder; - struct edid *edid = amdgpu_dm_connector->edid; + const struct drm_edid *drm_edid = amdgpu_dm_connector->edid; struct dc_link_settings *verified_link_cap = &amdgpu_dm_connector->dc_link->verified_link_cap; const struct dc *dc = amdgpu_dm_connector->dc_link->dc; encoder = amdgpu_dm_connector_to_encoder(connector); - if (!drm_edid_is_valid(edid)) { + if (!drm_edid_valid(drm_edid)) { amdgpu_dm_connector->num_modes = drm_add_modes_noedid(connector, 640, 480); if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING) amdgpu_dm_connector->num_modes += drm_add_modes_noedid(connector, 1920, 1080); } else { - amdgpu_dm_connector_ddc_get_modes(connector, edid); + amdgpu_dm_connector_ddc_get_modes(connector, drm_edid); amdgpu_dm_connector_add_common_modes(encoder, connector); - amdgpu_dm_connector_add_freesync_modes(connector, edid); + amdgpu_dm_connector_add_freesync_modes(connector, drm_edid); } amdgpu_dm_fbc_init(connector); @@ -11153,7 +11146,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector, } static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector, - struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) + const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) { u8 *edid_ext = NULL; int i; @@ -11188,7 +11181,8 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector, } static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector, - struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) + const struct edid *edid, + struct amdgpu_hdmi_vsdb_info *vsdb_info) { u8 *edid_ext = NULL; int i; @@ -11230,19 +11224,19 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector, * FreeSync parameters. */ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, - struct edid *edid) + const struct drm_edid *drm_edid) { int i = 0; - struct detailed_timing *timing; - struct detailed_non_pixel *data; - struct detailed_data_monitor_range *range; + const struct detailed_timing *timing; + const struct detailed_non_pixel *data; + const struct detailed_data_monitor_range *range; struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector); struct dm_connector_state *dm_con_state = NULL; struct dc_sink *sink; - struct amdgpu_device *adev = drm_to_adev(connector->dev); struct amdgpu_hdmi_vsdb_info vsdb_info = {0}; + const struct edid *edid; bool freesync_capable = false; enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE; @@ -11255,7 +11249,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->dc_sink : amdgpu_dm_connector->dc_em_sink; - if (!edid || !sink) { + if (!drm_edid || !sink) { dm_con_state = to_dm_connector_state(connector->state); amdgpu_dm_connector->min_vfreq = 0; @@ -11272,6 +11266,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, if (!adev->dm.freesync_module) goto update; + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT || sink->sink_signal == SIGNAL_TYPE_EDP)) { bool edid_check_required = false; @@ -11341,7 +11336,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP; } - } else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { + } else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info); if (i >= 0 && vsdb_info.freesync_supported) { amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index 67647bb5999b..384f68203590 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -637,7 +637,7 @@ struct amdgpu_dm_connector { /* we need to mind the EDID between detect and get modes due to analog/digital/tvencoder */ - struct edid *edid; + const struct drm_edid *edid; /* shared with amdgpu */ struct amdgpu_hpd hpd; @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev, struct drm_connector *connector); void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, - struct edid *edid); + const struct drm_edid *drm_edid); void amdgpu_dm_trigger_timing_sync(struct drm_device *dev); diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index c27063305a13..1a37dff9c2b6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -901,7 +901,8 @@ enum dc_edid_status dm_helpers_read_local_edid( struct i2c_adapter *ddc; int retry = 3; enum dc_edid_status edid_status; - struct edid *edid; + const struct drm_edid *drm_edid; + const struct edid *edid; if (link->aux_mode) ddc = &aconnector->dm_dp_aux.aux.ddc; @@ -913,7 +914,8 @@ enum dc_edid_status dm_helpers_read_local_edid( */ do { - edid = drm_get_edid(&aconnector->base, ddc); + drm_edid = drm_edid_read_ddc(connector, ddc); + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() /* DP Compliance Test 4.2.2.6 */ if (link->aux_mode && connector->edid_corrupt) @@ -931,7 +933,7 @@ enum dc_edid_status dm_helpers_read_local_edid( memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length); /* We don't need the original edid anymore */ - kfree(edid); + drm_edid_free(drm_edid); edid_status = dm_helpers_parse_edid_caps( link, diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 941e96f100f4..a28004f9378a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -128,7 +128,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector) dc_sink_release(aconnector->dc_sink); } - kfree(aconnector->edid); + drm_edid_free(aconnector->edid); drm_connector_cleanup(connector); drm_dp_mst_put_port_malloc(aconnector->mst_output_port); @@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) return drm_add_edid_modes(connector, NULL); if (!aconnector->edid) { - struct edid *edid; + const struct drm_edid *drm_edid; - edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port); + drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port); - if (!edid) { + if (!drm_edid) { amdgpu_dm_set_mst_status(&aconnector->mst_status, MST_REMOTE_EDID, false); - drm_connector_update_edid_property( + drm_edid_connector_update( &aconnector->base, NULL); @@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) return ret; } - aconnector->edid = edid; + aconnector->edid = drm_edid; amdgpu_dm_set_mst_status(&aconnector->mst_status, MST_REMOTE_EDID, true); } @@ -355,10 +355,13 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) struct dc_sink_init_data init_params = { .link = aconnector->dc_link, .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST }; + const struct edid *edid; + + edid = drm_edid_raw(aconnector->edid); // FIXME: Get rid of drm_edid_raw() dc_sink = dc_link_add_remote_sink( aconnector->dc_link, - (uint8_t *)aconnector->edid, - (aconnector->edid->extensions + 1) * EDID_LENGTH, + (uint8_t *)edid, + (edid->extensions + 1) * EDID_LENGTH, &init_params); if (!dc_sink) { @@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) } } - drm_connector_update_edid_property( - &aconnector->base, aconnector->edid); + drm_edid_connector_update(&aconnector->base, aconnector->edid); - ret = drm_add_edid_modes(connector, aconnector->edid); + ret = drm_edid_connector_add_modes(connector); return ret; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid 2024-03-27 21:44 ` [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen @ 2024-03-28 9:33 ` Jani Nikula 2024-04-08 13:39 ` Melissa Wen 0 siblings, 1 reply; 11+ messages in thread From: Jani Nikula @ 2024-03-28 9:33 UTC (permalink / raw) To: Melissa Wen, Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: amd-gfx, kernel-dev, dri-devel On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote: > Replace raw edid handling (struct edid) with the opaque EDID type > (struct drm_edid) on amdgpu_dm_connector for consistency. It may also > prevent mismatch of approaches in different parts of the driver code. > Working in progress. It was only exercised with IGT tests. > > v2: use const to fix warnings (Alex Hung) > v3: fix general protection fault on mst > > Signed-off-by: Melissa Wen <mwen@igalia.com> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +++++++++---------- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +- > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +- > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++--- > 4 files changed, 67 insertions(+), 68 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 280562707cd0..bbbf9c9d40d5 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect( > aconnector->dc_sink = sink; > dc_sink_retain(aconnector->dc_sink); > if (sink->dc_edid.length == 0) { > - aconnector->edid = NULL; > + drm_edid_free(aconnector->edid); If aconnector->edid is used as some kind of cache, it might be prudent to still set it to NULL. It's up to the amd maintainers, but personally I've renamed any ->edid fields to ->drm_edid when I've done these conversions to ensure every single place is covered, and also later stable backports will give a build failure if the change is not taken into account. > if (aconnector->dc_link->aux_mode) { > drm_dp_cec_unset_edid( > &aconnector->dm_dp_aux.aux); > } > } else { > - aconnector->edid = > - (struct edid *)sink->dc_edid.raw_edid; > + const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid; > + aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length); > > + /* FIXME: Get rid of drm_edid_raw() */ > if (aconnector->dc_link->aux_mode) > drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux, > - aconnector->edid); > + drm_edid_raw(aconnector->edid)); The goal should be to switch to use CEC functions that take the source physical address directly instead of parsing the EDID. > } > > if (!aconnector->timing_requested) { > @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect( > "failed to create aconnector->requested_timing\n"); > } > > - drm_connector_update_edid_property(connector, aconnector->edid); > + drm_edid_connector_update(connector, aconnector->edid); > amdgpu_dm_update_freesync_caps(connector, aconnector->edid); > update_connector_ext_caps(aconnector); > } else { > drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux); > amdgpu_dm_update_freesync_caps(connector, NULL); > - drm_connector_update_edid_property(connector, NULL); > + drm_edid_connector_update(connector, NULL); > aconnector->num_modes = 0; > dc_sink_release(aconnector->dc_sink); > aconnector->dc_sink = NULL; > - aconnector->edid = NULL; > + drm_edid_free(aconnector->edid); > kfree(aconnector->timing_requested); > aconnector->timing_requested = NULL; > /* Set CP to DESIRED if it was ENABLED, so we can re-enable it again on hotplug */ > @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) > struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); > struct dc_link *dc_link = aconnector->dc_link; > struct dc_sink *dc_em_sink = aconnector->dc_em_sink; > - struct edid *edid; > - struct i2c_adapter *ddc; > - > - if (dc_link && dc_link->aux_mode) > - ddc = &aconnector->dm_dp_aux.aux.ddc; > - else > - ddc = &aconnector->i2c->base; > + const struct drm_edid *drm_edid; > > /* > * Note: drm_get_edid gets edid in the following order: > @@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) > * 2) firmware EDID if set via edid_firmware module parameter > * 3) regular DDC read. > */ > - edid = drm_get_edid(connector, ddc); > - if (!edid) { > + drm_edid = drm_edid_read(connector); You should call drm_edid_connector_update() here to clear the EDID property etc. if it's NULL. Ditto everywhere. Due to a historical mistake, drm_get_edid() does that for you, but shouldn't really. > + > + if (!drm_edid) { > DRM_ERROR("No EDID found on connector: %s.\n", connector->name); > return; > } > - > - aconnector->edid = edid; > - > + aconnector->edid = drm_edid; > + drm_edid_connector_update(connector, drm_edid); > /* Update emulated (virtual) sink's EDID */ > if (dc_em_sink && dc_link) { > + const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > + > memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps)); > - memmove(dc_em_sink->dc_edid.raw_edid, edid, (edid->extensions + 1) * EDID_LENGTH); > + memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH); > dm_helpers_parse_edid_caps( > dc_link, > &dc_em_sink->dc_edid, > @@ -6716,18 +6713,12 @@ 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 dc_link *dc_link = aconnector->dc_link; > struct dc_sink_init_data init_params = { > .link = aconnector->dc_link, > .sink_signal = SIGNAL_TYPE_VIRTUAL > }; > - struct edid *edid; > - struct i2c_adapter *ddc; > - > - if (dc_link->aux_mode) > - ddc = &aconnector->dm_dp_aux.aux.ddc; > - else > - ddc = &aconnector->i2c->base; > + const struct drm_edid *drm_edid; > + const struct edid *edid; > > /* > * Note: drm_get_edid gets edid in the following order: > @@ -6735,17 +6726,19 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) > * 2) firmware EDID if set via edid_firmware module parameter > * 3) regular DDC read. > */ > - edid = drm_get_edid(connector, ddc); > - if (!edid) { > + drm_edid = drm_edid_read(connector); > + if (!drm_edid) { > DRM_ERROR("No EDID found on connector: %s.\n", connector->name); > return; > } > > - if (drm_detect_hdmi_monitor(edid)) > + if (connector->display_info.is_hdmi) > init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A; > > - aconnector->edid = edid; > + aconnector->edid = drm_edid; > + drm_edid_connector_update(connector, drm_edid); > > + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > aconnector->dc_em_sink = dc_link_add_remote_sink( > aconnector->dc_link, > (uint8_t *)edid, > @@ -7428,16 +7421,16 @@ static void amdgpu_set_panel_orientation(struct drm_connector *connector) > } > > static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector, > - struct edid *edid) > + const struct drm_edid *drm_edid) > { > struct amdgpu_dm_connector *amdgpu_dm_connector = > to_amdgpu_dm_connector(connector); > > - if (edid) { > + if (drm_edid) { > /* empty probed_modes */ > INIT_LIST_HEAD(&connector->probed_modes); > amdgpu_dm_connector->num_modes = > - drm_add_edid_modes(connector, edid); > + drm_edid_connector_add_modes(connector); > > /* sorting the probed modes before calling function > * amdgpu_dm_get_native_mode() since EDID can have > @@ -7451,10 +7444,10 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector, > amdgpu_dm_get_native_mode(connector); > > /* Freesync capabilities are reset by calling > - * drm_add_edid_modes() and need to be > + * drm_edid_connector_add_modes() and need to be > * restored here. > */ > - amdgpu_dm_update_freesync_caps(connector, edid); > + amdgpu_dm_update_freesync_caps(connector, drm_edid); > } else { > amdgpu_dm_connector->num_modes = 0; > } > @@ -7550,12 +7543,12 @@ static uint add_fs_modes(struct amdgpu_dm_connector *aconnector) > } > > static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector, > - struct edid *edid) > + const struct drm_edid *drm_edid) > { > struct amdgpu_dm_connector *amdgpu_dm_connector = > to_amdgpu_dm_connector(connector); > > - if (!(amdgpu_freesync_vid_mode && edid)) > + if (!(amdgpu_freesync_vid_mode && drm_edid)) > return; > > if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10) > @@ -7568,23 +7561,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector) > struct amdgpu_dm_connector *amdgpu_dm_connector = > to_amdgpu_dm_connector(connector); > struct drm_encoder *encoder; > - struct edid *edid = amdgpu_dm_connector->edid; > + const struct drm_edid *drm_edid = amdgpu_dm_connector->edid; > struct dc_link_settings *verified_link_cap = > &amdgpu_dm_connector->dc_link->verified_link_cap; > const struct dc *dc = amdgpu_dm_connector->dc_link->dc; > > encoder = amdgpu_dm_connector_to_encoder(connector); > > - if (!drm_edid_is_valid(edid)) { > + if (!drm_edid_valid(drm_edid)) { I don't think there should be reasons to validate the EDID at this point. The EDID reading functions do that, and any amd driver specific mechanisms should do that separately before setting ->edid. So it should be just a NULL check here. > amdgpu_dm_connector->num_modes = > drm_add_modes_noedid(connector, 640, 480); > if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING) > amdgpu_dm_connector->num_modes += > drm_add_modes_noedid(connector, 1920, 1080); > } else { > - amdgpu_dm_connector_ddc_get_modes(connector, edid); > + amdgpu_dm_connector_ddc_get_modes(connector, drm_edid); > amdgpu_dm_connector_add_common_modes(encoder, connector); > - amdgpu_dm_connector_add_freesync_modes(connector, edid); > + amdgpu_dm_connector_add_freesync_modes(connector, drm_edid); > } > amdgpu_dm_fbc_init(connector); > > @@ -11153,7 +11146,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector, > } > > static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector, > - struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) > + const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) > { > u8 *edid_ext = NULL; > int i; > @@ -11188,7 +11181,8 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector, > } > > static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector, > - struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) > + const struct edid *edid, > + struct amdgpu_hdmi_vsdb_info *vsdb_info) > { > u8 *edid_ext = NULL; > int i; > @@ -11230,19 +11224,19 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector, > * FreeSync parameters. > */ > void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > - struct edid *edid) > + const struct drm_edid *drm_edid) > { > int i = 0; > - struct detailed_timing *timing; > - struct detailed_non_pixel *data; > - struct detailed_data_monitor_range *range; > + const struct detailed_timing *timing; > + const struct detailed_non_pixel *data; > + const struct detailed_data_monitor_range *range; > struct amdgpu_dm_connector *amdgpu_dm_connector = > to_amdgpu_dm_connector(connector); > struct dm_connector_state *dm_con_state = NULL; > struct dc_sink *sink; > - > struct amdgpu_device *adev = drm_to_adev(connector->dev); > struct amdgpu_hdmi_vsdb_info vsdb_info = {0}; > + const struct edid *edid; > bool freesync_capable = false; > enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE; > > @@ -11255,7 +11249,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > amdgpu_dm_connector->dc_sink : > amdgpu_dm_connector->dc_em_sink; > > - if (!edid || !sink) { > + if (!drm_edid || !sink) { > dm_con_state = to_dm_connector_state(connector->state); > > amdgpu_dm_connector->min_vfreq = 0; > @@ -11272,6 +11266,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > if (!adev->dm.freesync_module) > goto update; > > + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT || > sink->sink_signal == SIGNAL_TYPE_EDP)) { > bool edid_check_required = false; > @@ -11341,7 +11336,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP; > } > > - } else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { > + } else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { > i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info); > if (i >= 0 && vsdb_info.freesync_supported) { > amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz; > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > index 67647bb5999b..384f68203590 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > @@ -637,7 +637,7 @@ struct amdgpu_dm_connector { > > /* we need to mind the EDID between detect > and get modes due to analog/digital/tvencoder */ > - struct edid *edid; > + const struct drm_edid *edid; As I said, I'd make this drm_edid. But not my call. BR, Jani. > > /* shared with amdgpu */ > struct amdgpu_hpd hpd; > @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev, > struct drm_connector *connector); > > void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > - struct edid *edid); > + const struct drm_edid *drm_edid); > > void amdgpu_dm_trigger_timing_sync(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index c27063305a13..1a37dff9c2b6 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > @@ -901,7 +901,8 @@ enum dc_edid_status dm_helpers_read_local_edid( > struct i2c_adapter *ddc; > int retry = 3; > enum dc_edid_status edid_status; > - struct edid *edid; > + const struct drm_edid *drm_edid; > + const struct edid *edid; > > if (link->aux_mode) > ddc = &aconnector->dm_dp_aux.aux.ddc; > @@ -913,7 +914,8 @@ enum dc_edid_status dm_helpers_read_local_edid( > */ > do { > > - edid = drm_get_edid(&aconnector->base, ddc); > + drm_edid = drm_edid_read_ddc(connector, ddc); > + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > > /* DP Compliance Test 4.2.2.6 */ > if (link->aux_mode && connector->edid_corrupt) > @@ -931,7 +933,7 @@ enum dc_edid_status dm_helpers_read_local_edid( > memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length); > > /* We don't need the original edid anymore */ > - kfree(edid); > + drm_edid_free(drm_edid); > > edid_status = dm_helpers_parse_edid_caps( > link, > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > index 941e96f100f4..a28004f9378a 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > @@ -128,7 +128,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector) > dc_sink_release(aconnector->dc_sink); > } > > - kfree(aconnector->edid); > + drm_edid_free(aconnector->edid); > > drm_connector_cleanup(connector); > drm_dp_mst_put_port_malloc(aconnector->mst_output_port); > @@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > return drm_add_edid_modes(connector, NULL); > > if (!aconnector->edid) { > - struct edid *edid; > + const struct drm_edid *drm_edid; > > - edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port); > + drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port); > > - if (!edid) { > + if (!drm_edid) { > amdgpu_dm_set_mst_status(&aconnector->mst_status, > MST_REMOTE_EDID, false); > > - drm_connector_update_edid_property( > + drm_edid_connector_update( > &aconnector->base, > NULL); > > @@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > return ret; > } > > - aconnector->edid = edid; > + aconnector->edid = drm_edid; > amdgpu_dm_set_mst_status(&aconnector->mst_status, > MST_REMOTE_EDID, true); > } > @@ -355,10 +355,13 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > struct dc_sink_init_data init_params = { > .link = aconnector->dc_link, > .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST }; > + const struct edid *edid; > + > + edid = drm_edid_raw(aconnector->edid); // FIXME: Get rid of drm_edid_raw() > dc_sink = dc_link_add_remote_sink( > aconnector->dc_link, > - (uint8_t *)aconnector->edid, > - (aconnector->edid->extensions + 1) * EDID_LENGTH, > + (uint8_t *)edid, > + (edid->extensions + 1) * EDID_LENGTH, > &init_params); > > if (!dc_sink) { > @@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > } > } > > - drm_connector_update_edid_property( > - &aconnector->base, aconnector->edid); > + drm_edid_connector_update(&aconnector->base, aconnector->edid); > > - ret = drm_add_edid_modes(connector, aconnector->edid); > + ret = drm_edid_connector_add_modes(connector); > > return ret; > } -- Jani Nikula, Intel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid 2024-03-28 9:33 ` Jani Nikula @ 2024-04-08 13:39 ` Melissa Wen 0 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-04-08 13:39 UTC (permalink / raw) To: Jani Nikula Cc: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel, amd-gfx, kernel-dev, dri-devel On 03/28, Jani Nikula wrote: > On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote: > > Replace raw edid handling (struct edid) with the opaque EDID type > > (struct drm_edid) on amdgpu_dm_connector for consistency. It may also > > prevent mismatch of approaches in different parts of the driver code. > > Working in progress. It was only exercised with IGT tests. > > > > v2: use const to fix warnings (Alex Hung) > > v3: fix general protection fault on mst > > > > Signed-off-by: Melissa Wen <mwen@igalia.com> > > --- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +++++++++---------- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +- > > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +- > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++--- > > 4 files changed, 67 insertions(+), 68 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 280562707cd0..bbbf9c9d40d5 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect( > > aconnector->dc_sink = sink; > > dc_sink_retain(aconnector->dc_sink); > > if (sink->dc_edid.length == 0) { > > - aconnector->edid = NULL; > > + drm_edid_free(aconnector->edid); > > If aconnector->edid is used as some kind of cache, it might be prudent > to still set it to NULL. Makes sense. Thanks for pointing it out. > > It's up to the amd maintainers, but personally I've renamed any ->edid > fields to ->drm_edid when I've done these conversions to ensure every > single place is covered, and also later stable backports will give a > build failure if the change is not taken into account. I see. Good points. I'll follow your advice and do this name change in the next version. > > > if (aconnector->dc_link->aux_mode) { > > drm_dp_cec_unset_edid( > > &aconnector->dm_dp_aux.aux); > > } > > } else { > > - aconnector->edid = > > - (struct edid *)sink->dc_edid.raw_edid; > > + const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid; > > + aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length); > > > > + /* FIXME: Get rid of drm_edid_raw() */ > > if (aconnector->dc_link->aux_mode) > > drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux, > > - aconnector->edid); > > + drm_edid_raw(aconnector->edid)); > > The goal should be to switch to use CEC functions that take the source > physical address directly instead of parsing the EDID. Yes, I understand I have the same goal as in the next patch: - https://lore.kernel.org/dri-devel/20240327214953.367126-5-mwen@igalia.com/ Am I missing something? > > > } > > > > if (!aconnector->timing_requested) { > > @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect( > > "failed to create aconnector->requested_timing\n"); > > } > > > > - drm_connector_update_edid_property(connector, aconnector->edid); > > + drm_edid_connector_update(connector, aconnector->edid); > > amdgpu_dm_update_freesync_caps(connector, aconnector->edid); > > update_connector_ext_caps(aconnector); > > } else { > > drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux); > > amdgpu_dm_update_freesync_caps(connector, NULL); > > - drm_connector_update_edid_property(connector, NULL); > > + drm_edid_connector_update(connector, NULL); > > aconnector->num_modes = 0; > > dc_sink_release(aconnector->dc_sink); > > aconnector->dc_sink = NULL; > > - aconnector->edid = NULL; > > + drm_edid_free(aconnector->edid); > > kfree(aconnector->timing_requested); > > aconnector->timing_requested = NULL; > > /* Set CP to DESIRED if it was ENABLED, so we can re-enable it again on hotplug */ > > @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) > > struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); > > struct dc_link *dc_link = aconnector->dc_link; > > struct dc_sink *dc_em_sink = aconnector->dc_em_sink; > > - struct edid *edid; > > - struct i2c_adapter *ddc; > > - > > - if (dc_link && dc_link->aux_mode) > > - ddc = &aconnector->dm_dp_aux.aux.ddc; > > - else > > - ddc = &aconnector->i2c->base; > > + const struct drm_edid *drm_edid; > > > > /* > > * Note: drm_get_edid gets edid in the following order: > > @@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) > > * 2) firmware EDID if set via edid_firmware module parameter > > * 3) regular DDC read. > > */ > > - edid = drm_get_edid(connector, ddc); > > - if (!edid) { > > + drm_edid = drm_edid_read(connector); > > You should call drm_edid_connector_update() here to clear the EDID > property etc. if it's NULL. > > Ditto everywhere. > > Due to a historical mistake, drm_get_edid() does that for you, but > shouldn't really. Oh, okay. I'll review the overall usage in this series. Thanks for warning. > > > + > > + if (!drm_edid) { > > DRM_ERROR("No EDID found on connector: %s.\n", connector->name); > > return; > > } > > - > > - aconnector->edid = edid; > > - > > + aconnector->edid = drm_edid; > > + drm_edid_connector_update(connector, drm_edid); > > /* Update emulated (virtual) sink's EDID */ > > if (dc_em_sink && dc_link) { > > + const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > > + > > memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps)); > > - memmove(dc_em_sink->dc_edid.raw_edid, edid, (edid->extensions + 1) * EDID_LENGTH); > > + memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH); > > dm_helpers_parse_edid_caps( > > dc_link, > > &dc_em_sink->dc_edid, > > @@ -6716,18 +6713,12 @@ 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 dc_link *dc_link = aconnector->dc_link; > > struct dc_sink_init_data init_params = { > > .link = aconnector->dc_link, > > .sink_signal = SIGNAL_TYPE_VIRTUAL > > }; > > - struct edid *edid; > > - struct i2c_adapter *ddc; > > - > > - if (dc_link->aux_mode) > > - ddc = &aconnector->dm_dp_aux.aux.ddc; > > - else > > - ddc = &aconnector->i2c->base; > > + const struct drm_edid *drm_edid; > > + const struct edid *edid; > > > > /* > > * Note: drm_get_edid gets edid in the following order: > > @@ -6735,17 +6726,19 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) > > * 2) firmware EDID if set via edid_firmware module parameter > > * 3) regular DDC read. > > */ > > - edid = drm_get_edid(connector, ddc); > > - if (!edid) { > > + drm_edid = drm_edid_read(connector); > > + if (!drm_edid) { > > DRM_ERROR("No EDID found on connector: %s.\n", connector->name); > > return; > > } > > > > - if (drm_detect_hdmi_monitor(edid)) > > + if (connector->display_info.is_hdmi) > > init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A; > > > > - aconnector->edid = edid; > > + aconnector->edid = drm_edid; > > + drm_edid_connector_update(connector, drm_edid); > > > > + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > > aconnector->dc_em_sink = dc_link_add_remote_sink( > > aconnector->dc_link, > > (uint8_t *)edid, > > @@ -7428,16 +7421,16 @@ static void amdgpu_set_panel_orientation(struct drm_connector *connector) > > } > > > > static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector, > > - struct edid *edid) > > + const struct drm_edid *drm_edid) > > { > > struct amdgpu_dm_connector *amdgpu_dm_connector = > > to_amdgpu_dm_connector(connector); > > > > - if (edid) { > > + if (drm_edid) { > > /* empty probed_modes */ > > INIT_LIST_HEAD(&connector->probed_modes); > > amdgpu_dm_connector->num_modes = > > - drm_add_edid_modes(connector, edid); > > + drm_edid_connector_add_modes(connector); > > > > /* sorting the probed modes before calling function > > * amdgpu_dm_get_native_mode() since EDID can have > > @@ -7451,10 +7444,10 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector, > > amdgpu_dm_get_native_mode(connector); > > > > /* Freesync capabilities are reset by calling > > - * drm_add_edid_modes() and need to be > > + * drm_edid_connector_add_modes() and need to be > > * restored here. > > */ > > - amdgpu_dm_update_freesync_caps(connector, edid); > > + amdgpu_dm_update_freesync_caps(connector, drm_edid); > > } else { > > amdgpu_dm_connector->num_modes = 0; > > } > > @@ -7550,12 +7543,12 @@ static uint add_fs_modes(struct amdgpu_dm_connector *aconnector) > > } > > > > static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector, > > - struct edid *edid) > > + const struct drm_edid *drm_edid) > > { > > struct amdgpu_dm_connector *amdgpu_dm_connector = > > to_amdgpu_dm_connector(connector); > > > > - if (!(amdgpu_freesync_vid_mode && edid)) > > + if (!(amdgpu_freesync_vid_mode && drm_edid)) > > return; > > > > if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10) > > @@ -7568,23 +7561,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector) > > struct amdgpu_dm_connector *amdgpu_dm_connector = > > to_amdgpu_dm_connector(connector); > > struct drm_encoder *encoder; > > - struct edid *edid = amdgpu_dm_connector->edid; > > + const struct drm_edid *drm_edid = amdgpu_dm_connector->edid; > > struct dc_link_settings *verified_link_cap = > > &amdgpu_dm_connector->dc_link->verified_link_cap; > > const struct dc *dc = amdgpu_dm_connector->dc_link->dc; > > > > encoder = amdgpu_dm_connector_to_encoder(connector); > > > > - if (!drm_edid_is_valid(edid)) { > > + if (!drm_edid_valid(drm_edid)) { > > I don't think there should be reasons to validate the EDID at this > point. The EDID reading functions do that, and any amd driver specific > mechanisms should do that separately before setting ->edid. So it should > be just a NULL check here. > > > amdgpu_dm_connector->num_modes = > > drm_add_modes_noedid(connector, 640, 480); > > if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING) > > amdgpu_dm_connector->num_modes += > > drm_add_modes_noedid(connector, 1920, 1080); > > } else { > > - amdgpu_dm_connector_ddc_get_modes(connector, edid); > > + amdgpu_dm_connector_ddc_get_modes(connector, drm_edid); > > amdgpu_dm_connector_add_common_modes(encoder, connector); > > - amdgpu_dm_connector_add_freesync_modes(connector, edid); > > + amdgpu_dm_connector_add_freesync_modes(connector, drm_edid); > > } > > amdgpu_dm_fbc_init(connector); > > > > @@ -11153,7 +11146,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector, > > } > > > > static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector, > > - struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) > > + const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) > > { > > u8 *edid_ext = NULL; > > int i; > > @@ -11188,7 +11181,8 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector, > > } > > > > static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector, > > - struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info) > > + const struct edid *edid, > > + struct amdgpu_hdmi_vsdb_info *vsdb_info) > > { > > u8 *edid_ext = NULL; > > int i; > > @@ -11230,19 +11224,19 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector, > > * FreeSync parameters. > > */ > > void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > > - struct edid *edid) > > + const struct drm_edid *drm_edid) > > { > > int i = 0; > > - struct detailed_timing *timing; > > - struct detailed_non_pixel *data; > > - struct detailed_data_monitor_range *range; > > + const struct detailed_timing *timing; > > + const struct detailed_non_pixel *data; > > + const struct detailed_data_monitor_range *range; > > struct amdgpu_dm_connector *amdgpu_dm_connector = > > to_amdgpu_dm_connector(connector); > > struct dm_connector_state *dm_con_state = NULL; > > struct dc_sink *sink; > > - > > struct amdgpu_device *adev = drm_to_adev(connector->dev); > > struct amdgpu_hdmi_vsdb_info vsdb_info = {0}; > > + const struct edid *edid; > > bool freesync_capable = false; > > enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE; > > > > @@ -11255,7 +11249,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > > amdgpu_dm_connector->dc_sink : > > amdgpu_dm_connector->dc_em_sink; > > > > - if (!edid || !sink) { > > + if (!drm_edid || !sink) { > > dm_con_state = to_dm_connector_state(connector->state); > > > > amdgpu_dm_connector->min_vfreq = 0; > > @@ -11272,6 +11266,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > > if (!adev->dm.freesync_module) > > goto update; > > > > + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > > if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT || > > sink->sink_signal == SIGNAL_TYPE_EDP)) { > > bool edid_check_required = false; > > @@ -11341,7 +11336,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > > amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP; > > } > > > > - } else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { > > + } else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { > > i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info); > > if (i >= 0 && vsdb_info.freesync_supported) { > > amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz; > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > > index 67647bb5999b..384f68203590 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > > @@ -637,7 +637,7 @@ struct amdgpu_dm_connector { > > > > /* we need to mind the EDID between detect > > and get modes due to analog/digital/tvencoder */ > > - struct edid *edid; > > + const struct drm_edid *edid; > > As I said, I'd make this drm_edid. But not my call. > > BR, > Jani. > > > > > /* shared with amdgpu */ > > struct amdgpu_hpd hpd; > > @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev, > > struct drm_connector *connector); > > > > void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, > > - struct edid *edid); > > + const struct drm_edid *drm_edid); > > > > void amdgpu_dm_trigger_timing_sync(struct drm_device *dev); > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > index c27063305a13..1a37dff9c2b6 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > @@ -901,7 +901,8 @@ enum dc_edid_status dm_helpers_read_local_edid( > > struct i2c_adapter *ddc; > > int retry = 3; > > enum dc_edid_status edid_status; > > - struct edid *edid; > > + const struct drm_edid *drm_edid; > > + const struct edid *edid; > > > > if (link->aux_mode) > > ddc = &aconnector->dm_dp_aux.aux.ddc; > > @@ -913,7 +914,8 @@ enum dc_edid_status dm_helpers_read_local_edid( > > */ > > do { > > > > - edid = drm_get_edid(&aconnector->base, ddc); > > + drm_edid = drm_edid_read_ddc(connector, ddc); > > + edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() > > > > /* DP Compliance Test 4.2.2.6 */ > > if (link->aux_mode && connector->edid_corrupt) > > @@ -931,7 +933,7 @@ enum dc_edid_status dm_helpers_read_local_edid( > > memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length); > > > > /* We don't need the original edid anymore */ > > - kfree(edid); > > + drm_edid_free(drm_edid); > > > > edid_status = dm_helpers_parse_edid_caps( > > link, > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > index 941e96f100f4..a28004f9378a 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > @@ -128,7 +128,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector) > > dc_sink_release(aconnector->dc_sink); > > } > > > > - kfree(aconnector->edid); > > + drm_edid_free(aconnector->edid); > > > > drm_connector_cleanup(connector); > > drm_dp_mst_put_port_malloc(aconnector->mst_output_port); > > @@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > > return drm_add_edid_modes(connector, NULL); > > > > if (!aconnector->edid) { > > - struct edid *edid; > > + const struct drm_edid *drm_edid; > > > > - edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port); > > + drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port); > > > > - if (!edid) { > > + if (!drm_edid) { > > amdgpu_dm_set_mst_status(&aconnector->mst_status, > > MST_REMOTE_EDID, false); > > > > - drm_connector_update_edid_property( > > + drm_edid_connector_update( > > &aconnector->base, > > NULL); > > > > @@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > > return ret; > > } > > > > - aconnector->edid = edid; > > + aconnector->edid = drm_edid; > > amdgpu_dm_set_mst_status(&aconnector->mst_status, > > MST_REMOTE_EDID, true); > > } > > @@ -355,10 +355,13 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > > struct dc_sink_init_data init_params = { > > .link = aconnector->dc_link, > > .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST }; > > + const struct edid *edid; > > + > > + edid = drm_edid_raw(aconnector->edid); // FIXME: Get rid of drm_edid_raw() > > dc_sink = dc_link_add_remote_sink( > > aconnector->dc_link, > > - (uint8_t *)aconnector->edid, > > - (aconnector->edid->extensions + 1) * EDID_LENGTH, > > + (uint8_t *)edid, > > + (edid->extensions + 1) * EDID_LENGTH, > > &init_params); > > > > if (!dc_sink) { > > @@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector) > > } > > } > > > > - drm_connector_update_edid_property( > > - &aconnector->base, aconnector->edid); > > + drm_edid_connector_update(&aconnector->base, aconnector->edid); > > > > - ret = drm_add_edid_modes(connector, aconnector->edid); > > + ret = drm_edid_connector_add_modes(connector); > > > > return ret; > > } > > -- > Jani Nikula, Intel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH v3 4/6] drm/amd/display: switch to setting physical address directly 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen ` (2 preceding siblings ...) 2024-03-27 21:44 ` [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen @ 2024-03-27 21:44 ` Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 5/6] drm/amd/display: always call connector_update when parsing freesync_caps Melissa Wen ` (2 subsequent siblings) 6 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel Connectors have source physical address available in display info. Use drm_dp_cec_attach() to use it instead of parsing the EDID again. Signed-off-by: Melissa Wen <mwen@igalia.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 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 bbbf9c9d40d5..edc5418db5be 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3277,11 +3277,11 @@ void amdgpu_dm_update_connector_after_detect( } else { const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid; aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length); + drm_edid_connector_update(connector, aconnector->edid); - /* FIXME: Get rid of drm_edid_raw() */ if (aconnector->dc_link->aux_mode) - drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux, - drm_edid_raw(aconnector->edid)); + drm_dp_cec_attach(&aconnector->dm_dp_aux.aux, + connector->display_info.source_physical_address); } if (!aconnector->timing_requested) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH v3 5/6] drm/amd/display: always call connector_update when parsing freesync_caps 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen ` (3 preceding siblings ...) 2024-03-27 21:44 ` [RFC PATCH v3 4/6] drm/amd/display: switch to setting physical address directly Melissa Wen @ 2024-03-27 21:44 ` Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 6/6] drm/amd/display: remove redundant freesync parser for DP Melissa Wen 2024-03-28 9:21 ` [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Jani Nikula 6 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel Update connector caps with drm_edid data before parsing info for freesync. Signed-off-by: Melissa Wen <mwen@igalia.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 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 edc5418db5be..b3c396d626e9 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3292,13 +3292,11 @@ void amdgpu_dm_update_connector_after_detect( "failed to create aconnector->requested_timing\n"); } - drm_edid_connector_update(connector, aconnector->edid); amdgpu_dm_update_freesync_caps(connector, aconnector->edid); update_connector_ext_caps(aconnector); } else { drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux); amdgpu_dm_update_freesync_caps(connector, NULL); - drm_edid_connector_update(connector, NULL); aconnector->num_modes = 0; dc_sink_release(aconnector->dc_sink); aconnector->dc_sink = NULL; @@ -11249,6 +11247,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->dc_sink : amdgpu_dm_connector->dc_em_sink; + drm_edid_connector_update(connector, drm_edid); + if (!drm_edid || !sink) { dm_con_state = to_dm_connector_state(connector->state); -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH v3 6/6] drm/amd/display: remove redundant freesync parser for DP 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen ` (4 preceding siblings ...) 2024-03-27 21:44 ` [RFC PATCH v3 5/6] drm/amd/display: always call connector_update when parsing freesync_caps Melissa Wen @ 2024-03-27 21:44 ` Melissa Wen 2024-03-28 9:21 ` [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Jani Nikula 6 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-03-27 21:44 UTC (permalink / raw) To: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: Jani Nikula, amd-gfx, kernel-dev, dri-devel When updating connector under drm_edid infrastructure, many calculations and validations are already done and become redundant inside AMD driver. Remove those driver-specific code in favor of the DRM common code. Signed-off-by: Melissa Wen <mwen@igalia.com> --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 86 +------------------ 1 file changed, 4 insertions(+), 82 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 b3c396d626e9..7e0f93de27e6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10994,24 +10994,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, return ret; } -static bool is_dp_capable_without_timing_msa(struct dc *dc, - struct amdgpu_dm_connector *amdgpu_dm_connector) -{ - u8 dpcd_data; - bool capable = false; - - if (amdgpu_dm_connector->dc_link && - dm_helpers_dp_read_dpcd( - NULL, - amdgpu_dm_connector->dc_link, - DP_DOWN_STREAM_PORT_COUNT, - &dpcd_data, - sizeof(dpcd_data))) { - capable = (dpcd_data & DP_MSA_TIMING_PAR_IGNORED) ? true:false; - } - - return capable; -} static bool dm_edid_parser_send_cea(struct amdgpu_display_manager *dm, unsigned int offset, @@ -11225,9 +11207,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, const struct drm_edid *drm_edid) { int i = 0; - const struct detailed_timing *timing; - const struct detailed_non_pixel *data; - const struct detailed_data_monitor_range *range; struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector); struct dm_connector_state *dm_con_state = NULL; @@ -11254,8 +11233,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->min_vfreq = 0; amdgpu_dm_connector->max_vfreq = 0; - connector->display_info.monitor_range.min_vfreq = 0; - connector->display_info.monitor_range.max_vfreq = 0; freesync_capable = false; goto update; @@ -11269,65 +11246,11 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw() if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT || sink->sink_signal == SIGNAL_TYPE_EDP)) { - bool edid_check_required = false; + amdgpu_dm_connector->min_vfreq = connector->display_info.monitor_range.min_vfreq; + amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq; + if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10) + freesync_capable = true; - if (is_dp_capable_without_timing_msa(adev->dm.dc, - amdgpu_dm_connector)) { - if (edid->features & DRM_EDID_FEATURE_CONTINUOUS_FREQ) { - freesync_capable = true; - amdgpu_dm_connector->min_vfreq = connector->display_info.monitor_range.min_vfreq; - amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq; - } else { - edid_check_required = edid->version > 1 || - (edid->version == 1 && - edid->revision > 1); - } - } - - if (edid_check_required) { - for (i = 0; i < 4; i++) { - - timing = &edid->detailed_timings[i]; - data = &timing->data.other_data; - range = &data->data.range; - /* - * Check if monitor has continuous frequency mode - */ - if (data->type != EDID_DETAIL_MONITOR_RANGE) - continue; - /* - * Check for flag range limits only. If flag == 1 then - * no additional timing information provided. - * Default GTF, GTF Secondary curve and CVT are not - * supported - */ - if (range->flags != 1) - continue; - - connector->display_info.monitor_range.min_vfreq = range->min_vfreq; - connector->display_info.monitor_range.max_vfreq = range->max_vfreq; - - if (edid->revision >= 4) { - if (data->pad2 & DRM_EDID_RANGE_OFFSET_MIN_VFREQ) - connector->display_info.monitor_range.min_vfreq += 255; - if (data->pad2 & DRM_EDID_RANGE_OFFSET_MAX_VFREQ) - connector->display_info.monitor_range.max_vfreq += 255; - } - - amdgpu_dm_connector->min_vfreq = - connector->display_info.monitor_range.min_vfreq; - amdgpu_dm_connector->max_vfreq = - connector->display_info.monitor_range.max_vfreq; - - break; - } - - if (amdgpu_dm_connector->max_vfreq - - amdgpu_dm_connector->min_vfreq > 10) { - - freesync_capable = true; - } - } parse_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info); if (vsdb_info.replay_mode) { @@ -11335,7 +11258,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_info.amd_vsdb_version; amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP; } - } else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) { i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info); if (i >= 0 && vsdb_info.freesync_supported) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen ` (5 preceding siblings ...) 2024-03-27 21:44 ` [RFC PATCH v3 6/6] drm/amd/display: remove redundant freesync parser for DP Melissa Wen @ 2024-03-28 9:21 ` Jani Nikula 2024-04-08 13:50 ` Melissa Wen 6 siblings, 1 reply; 11+ messages in thread From: Jani Nikula @ 2024-03-28 9:21 UTC (permalink / raw) To: Melissa Wen, Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel Cc: amd-gfx, kernel-dev, dri-devel On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote: > 2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are > in drm_edid halpers, but there are still a few that are not managed by > them yet. For example: > ``` > edid_caps->serial_number = edid_buf->serial; > edid_caps->manufacture_week = edid_buf->mfg_week; > edid_caps->manufacture_year = edid_buf->mfg_year; > ``` > AFAIU I see the same pending issue in i915/pnpid_get_panel_type() - > that still uses drm_edid_raw(). See https://lore.kernel.org/r/cover.1711015462.git.jani.nikula@intel.com BR, Jani. -- Jani Nikula, Intel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid 2024-03-28 9:21 ` [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Jani Nikula @ 2024-04-08 13:50 ` Melissa Wen 0 siblings, 0 replies; 11+ messages in thread From: Melissa Wen @ 2024-04-08 13:50 UTC (permalink / raw) To: Jani Nikula Cc: Harry Wentland, Alex Deucher, Leo Li, Rodrigo Siqueira, christian.koenig, Xinhui.Pan, Alex Hung, Mario Limonciello, airlied, daniel, amd-gfx, kernel-dev, dri-devel On 03/28, Jani Nikula wrote: > On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote: > > 2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are > > in drm_edid halpers, but there are still a few that are not managed by > > them yet. For example: > > ``` > > edid_caps->serial_number = edid_buf->serial; > > edid_caps->manufacture_week = edid_buf->mfg_week; > > edid_caps->manufacture_year = edid_buf->mfg_year; > > ``` > > AFAIU I see the same pending issue in i915/pnpid_get_panel_type() - > > that still uses drm_edid_raw(). > > See https://lore.kernel.org/r/cover.1711015462.git.jani.nikula@intel.com Thanks! I'll work on top of your series. Melissa > > BR, > Jani. > > > -- > Jani Nikula, Intel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-04-08 13:51 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-27 21:44 [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 1/6] drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 2/6] drm/amd/display: clean unused variables for hdmi freesync parser Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen 2024-03-28 9:33 ` Jani Nikula 2024-04-08 13:39 ` Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 4/6] drm/amd/display: switch to setting physical address directly Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 5/6] drm/amd/display: always call connector_update when parsing freesync_caps Melissa Wen 2024-03-27 21:44 ` [RFC PATCH v3 6/6] drm/amd/display: remove redundant freesync parser for DP Melissa Wen 2024-03-28 9:21 ` [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Jani Nikula 2024-04-08 13:50 ` Melissa Wen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.