diff mbox series

[4/4] drm/edid: Avoid multiple log lines for HFVSDB parsing

Message ID 20220811054718.2115917-5-ankit.k.nautiyal@intel.com (mailing list archive)
State New, archived
Headers show
Series Fix HFVSDB parsing | expand

Commit Message

Ankit Nautiyal Aug. 11, 2022, 5:47 a.m. UTC 
Replace multiple log lines with a single log line at the end of
parsing HF-VSDB. Also use drm_dbg_kms instead of DRM_DBG_KMS, and
add log for DSC1.2 support.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Jani Nikula Sept. 13, 2022, 1:54 p.m. UTC | #1 
On Thu, 11 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Replace multiple log lines with a single log line at the end of
> parsing HF-VSDB. Also use drm_dbg_kms instead of DRM_DBG_KMS, and
> add log for DSC1.2 support.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c9c3a9c8fa26..7a319d570297 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5781,6 +5781,9 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  	struct drm_display_info *display = &connector->display_info;
>  	struct drm_hdmi_info *hdmi = &display->hdmi;
>  	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
> +	u32 max_tmds_clock = 0;

This should be int because display->max_tmds_clock is int. Yes, it's a
change from the current local var, but logging u32 would require %u
instead of %d in the format string anyway, so better just use the right
type.

> +	u8 max_frl_rate = 0;
> +	bool dsc_support = false;
>  
>  	display->has_hdmi_infoframe = true;
>  
> @@ -5800,14 +5803,13 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  	 */
>  
>  	if (hf_scds[5]) {
> -		/* max clock is 5000 KHz times block value */
> -		u32 max_tmds_clock = hf_scds[5] * 5000;
>  		struct drm_scdc *scdc = &hdmi->scdc;
>  
> +		/* max clock is 5000 KHz times block value */
> +		max_tmds_clock = hf_scds[5] * 5000;
> +
>  		if (max_tmds_clock > 340000) {
>  			display->max_tmds_clock = max_tmds_clock;
> -			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> -				display->max_tmds_clock);

Hmm, the logic for what is logged gets changed.

>  		}
>  
>  		if (scdc->supported) {
> @@ -5820,9 +5822,6 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  	}
>  
>  	if (hf_scds[7]) {
> -		u8 max_frl_rate;
> -
> -		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
>  		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
>  		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>  				     &hdmi->max_frl_rate_per_lane);
> @@ -5830,8 +5829,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  
>  	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
>  
> -	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
> +	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
>  		drm_parse_dsc_info(hdmi_dsc, hf_scds);
> +		dsc_support = true;
> +	}
> +
> +	drm_dbg_kms(connector->dev,
> +		    "HF-VSDB: max TMDS clock:%d Khz, HDMI2.1 support:%s, DSC1.2 support:%s\n",

Nitpicks, %d needs int instead of u32, "kHz" not "Khz", "HDMI 2.1" and
"DSC 1.2" with spaces, would prefer a space after ":".

> +		    max_tmds_clock, max_frl_rate ? "yes" : "no", dsc_support ? "yes" : "no");

See str_yes_no().

BR,
Jani.

>  }
>  
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
Ankit Nautiyal Sept. 14, 2022, 10:09 a.m. UTC | #2 
Thanks Jani for the review and suggestions.

I agree with the suggestions and will make changes in next version.

Please find my response inline:

