Copy&paste from the TODO document Documentation/gpu/todo.rst
=== Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi ---------------------------------------------------------------
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Many drivers still call drm_detect_hdmi_monitor() to retrieve the same information, which is less efficient.
Audit each individual driver calling drm_detect_hdmi_monitor() and switch to drm_display_info.is_hdmi if applicable. =====
I did it in two steps: - check that drm_display_info has a correct value. - in that case, replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
Almost all occurrences of drm_detect_hdmi_monitor() could be changed. Some small inconsistencies have been solved.
Stats: drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 23 ++++++++++++++++------- drivers/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 ++-- drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 6 +++--- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +-- drivers/gpu/drm/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 +- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/drm_edid.c | 2 ++ drivers/gpu/drm/exynos/exynos_hdmi.c | 6 ++++-- drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 3 ++- drivers/gpu/drm/gma500/psb_intel_sdvo.c | 6 ++++-- drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 4 ++-- drivers/gpu/drm/nouveau/dispnv50/head.c | 8 +------- drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +- drivers/gpu/drm/nouveau/nouveau_connector.h | 6 ++++++ drivers/gpu/drm/radeon/atombios_encoders.c | 6 +++--- drivers/gpu/drm/radeon/radeon_connectors.c | 20 ++++++++++++++------ drivers/gpu/drm/radeon/radeon_display.c | 2 +- drivers/gpu/drm/radeon/radeon_encoders.c | 4 ++-- drivers/gpu/drm/radeon/radeon_mode.h | 1 + drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++-- drivers/gpu/drm/rockchip/rk3066_hdmi.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 10 ++++++---- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 ++-- drivers/gpu/drm/tegra/hdmi.c | 6 +----- drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++--- drivers/gpu/drm/zte/zx_hdmi.c | 4 ++-- 37 files changed, 112 insertions(+), 96 deletions(-)
Best regards. Claudio Suarez
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector); + drm_reset_display_info(connector); return 0; } if (!drm_edid_is_valid(edid)) { clear_eld(connector); + drm_reset_display_info(connector); drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0;
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector);
drm_reset_display_info(connector);drm_reset_display_info(connector);
Looks easier if you pull both of those out from these branches and just call them unconditionally at the start.
drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0;-- 2.33.0
On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector);
drm_reset_display_info(connector);drm_reset_display_info(connector);Looks easier if you pull both of those out from these branches and just call them unconditionally at the start.
After looking at the full code, I am not sure. This is the code: ================== int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) { int num_modes = 0; u32 quirks;
if (edid == NULL) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me return 0; } if (!drm_edid_is_valid(edid)) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; }
drm_edid_to_eld(connector, edid);
quirks = drm_add_display_info(connector, edid); etc... =================
If we move those out of these branches and edid != NULL, we are executing an unnecessary clear_eld(connector) and an unnecessary drm_reset_display_info(connector) because the fields will be set in the next drm_edid_to_eld(connector, edid) and drm_add_display_info(connector, edid)
Do we want this ?
BR Claudio Suarez
On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector);
drm_reset_display_info(connector);drm_reset_display_info(connector);Looks easier if you pull both of those out from these branches and just call them unconditionally at the start.
After looking at the full code, I am not sure. This is the code:
int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) { int num_modes = 0; u32 quirks;
if (edid == NULL) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me return 0; } if (!drm_edid_is_valid(edid)) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; } drm_edid_to_eld(connector, edid); quirks = drm_add_display_info(connector, edid);etc...
If we move those out of these branches and edid != NULL, we are executing an unnecessary clear_eld(connector) and an unnecessary drm_reset_display_info(connector) because the fields will be set in the next drm_edid_to_eld(connector, edid) and drm_add_display_info(connector, edid)
Do we want this ?
Seems fine by me. And maybe we could nuke the second drm_reset_display_info() from deeper inside drm_add_display_info()? Not sure if drm_add_display_info() still has to be able to operate standalone or not.
Hmm. Another option is to just move all these NULL/invalid edid checks into drm_edid_to_eld() and drm_add_display_info().
On Fri, Oct 15, 2021 at 10:33:29PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector);
drm_reset_display_info(connector);drm_reset_display_info(connector);Looks easier if you pull both of those out from these branches and just call them unconditionally at the start.
After looking at the full code, I am not sure. This is the code:
int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) { int num_modes = 0; u32 quirks;
if (edid == NULL) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me return 0; } if (!drm_edid_is_valid(edid)) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; } drm_edid_to_eld(connector, edid); quirks = drm_add_display_info(connector, edid);etc...
If we move those out of these branches and edid != NULL, we are executing an unnecessary clear_eld(connector) and an unnecessary drm_reset_display_info(connector) because the fields will be set in the next drm_edid_to_eld(connector, edid) and drm_add_display_info(connector, edid)
Do we want this ?
Seems fine by me. And maybe we could nuke the second drm_reset_display_info() from deeper inside drm_add_display_info()? Not sure if drm_add_display_info() still has to be able to operate standalone or not.
Hmm. Another option is to just move all these NULL/invalid edid checks into drm_edid_to_eld() and drm_add_display_info().
But maybe that's not so easy. Would still need to bail out from drm_add_edid_modes() I guess.
On Fri, Oct 15, 2021 at 10:33:29PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector);
drm_reset_display_info(connector);drm_reset_display_info(connector);Looks easier if you pull both of those out from these branches and just call them unconditionally at the start.
After looking at the full code, I am not sure. This is the code:
int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) { int num_modes = 0; u32 quirks;
if (edid == NULL) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me return 0; } if (!drm_edid_is_valid(edid)) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; } drm_edid_to_eld(connector, edid); quirks = drm_add_display_info(connector, edid);etc...
If we move those out of these branches and edid != NULL, we are executing an unnecessary clear_eld(connector) and an unnecessary drm_reset_display_info(connector) because the fields will be set in the next drm_edid_to_eld(connector, edid) and drm_add_display_info(connector, edid)
Do we want this ?
Seems fine by me. And maybe we could nuke the second drm_reset_display_info() from deeper inside drm_add_display_info()? Not sure if drm_add_display_info() still has to be able to operate standalone or not.
Hmm. Another option is to just move all these NULL/invalid edid checks into drm_edid_to_eld() and drm_add_display_info().
I was thinking about this. We can use a boolean variable: =============== bool edid_is_invalid;
edid_is_invalid = !drm_edid_is_valid(edid);
if (edid == NULL || edid_is_invalid) { clear_eld(connector); drm_reset_display_info(connector); if (edid_is_invalid) drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; }
drm_edid_to_eld(connector, edid); ... =============== Internally, drm_edid_is_valid() handles NULL pointers properly. It is a quite elegant solution with a small change in the original design, and it improves this part in the way you pointed out.
Best regards, Claudio Suarez
On Sat, Oct 16, 2021 at 10:25:03AM +0200, Claudio Suarez wrote:
On Fri, Oct 15, 2021 at 10:33:29PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
According to the documentation, drm_add_edid_modes "... Also fills out the &drm_display_info structure and ELD in @connector with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a value or may be null. When it is not null, connector->display_info and connector->eld are updated according to the edid. When edid=NULL, only connector->eld is reset. Reset connector->display_info to be consistent and accurate.
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/drm_edid.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..6cbe09b2357c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
if (edid == NULL) { clear_eld(connector);
drm_reset_display_info(connector);drm_reset_display_info(connector);Looks easier if you pull both of those out from these branches and just call them unconditionally at the start.
After looking at the full code, I am not sure. This is the code:
int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) { int num_modes = 0; u32 quirks;
if (edid == NULL) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me return 0; } if (!drm_edid_is_valid(edid)) { clear_eld(connector); drm_reset_display_info(connector); <--- added by me drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; } drm_edid_to_eld(connector, edid); quirks = drm_add_display_info(connector, edid);etc...
If we move those out of these branches and edid != NULL, we are executing an unnecessary clear_eld(connector) and an unnecessary drm_reset_display_info(connector) because the fields will be set in the next drm_edid_to_eld(connector, edid) and drm_add_display_info(connector, edid)
Do we want this ?
Seems fine by me. And maybe we could nuke the second drm_reset_display_info() from deeper inside drm_add_display_info()? Not sure if drm_add_display_info() still has to be able to operate standalone or not.
Hmm. Another option is to just move all these NULL/invalid edid checks into drm_edid_to_eld() and drm_add_display_info().
I was thinking about this. We can use a boolean variable:
bool edid_is_invalid; edid_is_invalid = !drm_edid_is_valid(edid); if (edid == NULL || edid_is_invalid) { clear_eld(connector); drm_reset_display_info(connector); if (edid_is_invalid) drm_warn(connector->dev, "%s: EDID invalid.\n", connector->name); return 0; } drm_edid_to_eld(connector, edid);...
Internally, drm_edid_is_valid() handles NULL pointers properly. It is a quite elegant solution with a small change in the original design, and it improves this part in the way you pointed out.
I'll send a patch with this idea and we can talk about the new code. Thanks!
Best regards, Claudio Suarez.
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()
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);
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; elsediff --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; elsediff --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);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,{
- 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);
On Fri, Oct 15, 2021 at 11:14:54AM -0400, Harry Wentland wrote:
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
Ok, thanks. I'll send three patches.
BR Claudio Suarez
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Use this value instead of calling drm_detect_hdmi_monitor() to avoid a second parse.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b4b4653fe301..d531e4c501eb 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -182,7 +182,8 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
if (edid) { cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); - vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); + vc4_hdmi->encoder.hdmi_monitor = + connector->display_info.is_hdmi; kfree(edid); } } @@ -212,10 +213,9 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) if (!edid) return -ENODEV;
- vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid); - drm_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); + vc4_encoder->hdmi_monitor = connector->display_info.is_hdmi; kfree(edid);
if (vc4_hdmi->disable_4kp60) {
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information is less efficient. Change to drm_display_info.is_hdmi
This is a TODO task in Documentation/gpu/todo.rst
Also, correct an inacurracy or bug in radeon_connector_get_edid()/radeon_connector_free_edid(). Two variables have EDID data: - struct radeon_connector.edid - struct drm_connector.edid_blob_ptr The second is updated by calling drm_connector_update_edid_property() or drm_get_edid() - which internally calls drm_connector_update_edid_property(). drm_display_info.is_hdmi is updated when this function is called. radeon_connector_get_edid() calls drm_get_edid() to update drm_connector.edid_blob_ptr/drm_display_info only in some cases. Change it to be always up to date, so drm_display_info is always correct. As counterpart, reset these values in radeon_connector_free_edid().
This second change is necessary for the previous one to work properly.
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/radeon/atombios_encoders.c | 6 +++--- drivers/gpu/drm/radeon/radeon_connectors.c | 20 ++++++++++++++------ drivers/gpu/drm/radeon/radeon_display.c | 2 +- drivers/gpu/drm/radeon/radeon_encoders.c | 4 ++-- drivers/gpu/drm/radeon/radeon_mode.h | 1 + 5 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c index 0fce73b9a646..29a140732f71 100644 --- a/drivers/gpu/drm/radeon/atombios_encoders.c +++ b/drivers/gpu/drm/radeon/atombios_encoders.c @@ -713,7 +713,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder) if (radeon_connector->use_digital && (radeon_connector->audio == RADEON_AUDIO_ENABLE)) return ATOM_ENCODER_MODE_HDMI; - else if (drm_detect_hdmi_monitor(radeon_connector_edid(connector)) && + else if (radeon_connector_is_hdmi_monitor(connector) && (radeon_connector->audio == RADEON_AUDIO_AUTO)) return ATOM_ENCODER_MODE_HDMI; else if (radeon_connector->use_digital) @@ -732,7 +732,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder) if (radeon_audio != 0) { if (radeon_connector->audio == RADEON_AUDIO_ENABLE) return ATOM_ENCODER_MODE_HDMI; - else if (drm_detect_hdmi_monitor(radeon_connector_edid(connector)) && + else if (radeon_connector_is_hdmi_monitor(connector) && (radeon_connector->audio == RADEON_AUDIO_AUTO)) return ATOM_ENCODER_MODE_HDMI; else @@ -756,7 +756,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder) } else if (radeon_audio != 0) { if (radeon_connector->audio == RADEON_AUDIO_ENABLE) return ATOM_ENCODER_MODE_HDMI; - else if (drm_detect_hdmi_monitor(radeon_connector_edid(connector)) && + else if (radeon_connector_is_hdmi_monitor(connector) && (radeon_connector->audio == RADEON_AUDIO_AUTO)) return ATOM_ENCODER_MODE_HDMI; else diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 607ad5620bd9..0200f094467c 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -130,7 +130,7 @@ int radeon_get_monitor_bpc(struct drm_connector *connector) case DRM_MODE_CONNECTOR_DVII: case DRM_MODE_CONNECTOR_HDMIB: if (radeon_connector->use_digital) { - if (drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + if (radeon_connector_is_hdmi_monitor(connector)) { if (connector->display_info.bpc) bpc = connector->display_info.bpc; } @@ -138,7 +138,7 @@ int radeon_get_monitor_bpc(struct drm_connector *connector) break; case DRM_MODE_CONNECTOR_DVID: case DRM_MODE_CONNECTOR_HDMIA: - if (drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + if (radeon_connector_is_hdmi_monitor(connector)) { if (connector->display_info.bpc) bpc = connector->display_info.bpc; } @@ -147,7 +147,7 @@ int radeon_get_monitor_bpc(struct drm_connector *connector) dig_connector = radeon_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(radeon_connector_edid(connector))) { + radeon_connector_is_hdmi_monitor(connector)) { if (connector->display_info.bpc) bpc = connector->display_info.bpc; } @@ -171,7 +171,7 @@ int radeon_get_monitor_bpc(struct drm_connector *connector) break; }
- if (drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + if (radeon_connector_is_hdmi_monitor(connector)) { /* hdmi deep color only implemented on DCE4+ */ if ((bpc > 8) && !ASIC_IS_DCE4(rdev)) { DRM_DEBUG("%s: HDMI deep color %d bpc unsupported. Using 8 bpc.\n", @@ -348,6 +348,8 @@ static void radeon_connector_get_edid(struct drm_connector *connector) /* some servers provide a hardcoded edid in rom for KVMs */ radeon_connector->edid = radeon_bios_get_hardcoded_edid(rdev); } + if (radeon_connector->edid) + drm_connector_update_edid_property(connector, radeon_connector->edid); } }
@@ -358,6 +360,7 @@ static void radeon_connector_free_edid(struct drm_connector *connector) if (radeon_connector->edid) { kfree(radeon_connector->edid); radeon_connector->edid = NULL; + drm_connector_update_edid_property(connector, NULL); } }
@@ -1496,7 +1499,7 @@ static enum drm_mode_status radeon_dvi_mode_valid(struct drm_connector *connecto (radeon_connector->connector_object_id == CONNECTOR_OBJECT_ID_DUAL_LINK_DVI_D) || (radeon_connector->connector_object_id == CONNECTOR_OBJECT_ID_HDMI_TYPE_B)) return MODE_OK; - else if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + else if (ASIC_IS_DCE6(rdev) && radeon_connector_is_hdmi_monitor(connector)) { /* HDMI 1.3+ supports max clock of 340 Mhz */ if (mode->clock > 340000) return MODE_CLOCK_HIGH; @@ -1649,6 +1652,11 @@ bool radeon_connector_is_dp12_capable(struct drm_connector *connector) return false; }
+bool radeon_connector_is_hdmi_monitor(struct drm_connector *connector) +{ + return connector->display_info.is_hdmi; +} + static enum drm_connector_status radeon_dp_detect(struct drm_connector *connector, bool force) { @@ -1804,7 +1812,7 @@ static enum drm_mode_status radeon_dp_mode_valid(struct drm_connector *connector (radeon_dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_eDP)) { return radeon_dp_mode_valid_helper(connector, mode); } else { - if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + if (ASIC_IS_DCE6(rdev) && radeon_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/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 573154268d43..0d1f79156ab2 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1720,7 +1720,7 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc, (!(mode->flags & DRM_MODE_FLAG_INTERLACE)) && ((radeon_encoder->underscan_type == UNDERSCAN_ON) || ((radeon_encoder->underscan_type == UNDERSCAN_AUTO) && - drm_detect_hdmi_monitor(radeon_connector_edid(connector)) && + radeon_connector_is_hdmi_monitor(connector) && is_hdtv_mode(mode)))) { if (radeon_encoder->underscan_hborder != 0) radeon_crtc->h_border = radeon_encoder->underscan_hborder; diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c b/drivers/gpu/drm/radeon/radeon_encoders.c index 46549d5179ee..8f65fa744f48 100644 --- a/drivers/gpu/drm/radeon/radeon_encoders.c +++ b/drivers/gpu/drm/radeon/radeon_encoders.c @@ -383,7 +383,7 @@ bool radeon_dig_monitor_is_duallink(struct drm_encoder *encoder, case DRM_MODE_CONNECTOR_HDMIB: if (radeon_connector->use_digital) { /* HDMI 1.3 supports up to 340 Mhz over single link */ - if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + if (ASIC_IS_DCE6(rdev) && radeon_connector_is_hdmi_monitor(connector)) { if (pixel_clock > 340000) return true; else @@ -408,7 +408,7 @@ bool radeon_dig_monitor_is_duallink(struct drm_encoder *encoder, return false; else { /* HDMI 1.3 supports up to 340 Mhz over single link */ - if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) { + if (ASIC_IS_DCE6(rdev) && radeon_connector_is_hdmi_monitor(connector)) { if (pixel_clock > 340000) return true; else diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index fe16f140a6b4..e591179d5e5d 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -739,6 +739,7 @@ extern bool radeon_dig_monitor_is_duallink(struct drm_encoder *encoder, extern u16 radeon_encoder_get_dp_bridge_encoder_id(struct drm_encoder *encoder); extern u16 radeon_connector_encoder_get_dp_bridge_encoder_id(struct drm_connector *connector); extern bool radeon_connector_is_dp12_capable(struct drm_connector *connector); +bool radeon_connector_is_hdmi_monitor(struct drm_connector *connector); extern int radeon_get_monitor_bpc(struct drm_connector *connector);
extern struct edid *radeon_connector_edid(struct drm_connector *connector);
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/tegra/hdmi.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index e5d2a4026028..21571221b49b 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -831,14 +831,10 @@ static void tegra_hdmi_setup_tmds(struct tegra_hdmi *hdmi,
static bool tegra_output_is_hdmi(struct tegra_output *output) { - struct edid *edid; - if (!output->connector.edid_blob_ptr) return false;
- edid = (struct edid *)output->connector.edid_blob_ptr->data; - - return drm_detect_hdmi_monitor(edid); + return output->connector.display_info.is_hdmi; }
static enum drm_connector_status
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 3 ++- drivers/gpu/drm/gma500/psb_intel_sdvo.c | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c index e525689f84f0..d9db5d52d52e 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c @@ -130,6 +130,7 @@ static enum drm_connector_status cdv_hdmi_detect( struct edid *edid = NULL; enum drm_connector_status status = connector_status_disconnected;
+ /* This updates connector->display_info */ edid = drm_get_edid(connector, &gma_encoder->i2c_bus->adapter);
hdmi_priv->has_hdmi_sink = false; @@ -138,7 +139,7 @@ static enum drm_connector_status cdv_hdmi_detect( if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; hdmi_priv->has_hdmi_sink = - drm_detect_hdmi_monitor(edid); + connector->display_info.is_hdmi; hdmi_priv->has_hdmi_audio = drm_detect_monitor_audio(edid); } diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index 355da2856389..5ef49d17de98 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -1266,8 +1266,10 @@ psb_intel_sdvo_hdmi_sink_detect(struct drm_connector *connector) if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; if (psb_intel_sdvo->is_hdmi) { - psb_intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); - psb_intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid); + psb_intel_sdvo->has_hdmi_monitor = + connector->display_info.is_hdmi; + psb_intel_sdvo->has_hdmi_audio = + drm_detect_monitor_audio(edid); } } else status = connector_status_disconnected;
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/exynos/exynos_hdmi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 7655142a4651..a563d6386abe 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -893,12 +893,14 @@ static int hdmi_get_modes(struct drm_connector *connector) if (!edid) return -ENODEV;
- hdata->dvi_mode = !drm_detect_hdmi_monitor(edid); + /* This updates connector->display_info */ + drm_connector_update_edid_property(connector, edid); + + hdata->dvi_mode = !connector->display_info.is_hdmi; DRM_DEV_DEBUG_KMS(hdata->dev, "%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), edid->width_cm, edid->height_cm);
- drm_connector_update_edid_property(connector, edid); cec_notifier_set_phys_addr_from_edid(hdata->notifier, edid);
ret = drm_add_edid_modes(connector, edid);
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c index 58707a1f3878..07585092f919 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c @@ -364,8 +364,8 @@ static int msm_hdmi_connector_get_modes(struct drm_connector *connector)
hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl);
- hdmi->hdmi_mode = drm_detect_hdmi_monitor(edid); drm_connector_update_edid_property(connector, edid); + hdmi->hdmi_mode = connector->display_info.is_hdmi;
if (edid) { ret = drm_add_edid_modes(connector, edid);
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index 2f2c9f0a1071..f57bedbbeeb8 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -215,11 +215,11 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector) if (!edid) return 0;
- hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid); + drm_connector_update_edid_property(connector, edid); + hdmi->hdmi_monitor = connector->display_info.is_hdmi; DRM_DEBUG_DRIVER("Monitor is %s monitor\n", hdmi->hdmi_monitor ? "an HDMI" : "a DVI");
- drm_connector_update_edid_property(connector, edid); cec_s_phys_addr_from_edid(hdmi->cec_adap, edid); ret = drm_add_edid_modes(connector, edid); kfree(edid);
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/sti/sti_hdmi.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index f3ace11209dd..3f8b04a1407e 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -984,14 +984,16 @@ static int sti_hdmi_connector_get_modes(struct drm_connector *connector) if (!edid) goto fail;
- hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid); - DRM_DEBUG_KMS("%s : %dx%d cm\n", - (hdmi->hdmi_monitor ? "hdmi monitor" : "dvi monitor"), - edid->width_cm, edid->height_cm); cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
count = drm_add_edid_modes(connector, edid); + + /* This updates connector->display_info */ drm_connector_update_edid_property(connector, edid); + hdmi->hdmi_monitor = connector->display_info.is_hdmi; + DRM_DEBUG_KMS("%s : %dx%d cm\n", + (hdmi->hdmi_monitor ? "hdmi monitor" : "dvi monitor"), + edid->width_cm, edid->height_cm);
kfree(edid); return count;
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/zte/zx_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c index cd79ca0a92a9..7df682d90723 100644 --- a/drivers/gpu/drm/zte/zx_hdmi.c +++ b/drivers/gpu/drm/zte/zx_hdmi.c @@ -265,9 +265,9 @@ static int zx_hdmi_connector_get_modes(struct drm_connector *connector) if (!edid) return 0;
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); - hdmi->sink_has_audio = drm_detect_monitor_audio(edid); drm_connector_update_edid_property(connector, edid); + hdmi->sink_is_hdmi = connector->display_info.is_hdmi; + hdmi->sink_has_audio = drm_detect_monitor_audio(edid); ret = drm_add_edid_modes(connector, edid); kfree(edid);
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++-- drivers/gpu/drm/rockchip/rk3066_hdmi.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c index 7afdc54eb3ec..d479f230833e 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -553,9 +553,9 @@ static int inno_hdmi_connector_get_modes(struct drm_connector *connector)
edid = drm_get_edid(connector, hdmi->ddc); if (edid) { - hdmi->hdmi_data.sink_is_hdmi = drm_detect_hdmi_monitor(edid); - hdmi->hdmi_data.sink_has_audio = drm_detect_monitor_audio(edid); drm_connector_update_edid_property(connector, edid); + hdmi->hdmi_data.sink_is_hdmi = connector->display_info.is_hdmi; + hdmi->hdmi_data.sink_has_audio = drm_detect_monitor_audio(edid); ret = drm_add_edid_modes(connector, edid); kfree(edid); } diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c index 1c546c3a8998..03aaae39cf61 100644 --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c @@ -472,8 +472,8 @@ static int rk3066_hdmi_connector_get_modes(struct drm_connector *connector)
edid = drm_get_edid(connector, hdmi->ddc); if (edid) { - hdmi->hdmi_data.sink_is_hdmi = drm_detect_hdmi_monitor(edid); drm_connector_update_edid_property(connector, edid); + hdmi->hdmi_data.sink_is_hdmi = connector->display_info.is_hdmi; ret = drm_add_edid_modes(connector, edid); kfree(edid); }
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 76555ae64e9c..f6891280a58d 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -617,7 +617,7 @@ static struct edid *adv7511_get_edid(struct adv7511 *adv7511, __adv7511_power_off(adv7511);
adv7511_set_config_csc(adv7511, connector, adv7511->rgb, - drm_detect_hdmi_monitor(edid)); + connector->display_info.is_hdmi);
cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index 89558e581530..5719be0a03c7 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -283,7 +283,7 @@ static int sii902x_get_modes(struct drm_connector *connector) edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); drm_connector_update_edid_property(connector, edid); if (edid) { - if (drm_detect_hdmi_monitor(edid)) + if (connector->display_info.is_hdmi) output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI;
num = drm_add_edid_modes(connector, edid); diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index f08d0fded61f..33f0afb6b646 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2359,7 +2359,7 @@ static struct edid *dw_hdmi_get_edid(struct dw_hdmi *hdmi, dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n", edid->width_cm, edid->height_cm);
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); + hdmi->sink_is_hdmi = connector->display_info.is_hdmi; hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
return edid;
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 4 ++-- drivers/gpu/drm/nouveau/dispnv50/head.c | 8 +------- drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +- drivers/gpu/drm/nouveau/nouveau_connector.h | 6 ++++++ 4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index d7b9f7f8c9e3..fadd58b015d6 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -844,7 +844,7 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc, int ret; int size;
- if (!drm_detect_hdmi_monitor(nv_connector->edid)) + if (nouveau_connector_is_hdmi_monitor(&nv_connector->base)) return;
hdmi = &nv_connector->base.display_info.hdmi; @@ -1745,7 +1745,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_state *sta */ if (mode->clock >= 165000 && nv_encoder->dcb->duallink_possible && - !drm_detect_hdmi_monitor(nv_connector->edid)) + !nouveau_connector_is_hdmi_monitor(&nv_connector->base)) proto = NV507D_SOR_SET_CONTROL_PROTOCOL_DUAL_TMDS; } else { proto = NV507D_SOR_SET_CONTROL_PROTOCOL_SINGLE_TMDS_B; diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c index d66f97280282..0a138bfb8f32 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/head.c +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c @@ -127,14 +127,8 @@ nv50_head_atomic_check_view(struct nv50_head_atom *armh, struct drm_display_mode *omode = &asyh->state.adjusted_mode; struct drm_display_mode *umode = &asyh->state.mode; int mode = asyc->scaler.mode; - struct edid *edid; int umode_vdisplay, omode_hdisplay, omode_vdisplay;
- if (connector->edid_blob_ptr) - edid = (struct edid *)connector->edid_blob_ptr->data; - else - edid = NULL; - if (!asyc->scaler.full) { if (mode == DRM_MODE_SCALE_NONE) omode = umode; @@ -162,7 +156,7 @@ nv50_head_atomic_check_view(struct nv50_head_atom *armh, */ if ((asyc->scaler.underscan.mode == UNDERSCAN_ON || (asyc->scaler.underscan.mode == UNDERSCAN_AUTO && - drm_detect_hdmi_monitor(edid)))) { + nouveau_connector_is_hdmi_monitor(connector)))) { u32 bX = asyc->scaler.underscan.hborder; u32 bY = asyc->scaler.underscan.vborder; u32 r = (asyh->view.oH << 19) / asyh->view.oW; diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 22b83a6577eb..211543373b72 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -1010,7 +1010,7 @@ get_tmds_link_bandwidth(struct drm_connector *connector) unsigned duallink_scale = nouveau_duallink && nv_encoder->dcb->duallink_possible ? 2 : 1;
- if (drm_detect_hdmi_monitor(nv_connector->edid)) { + if (nouveau_connector_is_hdmi_monitor(connector)) { info = &nv_connector->base.display_info; duallink_scale = 1; } diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h index 40f90e353540..299f3a3b2331 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.h +++ b/drivers/gpu/drm/nouveau/nouveau_connector.h @@ -167,6 +167,12 @@ nouveau_connector_is_mst(struct drm_connector *connector) return encoder->encoder_type == DRM_MODE_ENCODER_DPMST; }
+static inline bool +nouveau_connector_is_hdmi_monitor(struct drm_connector *connector) +{ + return connector->display_info.is_hdmi; +} + #define nouveau_for_each_non_mst_connector_iter(connector, iter) \ drm_for_each_connector_iter(connector, iter) \ for_each_if(!nouveau_connector_is_mst(connector))
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es --- drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{ + return connector->display_info.is_hdmi; +} + static const struct drm_prop_enum_list force_audio_names[] = { { HDMI_AUDIO_OFF_DVI, "force-dvi" }, { HDMI_AUDIO_OFF, "off" }, diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 661a37a3c6d8..ceda6e72ece6 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector *connector); int intel_connector_update_modes(struct drm_connector *connector, struct edid *edid); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector); void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b04685bb6439..2b1d7c5bebdd 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid); - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); + intel_hdmi->has_hdmi_sink = intel_connector_is_hdmi_monitor(connector);
connected = true; } diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c index 6cb27599ea03..a32279e4fee8 100644 --- a/drivers/gpu/drm/i915/display/intel_sdvo.c +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; if (intel_sdvo_connector->is_hdmi) { - intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid); + intel_sdvo->has_hdmi_monitor = + intel_connector_is_hdmi_monitor(connector); } } else status = connector_status_disconnected;
On Fri, Oct 15, 2021 at 01:37:13PM +0200, Claudio Suarez wrote:
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{
- return connector->display_info.is_hdmi;
+}
static const struct drm_prop_enum_list force_audio_names[] = { { HDMI_AUDIO_OFF_DVI, "force-dvi" }, { HDMI_AUDIO_OFF, "off" }, diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 661a37a3c6d8..ceda6e72ece6 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector *connector); int intel_connector_update_modes(struct drm_connector *connector, struct edid *edid); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector); void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b04685bb6439..2b1d7c5bebdd 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
intel_hdmi->has_hdmi_sink = intel_connector_is_hdmi_monitor(connector);
Hmm. Have we parse the EDID by this point actually? I don't think that was the case in the past but maybe it changed at some point.
connected = true;} diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c index 6cb27599ea03..a32279e4fee8 100644 --- a/drivers/gpu/drm/i915/display/intel_sdvo.c +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; if (intel_sdvo_connector->is_hdmi) {
intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
intel_sdvo->has_hdmi_monitor =intel_connector_is_hdmi_monitor(connector);
FYI there's a third copy of this in intel_dp.c
} } else status = connector_status_disconnected;-- 2.33.0
On Fri, Oct 15, 2021 at 03:30:49PM +0300, Ville Syrjälä wrote:
On Fri, Oct 15, 2021 at 01:37:13PM +0200, Claudio Suarez wrote:
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{
- return connector->display_info.is_hdmi;
+}
static const struct drm_prop_enum_list force_audio_names[] = { { HDMI_AUDIO_OFF_DVI, "force-dvi" }, { HDMI_AUDIO_OFF, "off" }, diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 661a37a3c6d8..ceda6e72ece6 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector *connector); int intel_connector_update_modes(struct drm_connector *connector, struct edid *edid); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector); void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b04685bb6439..2b1d7c5bebdd 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
intel_hdmi->has_hdmi_sink = intel_connector_is_hdmi_monitor(connector);Hmm. Have we parse the EDID by this point actually? I don't think that was the case in the past but maybe it changed at some point.
Yes, I think so. The complete code is:
---- edid = drm_get_edid(connector, i2c);
if (!edid && !intel_gmbus_is_forced_bit(i2c)) { drm_dbg_kms(&dev_priv->drm, "HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n"); intel_gmbus_force_bit(i2c, true); edid = drm_get_edid(connector, i2c); intel_gmbus_force_bit(i2c, false); }
intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid); intel_hdmi->has_hdmi_sink = intel_connector_is_hdmi_monitor(connector); ---- The edid value comes from drm_get_edid(), first or second. drm_get_edid() internally calls drm_connector_update_edid_property() drm_connector_update_edid_property() calls drm_add_display_info() and parses the edid. So, the edid is parsed. I checked this and I read the docs many times because at the first time I felt something was wrong. But that is the sequence of calls.
connected = true;} diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c index 6cb27599ea03..a32279e4fee8 100644 --- a/drivers/gpu/drm/i915/display/intel_sdvo.c +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; if (intel_sdvo_connector->is_hdmi) {
intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
intel_sdvo->has_hdmi_monitor =intel_connector_is_hdmi_monitor(connector);FYI there's a third copy of this in intel_dp.c
Yes. But in this case the edid comes from intel_dp_get_edid(). In intel_dp_get_edid(), there is a if. One of the branches calls drm_get_edid(), so no problem here. But the other branch gets the edid from drm_edid_duplicate(). I haven't seen any guarantee that display_info is updated in this case.
I didn't change this file for that reason.
Thank you for your comments :)
BR Claudio Suarez.
On Fri, 15 Oct 2021, Claudio Suarez cssk@net-c.es wrote:
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{
- return connector->display_info.is_hdmi;
+}
A helper like this belongs in drm, not i915. Seems useful in other drivers too.
BR, Jani.
static const struct drm_prop_enum_list force_audio_names[] = { { HDMI_AUDIO_OFF_DVI, "force-dvi" }, { HDMI_AUDIO_OFF, "off" }, diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 661a37a3c6d8..ceda6e72ece6 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector *connector); int intel_connector_update_modes(struct drm_connector *connector, struct edid *edid); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector); void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b04685bb6439..2b1d7c5bebdd 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
intel_hdmi->has_hdmi_sink = intel_connector_is_hdmi_monitor(connector);connected = true; }
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c index 6cb27599ea03..a32279e4fee8 100644 --- a/drivers/gpu/drm/i915/display/intel_sdvo.c +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; if (intel_sdvo_connector->is_hdmi) {
intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
intel_sdvo->has_hdmi_monitor =intel_connector_is_hdmi_monitor(connector); }
On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
On Fri, 15 Oct 2021, Claudio Suarez cssk@net-c.es wrote:
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{
- return connector->display_info.is_hdmi;
+}
A helper like this belongs in drm, not i915. Seems useful in other drivers too.
Not sure it's actually helpful for i915. We end up having to root around in the display_info in a lot of places anyway. So a helper for single boolean seems a bit out of place perhaps.
On Fri, 15 Oct 2021, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
On Fri, 15 Oct 2021, Claudio Suarez cssk@net-c.es wrote:
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{
- return connector->display_info.is_hdmi;
+}
A helper like this belongs in drm, not i915. Seems useful in other drivers too.
Not sure it's actually helpful for i915. We end up having to root around in the display_info in a lot of places anyway. So a helper for single boolean seems a bit out of place perhaps.
*shrug*
Maybe it's just my frustration at the lack of interfaces and poking around in the depths of nested structs and pointer chasing that's coming through. You just need to change so many things if you want to later refactor where "is hdmi" comes from and is stored.
Anyway, if a helper is being added like in this series, I think it should be one helper in drm, not redundant copies in multiple drivers. Or we should not have the helper(s) at all. One or the other, not the worst of both worlds.
BR, Jani.
On Fri, Oct 15, 2021 at 06:18:34PM +0300, Jani Nikula wrote:
On Fri, 15 Oct 2021, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
On Fri, 15 Oct 2021, Claudio Suarez cssk@net-c.es wrote:
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible.
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez cssk@net-c.es
drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++ drivers/gpu/drm/i915/display/intel_connector.h | 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 9bed1ccecea0..3346b55df6e1 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
+bool intel_connector_is_hdmi_monitor(struct drm_connector *connector) +{
- return connector->display_info.is_hdmi;
+}
A helper like this belongs in drm, not i915. Seems useful in other drivers too.
Not sure it's actually helpful for i915. We end up having to root around in the display_info in a lot of places anyway. So a helper for single boolean seems a bit out of place perhaps.
*shrug*
Maybe it's just my frustration at the lack of interfaces and poking around in the depths of nested structs and pointer chasing that's coming through. You just need to change so many things if you want to later refactor where "is hdmi" comes from and is stored.
Anyway, if a helper is being added like in this series, I think it should be one helper in drm, not redundant copies in multiple drivers. Or we should not have the helper(s) at all. One or the other, not the worst of both worlds.
Thank you all for your comments :) The big work here was to figure out which drm_detect_hdmi_monitor() can be replaced. Changing a helper isn't a problem. I'll send a new patch in a few hours.
BR Claudio Suarez.
dri-devel@lists.freedesktop.org
-
Claudio Suarez -
Harry Wentland -
Jani Nikula -
Ville Syrjälä