Re: [PATCH 09/12] drm/amd/display: Switch amdgpu_dm_connector to struct drm_edid

From: Jani Nikula
Date: Mon Aug 19 2024 - 04:26:48 EST

Next message: 胡连勤: " 答复: 答复: [PATCH v1] usb: gadget: u_serial: check Null pointer in EP callback"
Previous message: Bartosz Golaszewski: "[PATCH v2 2/2] PCI/pwrctl: put the bus rescan on a different thread"
In reply to: Thomas Weißschuh: "[PATCH 09/12] drm/amd/display: Switch amdgpu_dm_connector to struct drm_edid"
Next in thread: Thomas Weißschuh: "[PATCH 04/12] drm/amd/display: Simplify raw_edid handling in dm_helpers_parse_edid_caps()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, 18 Aug 2024, Thomas Weißschuh <linux@xxxxxxxxxxxxxx> wrote:
> "struct drm_edid" is the safe and recommended alternative to "struct edid".
>
> Rename the member to make sure that no usage sites are missed,
> as "struct drm_edid" has some restrictions, for example it can not be
> used with kfree().
>
> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 70 ++++++++++++----------
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 5 +-
> .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 26 ++++----
> 3 files changed, 53 insertions(+), 48 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 bd9a1a21720e..961f4f308dc7 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3426,7 +3426,7 @@ void amdgpu_dm_update_connector_after_detect(
> aconnector->dc_sink = sink;
> dc_sink_retain(aconnector->dc_sink);
> amdgpu_dm_update_freesync_caps(connector,
> - aconnector->edid);
> + aconnector->drm_edid);
> } else {
> amdgpu_dm_update_freesync_caps(connector, NULL);
> if (!aconnector->dc_sink) {
> @@ -3485,18 +3485,18 @@ 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;
> + aconnector->drm_edid = NULL;
> 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;
> + aconnector->drm_edid = drm_edid_alloc(sink->dc_edid.raw_edid,
> + sink->dc_edid.length);
>
> if (aconnector->dc_link->aux_mode)
> drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> - aconnector->edid);
> + drm_edid_raw(aconnector->drm_edid));
> }
>
> if (!aconnector->timing_requested) {
> @@ -3507,8 +3507,8 @@ void amdgpu_dm_update_connector_after_detect(
> "failed to create aconnector->requested_timing\n");
> }
>
> - drm_connector_update_edid_property(connector, aconnector->edid);
> - amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> + drm_edid_connector_update(connector, aconnector->drm_edid);
> + amdgpu_dm_update_freesync_caps(connector, aconnector->drm_edid);
> update_connector_ext_caps(aconnector);
> } else {
> drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
> @@ -3517,7 +3517,7 @@ void amdgpu_dm_update_connector_after_detect(
> aconnector->num_modes = 0;
> dc_sink_release(aconnector->dc_sink);
> aconnector->dc_sink = NULL;
> - aconnector->edid = NULL;
> + aconnector->drm_edid = NULL;
> 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 */
> @@ -7016,7 +7016,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;
> + const struct drm_edid *drm_edid;
> struct i2c_adapter *ddc;
>
> if (dc_link && dc_link->aux_mode)
> @@ -7025,23 +7025,25 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
> ddc = &aconnector->i2c->base;
>
> /*
> - * Note: drm_get_edid gets edid in the following order:
> + * Note: drm_edid_read_ddc 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, ddc);
> - if (!edid) {
> + drm_edid = drm_edid_read_ddc(connector, ddc);
> + if (!drm_edid) {
> DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
> return;
> }
>
> - aconnector->edid = edid;
> + aconnector->drm_edid = drm_edid;
>
> /* Update emulated (virtual) sink's EDID */
> if (dc_em_sink && dc_link) {
> 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,
> + drm_edid_raw(drm_edid),
> + (drm_edid_raw(drm_edid)->extensions + 1) * EDID_LENGTH);
> dm_helpers_parse_edid_caps(
> dc_link,
> &dc_em_sink->dc_edid,
> @@ -7076,7 +7078,7 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> .link = aconnector->dc_link,
> .sink_signal = SIGNAL_TYPE_VIRTUAL
> };
> - struct edid *edid;
> + const struct drm_edid *drm_edid;
> struct i2c_adapter *ddc;
>
> if (dc_link->aux_mode)
> @@ -7085,25 +7087,25 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> ddc = &aconnector->i2c->base;
>
> /*
> - * Note: drm_get_edid gets edid in the following order:
> + * Note: drm_edid_read_ddc 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, ddc);
> - if (!edid) {
> + drm_edid = drm_edid_read_ddc(connector, ddc);
> + 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;

You need to drm_edid_connector_update() to update
connector->display_info with the EDID info.

BR,
Jani.

>
> - aconnector->edid = edid;
> + aconnector->drm_edid = drm_edid;
>
> aconnector->dc_em_sink = dc_link_add_remote_sink(
> aconnector->dc_link,
> - edid,
> + drm_edid_raw(drm_edid),
> &init_params);
>
> if (aconnector->base.force == DRM_FORCE_ON) {
> @@ -7786,16 +7788,17 @@ 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);
> + drm_edid_connector_update(connector, drm_edid);
> 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
> @@ -7809,10 +7812,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_connector_edid_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;
> }
> @@ -7908,12 +7911,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)
> @@ -7926,24 +7929,24 @@ 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->drm_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);
> if (encoder)
> 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);
>
> @@ -11953,12 +11956,13 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> * FreeSync parameters.
> */
> void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> - const struct edid *edid)
> + 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;
> + const struct edid *edid = drm_edid_raw(drm_edid);
> struct amdgpu_dm_connector *amdgpu_dm_connector =
> to_amdgpu_dm_connector(connector);
> struct dm_connector_state *dm_con_state = NULL;
> 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 27c0017707dd..c49cc0170fc5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -81,6 +81,7 @@ struct amdgpu_bo;
> struct dmub_srv;
> struct dc_plane_state;
> struct dmub_notification;
> +struct drm_edid;
>
> struct amd_vsdb_block {
> unsigned char ieee_id[3];
> @@ -673,7 +674,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 *drm_edid;
>
> /* shared with amdgpu */
> struct amdgpu_hpd hpd;
> @@ -951,7 +952,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,
> - const 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_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 25e98d248c21..cd03108db28b 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
> @@ -129,7 +129,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
> dc_sink_release(aconnector->dc_sink);
> }
>
> - kfree(aconnector->edid);
> + kfree(aconnector->drm_edid);
>
> drm_connector_cleanup(connector);
> drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> @@ -182,7 +182,7 @@ amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector)
>
> dc_sink_release(dc_sink);
> aconnector->dc_sink = NULL;
> - aconnector->edid = NULL;
> + aconnector->drm_edid = NULL;
> aconnector->dsc_aux = NULL;
> port->passthrough_aux = NULL;
> }
> @@ -302,12 +302,13 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> if (!aconnector)
> return drm_add_edid_modes(connector, NULL);
>
> - if (!aconnector->edid) {
> - struct edid *edid;
> + if (!aconnector->drm_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);
>
> @@ -344,7 +345,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> return ret;
> }
>
> - aconnector->edid = edid;
> + aconnector->drm_edid = drm_edid;
> amdgpu_dm_set_mst_status(&aconnector->mst_status,
> MST_REMOTE_EDID, true);
> }
> @@ -361,7 +362,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
> dc_sink = dc_link_add_remote_sink(
> aconnector->dc_link,
> - aconnector->edid,
> + drm_edid_raw(aconnector->drm_edid),
> &init_params);
>
> if (!dc_sink) {
> @@ -403,7 +404,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>
> if (aconnector->dc_sink) {
> amdgpu_dm_update_freesync_caps(
> - connector, aconnector->edid);
> + connector, aconnector->drm_edid);
>
> #if defined(CONFIG_DRM_AMD_DC_FP)
> if (!validate_dsc_caps_on_connector(aconnector))
> @@ -417,10 +418,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->drm_edid);
>
> - ret = drm_add_edid_modes(connector, aconnector->edid);
> + ret = drm_edid_connector_add_modes(connector);
>
> return ret;
> }
> @@ -498,7 +498,7 @@ dm_dp_mst_detect(struct drm_connector *connector,
>
> dc_sink_release(aconnector->dc_sink);
> aconnector->dc_sink = NULL;
> - aconnector->edid = NULL;
> + aconnector->drm_edid = NULL;
> aconnector->dsc_aux = NULL;
> port->passthrough_aux = NULL;

--
Jani Nikula, Intel

Next message: 胡连勤: " 答复: 答复: [PATCH v1] usb: gadget: u_serial: check Null pointer in EP callback"
Previous message: Bartosz Golaszewski: "[PATCH v2 2/2] PCI/pwrctl: put the bus rescan on a different thread"
In reply to: Thomas Weißschuh: "[PATCH 09/12] drm/amd/display: Switch amdgpu_dm_connector to struct drm_edid"
Next in thread: Thomas Weißschuh: "[PATCH 04/12] drm/amd/display: Simplify raw_edid handling in dm_helpers_parse_edid_caps()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]