On 9/13/2022 7:24 PM, Jani Nikula wrote:
> On Thu, 11 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Replace multiple log lines with a single log line at the end of
>> parsing HF-VSDB. Also use drm_dbg_kms instead of DRM_DBG_KMS, and
>> add log for DSC1.2 support.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index c9c3a9c8fa26..7a319d570297 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -5781,6 +5781,9 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   	struct drm_display_info *display = &connector->display_info;
>>   	struct drm_hdmi_info *hdmi = &display->hdmi;
>>   	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
>> +	u32 max_tmds_clock = 0;
> This should be int because display->max_tmds_clock is int. Yes, it's a
> change from the current local var, but logging u32 would require %u
> instead of %d in the format string anyway, so better just use the right
> type.
Alright, makes sense.
>> +	u8 max_frl_rate = 0;
>> +	bool dsc_support = false;
>>   
>>   	display->has_hdmi_infoframe = true;
>>   
>> @@ -5800,14 +5803,13 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   	 */
>>   
>>   	if (hf_scds[5]) {
>> -		/* max clock is 5000 KHz times block value */
>> -		u32 max_tmds_clock = hf_scds[5] * 5000;
>>   		struct drm_scdc *scdc = &hdmi->scdc;
>>   
>> +		/* max clock is 5000 KHz times block value */
>> +		max_tmds_clock = hf_scds[5] * 5000;
>> +
>>   		if (max_tmds_clock > 340000) {
>>   			display->max_tmds_clock = max_tmds_clock;
>> -			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> -				display->max_tmds_clock);
> Hmm, the logic for what is logged gets changed.

You are right, we are now logging this always.

Should we log this only for rate > 340MHz? The logging line at last will 
require some jugglery.

>>   		}
>>   
>>   		if (scdc->supported) {
>> @@ -5820,9 +5822,6 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   	}
>>   
>>   	if (hf_scds[7]) {
>> -		u8 max_frl_rate;
>> -
>> -		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
>>   		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
>>   		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>>   				     &hdmi->max_frl_rate_per_lane);
>> @@ -5830,8 +5829,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   
>>   	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
>>   
>> -	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
>> +	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
>>   		drm_parse_dsc_info(hdmi_dsc, hf_scds);
>> +		dsc_support = true;
>> +	}
>> +
>> +	drm_dbg_kms(connector->dev,
>> +		    "HF-VSDB: max TMDS clock:%d Khz, HDMI2.1 support:%s, DSC1.2 support:%s\n",
> Nitpicks, %d needs int instead of u32, "kHz" not "Khz", "HDMI 2.1" and
> "DSC 1.2" with spaces, would prefer a space after ":".
Noted, Will fix this.
>
>> +		    max_tmds_clock, max_frl_rate ? "yes" : "no", dsc_support ? "yes" : "no");
> See str_yes_no().

Right, should have used str_yes_no().


Thanks & Regards,

Ankit

>
> BR,
> Jani.
>
>>   }
>>   
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c9c3a9c8fa26..7a319d570297 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5781,6 +5781,9 @@  static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	struct drm_display_info *display = &connector->display_info;
 	struct drm_hdmi_info *hdmi = &display->hdmi;
 	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
+	u32 max_tmds_clock = 0;
+	u8 max_frl_rate = 0;
+	bool dsc_support = false;
 
 	display->has_hdmi_infoframe = true;
 
@@ -5800,14 +5803,13 @@  static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	 */
 
 	if (hf_scds[5]) {
-		/* max clock is 5000 KHz times block value */
-		u32 max_tmds_clock = hf_scds[5] * 5000;
 		struct drm_scdc *scdc = &hdmi->scdc;
 
+		/* max clock is 5000 KHz times block value */
+		max_tmds_clock = hf_scds[5] * 5000;
+
 		if (max_tmds_clock > 340000) {
 			display->max_tmds_clock = max_tmds_clock;
-			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
-				display->max_tmds_clock);
 		}
 
 		if (scdc->supported) {
@@ -5820,9 +5822,6 @@  static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	}
 
 	if (hf_scds[7]) {
-		u8 max_frl_rate;
-
-		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
 		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
 		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
 				     &hdmi->max_frl_rate_per_lane);
@@ -5830,8 +5829,14 @@  static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 
 	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
 
-	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
+	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
 		drm_parse_dsc_info(hdmi_dsc, hf_scds);
+		dsc_support = true;
+	}
+
+	drm_dbg_kms(connector->dev,
+		    "HF-VSDB: max TMDS clock:%d Khz, HDMI2.1 support:%s, DSC1.2 support:%s\n",
+		    max_tmds_clock, max_frl_rate ? "yes" : "no", dsc_support ? "yes" : "no");
 }
 
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,