Re: [PATCH 02/15] drm/amdgpu: use drm_* functions instead of duplicated code in amdgpu driver

On 2021-10-15 07:37, Claudio Suarez wrote:

a) Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. The amdgpu driver still calls drm_detect_hdmi_monitor() to retrieve the same information, which is less efficient. Change to drm_display_info.is_hdmi

This is a TODO task in Documentation/gpu/todo.rst

b) drm_display_info is updated by drm_get_edid() or drm_connector_update_edid_property(). In the amdgpu driver it is almost always updated when the edid is read in amdgpu_connector_get_edid(), but not always. Change amdgpu_connector_get_edid() and amdgpu_connector_free_edid() to keep drm_display_info updated. This allows a) to work properly.

c) Use drm_edid_get_monitor_name() instead of duplicating the code that parses the EDID in dm_helpers_parse_edid_caps()

Also, remove the unused "struct dc_context *ctx" parameter in dm_helpers_parse_edid_caps()

Thanks for this work.

The fact that you listed three separate changes in this commit is a clear indication that this patch should be three separate patches instead. Separating the functional bits from the straight refactor will help with bisection if this leads to a regression.

All changes look reasonable to me, though. With this patch split into three patches in the sequence (b), (c), then (a) this is Reviewed-by: Harry Wentland harry.wentland@amd.com

Harry

Signed-off-by: Claudio Suarez cssk@net-c.es

.../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 23 +++++++---- .../gpu/drm/amd/amdgpu/amdgpu_connectors.h | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 4 +- .../gpu/drm/amd/amdgpu/atombios_encoders.c | 6 +-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 39 ++++++------------- drivers/gpu/drm/amd/display/dc/core/dc.c | 2 +- drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +- 9 files changed, 39 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index b9c11c2b2885..7b41a1120b70 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -25,6 +25,7 @@ */

#include <drm/drm_edid.h> +#include <drm/drm_connector.h> #include <drm/drm_fb_helper.h> #include <drm/drm_dp_helper.h> #include <drm/drm_probe_helper.h> @@ -108,7 +109,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector) case DRM_MODE_CONNECTOR_DVII: case DRM_MODE_CONNECTOR_HDMIB: if (amdgpu_connector->use_digital) {

• 	if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

• 	if (amdgpu_connector_is_hdmi_monitor(connector)) {
  	if (connector->display_info.bpc)
  		bpc = connector->display_info.bpc;
  }
  

@@ -116,7 +117,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector) break; case DRM_MODE_CONNECTOR_DVID: case DRM_MODE_CONNECTOR_HDMIA:

• if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

• if (amdgpu_connector_is_hdmi_monitor(connector)) {
  if (connector->display_info.bpc)
  	bpc = connector->display_info.bpc;
  

  }

@@ -125,7 +126,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector) dig_connector = amdgpu_connector->con_priv; if ((dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT) || (dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) ||

•     drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

•     (amdgpu_connector_is_hdmi_monitor(connector))) {
  if (connector->display_info.bpc)
  	bpc = connector->display_info.bpc;
  

  }

@@ -149,7 +150,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector) break; }

• if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {

• if (amdgpu_connector_is_hdmi_monitor(connector)) { /*
  • Pre DCE-8 hw can't handle > 12 bpc, and more than 12 bpc doesn't make
  • much sense without support for > 12 bpc framebuffers. RGB 4:4:4 at

@@ -315,8 +316,10 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector) if (!amdgpu_connector->edid) { /* some laptops provide a hardcoded edid in rom for LCDs */ if (((connector->connector_type == DRM_MODE_CONNECTOR_LVDS) ||

•      (connector->connector_type == DRM_MODE_CONNECTOR_eDP)))
  

•      (connector->connector_type == DRM_MODE_CONNECTOR_eDP))) {
  amdgpu_connector->edid = amdgpu_connector_get_hardcoded_edid(adev);
  

• 	drm_connector_update_edid_property(connector, amdgpu_connector->edid);
  

• }
  

  }

}

@@ -326,6 +329,7 @@ static void amdgpu_connector_free_edid(struct drm_connector *connector)

kfree(amdgpu_connector->edid); amdgpu_connector->edid = NULL;

• drm_connector_update_edid_property(connector, NULL);

}

static int amdgpu_connector_ddc_get_modes(struct drm_connector *connector) @@ -1170,7 +1174,7 @@ static enum drm_mode_status amdgpu_connector_dvi_mode_valid(struct drm_connector (amdgpu_connector->connector_object_id == CONNECTOR_OBJECT_ID_DUAL_LINK_DVI_D) || (amdgpu_connector->connector_object_id == CONNECTOR_OBJECT_ID_HDMI_TYPE_B)) { return MODE_OK;

• } else if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

• } else if (amdgpu_connector_is_hdmi_monitor(connector)) {
  /* HDMI 1.3+ supports max clock of 340 Mhz */
  if (mode->clock > 340000)
  	return MODE_CLOCK_HIGH;
  

@@ -1322,6 +1326,11 @@ bool amdgpu_connector_is_dp12_capable(struct drm_connector *connector) return false; }

