RE: [PATCH] drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic

[allen.chen]

Hi Jani

Thanks for your help and I will follow your suggestion to modify the patch.

-----Original Message-----
From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] 
Sent: Tuesday, December 10, 2019 7:10 PM
To: Allen Chen (陳柏宇)
Cc: Jau-Chih Tseng (曾昭智); Maxime Ripard; Allen Chen (陳柏宇); open list; open list:DRM DRIVERS; David Airlie; Pi-Hsun Shih; Sean Paul
Subject: Re: [PATCH] drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic

On Tue, 26 Nov 2019, allen <allen.chen@xxxxxxxxxx> wrote:
> According to VESA ENHANCED EXTENDED DISPLAY IDENTIFICATION DATA STANDARD
> (Defines EDID Structure Version 1, Revision 4) page: 39
> How to determine whether the monitor support RB timing or not?
> EDID 1.4
> First:  read detailed timing descriptor and make sure byte 0 = 0x00,
> 	byte 1 = 0x00, byte 2 = 0x00 and byte 3 = 0xFD
> Second: read EDID bit 0 in feature support byte at address 18h = 1
> 	and detailed timing descriptor byte 10 = 0x04
> Third:  if EDID bit 0 in feature support byte = 1 &&
> 	detailed timing descriptor byte 10 = 0x04
> 	then we can check byte 15, if bit 4 in byte 15 = 1 is support RB
>         if EDID bit 0 in feature support byte != 1 ||
> 	detailed timing descriptor byte 10 != 0x04,
> 	then byte 15 can not be used
>
> The linux code is_rb function not follow the VESA's rule
>
> Signed-off-by: Allen Chen <allen.chen@xxxxxxxxxx>
> Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_edid.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f5926bf..e11e585 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -93,6 +93,12 @@ struct detailed_mode_closure {
>  	int modes;
>  };
>  
> +struct edid_support_rb_closure {
> +	struct edid *edid;
> +	bool valid_support_rb;
> +	bool support_rb;
> +};
> +
>  #define LEVEL_DMT	0
>  #define LEVEL_GTF	1
>  #define LEVEL_GTF2	2
> @@ -2017,23 +2023,41 @@ struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  	}
>  }
>  
> +static bool
> +is_display_descriptor(const u8 *r, u8 tag)
> +{
> +	return (!r[0] && !r[1] && !r[2] && r[3] == tag) ? true : false;
> +}
> +
>  static void
>  is_rb(struct detailed_timing *t, void *data)
>  {
>  	u8 *r = (u8 *)t;
> -	if (r[3] == EDID_DETAIL_MONITOR_RANGE)
> -		if (r[15] & 0x10)
> -			*(bool *)data = true;
> +	struct edid_support_rb_closure *closure = data;
> +	struct edid *edid = closure->edid;
> +
> +	if (is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE)) {
> +		if (edid->features & BIT(0) && r[10] == BIT(2)) {

I'll try to explain my original comment again.

Consider edid->features & BIT(0). It remains unchanged across the
iteration. The code will only change anything if edid->features &
BIT(0).

> +			closure->valid_support_rb = true;
> +			closure->support_rb = (r[15] & 0x10) ? true : false;

You could combine these to e.g. a single int.

	if (r[10] == BIT(2)) {
		int *ret = data;
		*ret = !!(r[15] & 0x10);
	}

> +		}
> +	}
>  }
>  
>  /* EDID 1.4 defines this explicitly.  For EDID 1.3, we guess, badly. */
>  static bool
>  drm_monitor_supports_rb(struct edid *edid)
>  {
> +	struct edid_support_rb_closure closure = {
> +		.edid = edid,
> +		.valid_support_rb = false,
> +		.support_rb = false,
> +	};
> +
>  	if (edid->revision >= 4) {
> -		bool ret = false;
> -		drm_for_each_detailed_block((u8 *)edid, is_rb, &ret);
> -		return ret;
> +		drm_for_each_detailed_block((u8 *)edid, is_rb, &closure);
> +		if (closure.valid_support_rb)
> +			return closure.support_rb;

Here, you'd do:

        if (edid->features & BIT(0)) {
        	int ret = -1;
		drm_for_each_detailed_block((u8 *)edid, is_rb, &ret);
                if (ret != -1)
                	return ret;
	}


>  	}
>  
>  	return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0);

-- 
Jani Nikula, Intel Open Source Graphics Center