+bool amdgpu_connector_is_hdmi_monitor(struct drm_connector *connector) +{

• return connector->display_info.is_hdmi;

+}

static enum drm_connector_status amdgpu_connector_dp_detect(struct drm_connector *connector, bool force) { @@ -1462,7 +1471,7 @@ static enum drm_mode_status amdgpu_connector_dp_mode_valid(struct drm_connector (amdgpu_dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_eDP)) { return amdgpu_atombios_dp_mode_valid_helper(connector, mode); } else {

• 	if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

• 	if (amdgpu_connector_is_hdmi_monitor(connector)) {
  	/* HDMI 1.3+ supports max clock of 340 Mhz */
  	if (mode->clock > 340000)
  		return MODE_CLOCK_HIGH;
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h index 61fcef15ad72..0843540e01f2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h @@ -29,6 +29,8 @@ void amdgpu_connector_hotplug(struct drm_connector *connector); int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector); u16 amdgpu_connector_encoder_get_dp_bridge_encoder_id(struct drm_connector *connector); bool amdgpu_connector_is_dp12_capable(struct drm_connector *connector); +bool amdgpu_connector_is_hdmi_monitor(struct drm_connector *connector);

void amdgpu_connector_add(struct amdgpu_device *adev, uint32_t connector_id, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index dc50c05f23fc..41b43207e9fa 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -1364,7 +1364,7 @@ bool amdgpu_display_crtc_scaling_mode_fixup(struct drm_crtc *crtc, if ((!(mode->flags & DRM_MODE_FLAG_INTERLACE)) && ((amdgpu_encoder->underscan_type == UNDERSCAN_ON) || ((amdgpu_encoder->underscan_type == UNDERSCAN_AUTO) &&

•       drm_detect_hdmi_monitor(amdgpu_connector_edid(connector)) &&
  

•       amdgpu_connector_is_hdmi_monitor(connector) &&
   amdgpu_display_is_hdtv_mode(mode)))) {
  if (amdgpu_encoder->underscan_hborder != 0)
  	amdgpu_crtc->h_border = amdgpu_encoder->underscan_hborder;
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c index af4ef84e27a7..34799786bb40 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c @@ -222,7 +222,7 @@ bool amdgpu_dig_monitor_is_duallink(struct drm_encoder *encoder, case DRM_MODE_CONNECTOR_HDMIB: if (amdgpu_connector->use_digital) { /* HDMI 1.3 supports up to 340 Mhz over single link */

• 	if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

• 	if (amdgpu_connector_is_hdmi_monitor(connector)) {
  	if (pixel_clock > 340000)
  		return true;
  	else
  

@@ -244,7 +244,7 @@ bool amdgpu_dig_monitor_is_duallink(struct drm_encoder *encoder, return false; else { /* HDMI 1.3 supports up to 340 Mhz over single link */

• 	if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
  

• 	if (amdgpu_connector_is_hdmi_monitor(connector)) {
  	if (pixel_clock > 340000)
  		return true;
  	else
  

diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c index 6134ed964027..07c4ff14f2a7 100644 --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c @@ -469,7 +469,7 @@ int amdgpu_atombios_encoder_get_encoder_mode(struct drm_encoder *encoder) if (amdgpu_connector->use_digital && (amdgpu_connector->audio == AMDGPU_AUDIO_ENABLE)) return ATOM_ENCODER_MODE_HDMI;

• 	else if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector)) &&
  

• 	else if (amdgpu_connector_is_hdmi_monitor(connector) &&
  	 (amdgpu_connector->audio == AMDGPU_AUDIO_AUTO))
  	return ATOM_ENCODER_MODE_HDMI;
  else if (amdgpu_connector->use_digital)
  

@@ -488,7 +488,7 @@ int amdgpu_atombios_encoder_get_encoder_mode(struct drm_encoder *encoder) if (amdgpu_audio != 0) { if (amdgpu_connector->audio == AMDGPU_AUDIO_ENABLE) return ATOM_ENCODER_MODE_HDMI;

• 	else if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector)) &&
  

• 	else if (amdgpu_connector_is_hdmi_monitor(connector) &&
  	 (amdgpu_connector->audio == AMDGPU_AUDIO_AUTO))
  	return ATOM_ENCODER_MODE_HDMI;
  else
  

@@ -506,7 +506,7 @@ int amdgpu_atombios_encoder_get_encoder_mode(struct drm_encoder *encoder) } else if (amdgpu_audio != 0) { if (amdgpu_connector->audio == AMDGPU_AUDIO_ENABLE) return ATOM_ENCODER_MODE_HDMI;

• 	else if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector)) &&
  

• 	else if (amdgpu_connector_is_hdmi_monitor(connector) &&
  	 (amdgpu_connector->audio == AMDGPU_AUDIO_AUTO))
  	return ATOM_ENCODER_MODE_HDMI;
  else
  

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 1ea31dcc7a8b..02ecd216a556 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2583,13 +2583,12 @@ void amdgpu_dm_update_connector_after_detect( aconnector->edid = (struct edid *)sink->dc_edid.raw_edid;

• 	drm_connector_update_edid_property(connector,
  

• 					   aconnector->edid);
  if (aconnector->dc_link->aux_mode)
  	drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
  			    aconnector->edid);
  

  }

• drm_connector_update_edid_property(connector, aconnector->edid);
  

  amdgpu_dm_update_freesync_caps(connector, aconnector->edid); update_connector_ext_caps(aconnector); } else {

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 6fee12c91ef5..2051dd27ef3b 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 @@ -29,6 +29,7 @@

#include <drm/drm_probe_helper.h> #include <drm/amdgpu_drm.h> +#include <drm/drm_connector.h> #include <drm/drm_edid.h>

#include "dm_services.h" @@ -37,6 +38,7 @@ #include "amdgpu_dm.h" #include "amdgpu_dm_irq.h" #include "amdgpu_dm_mst_types.h" +#include "amdgpu_connectors.h"

#include "dm_helpers.h"

@@ -50,16 +52,17 @@

• void
• */

enum dc_edid_status dm_helpers_parse_edid_caps(

• struct dc_context *ctx,
  

• struct dc_link *link,
  

  const struct dc_edid *edid, struct dc_edid_caps *edid_caps)

{

• struct amdgpu_dm_connector *aconnector = link->priv;
• struct drm_connector *connector = &aconnector->base; struct edid *edid_buf = (struct edid *) edid->raw_edid; struct cea_sad *sads; int sad_count = -1; int sadb_count = -1; int i = 0;

• int j = 0; uint8_t *sadb = NULL;

  enum dc_edid_status result = EDID_OK;

@@ -78,23 +81,11 @@ enum dc_edid_status dm_helpers_parse_edid_caps( edid_caps->manufacture_week = edid_buf->mfg_week; edid_caps->manufacture_year = edid_buf->mfg_year;

• /* One of the four detailed_timings stores the monitor name. It's

• * stored in an array of length 13. */
  

• for (i = 0; i < 4; i++) {

• if (edid_buf->detailed_timings[i].data.other_data.type == 0xfc) {
  

• 	while (j < 13 && edid_buf->detailed_timings[i].data.other_data.data.str.str[j]) {
  

• 		if (edid_buf->detailed_timings[i].data.other_data.data.str.str[j] == '\n')
  

• 			break;
  

• 		edid_caps->display_name[j] =
  

• 			edid_buf->detailed_timings[i].data.other_data.data.str.str[j];
  

• 		j++;
  

• 	}
  

• }
  

• }

• drm_edid_get_monitor_name(edid_buf,

• 		  edid_caps->display_name,
  

• 		  AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS);
  
  

• edid_caps->edid_hdmi = drm_detect_hdmi_monitor(

• 	(struct edid *) edid->raw_edid);
  

• edid_caps->edid_hdmi = amdgpu_connector_is_hdmi_monitor(connector);

  sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads); if (sad_count <= 0)

@@ -610,14 +601,8 @@ enum dc_edid_status dm_helpers_read_local_edid( /* We don't need the original edid anymore */ kfree(edid);

• /* connector->display_info will be parsed from EDID and saved
  

•  * into drm_connector->display_info from edid by call stack
  

•  * below:
  

•  * drm_parse_ycbcr420_deep_color_info
  

•  * drm_parse_hdmi_forum_vsdb
  

•  * drm_parse_cea_ext
  

•  * drm_add_display_info
  

•  * drm_connector_update_edid_property
  

• /* connector->display_info is parsed from EDID and saved
  

•  * into drm_connector->display_info
  

  • drm_connector->display_info will be used by amdgpu_dm funcs,
  • like fill_stream_properties_from_drm_display_mode

@@ -625,7 +610,7 @@ enum dc_edid_status dm_helpers_read_local_edid( amdgpu_dm_update_connector_after_detect(aconnector);

edid_status = dm_helpers_parse_edid_caps(

• 				ctx,
  

• 				link,
  			&sink->dc_edid,
  			&sink->edid_caps);
  
  

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index c798c65d4276..5efe89fe6c2c 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -3254,7 +3254,7 @@ struct dc_sink *dc_link_add_remote_sink( goto fail_add_sink;

edid_status = dm_helpers_parse_edid_caps(

• 	link->ctx,
  

• 	link,
  &dc_sink->dc_edid,
  &dc_sink->edid_caps);
  
  

diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h b/drivers/gpu/drm/amd/display/dc/dm_helpers.h index 9ab854293ace..94dc80060610 100644 --- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h +++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h @@ -59,7 +59,7 @@ void dm_helpers_free_gpu_mem( void *pvMem);

enum dc_edid_status dm_helpers_parse_edid_caps(

• struct dc_context *ctx,

• struct dc_link *link, const struct dc_edid *edid, struct dc_edid_caps *edid_caps);