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

[Ville Syrjälä]

On Mon, Nov 11, 2019 at 01:43:52AM +0000, allen.chen@xxxxxxxxxx wrote:
> Hi Ville SyrjÃlÃ
> 
> Thanks for your suggestion and I have replied two comments below.
> 
> From: Ville SyrjÃlà [mailto:ville.syrjala@xxxxxxxxxxxxxxx] 
> Sent: Thursday, November 07, 2019 11:42 PM
> To: Allen Chen (éæå)
> Cc: Jau-Chih Tseng (æææ); Maxime Ripard; 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 Mon, Nov 04, 2019 at 04:42:49PM +0800, allen 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 byte0 = 0,
> > 	byte1 = 0, byte2 = 0 and byte3 = 0xFD
> 
> That should probably be some new function:
> bool is_display_descriptor(const u8 *desc, u8 tag);
> is_display_descriptor(EDID_DETAIL_MONITOR_RANGE)
> or something along those lines
> 
> We don't seem to check that in most places so should be rolled out all
> over. The usage of struct detailed_timing all over also makes everything
> rather confusing.
> 
> > Second: read detailed timing descriptor byte10 = 0x04 and
> > 	EDID byte18h bit0 = 1
> 
> Indicates CVT support. Should give these things real names so
> one wouldn't have to decode by hand.
> 
> > Third:  if EDID byte18h bit0 == 1 && byte10 == 0x04,
> > 	then we can check byte15, if byte15 bit4 =1 is support RB
> >         if EDID byte18h bit0 != 1 || byte10 != 0x04,
> > 	then byte15 can not be used
> > 
> > The linux code is_rb function not follow the VESA's rule
> > 
> > EDID 1.3
> > LCDâflatâpanels doânotârequireâlongâblankingâintervalsâasâaâretrace
> > period so default support reduced-blanking timings.
> > 
> > Signed-off-by: Allen Chen <allen.chen@xxxxxxxxxx>
> > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index e5e7e65..9b67b80 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -93,6 +93,11 @@ struct detailed_mode_closure {
> >  	int modes;
> >  };
> >  
> > +struct edid_support_rb_closure {
> > +	struct edid *edid;
> > +	s8 support_rb;
> 
> bool
> 
> ==> ITE:  If use bool, we could not return EDID1.3 when EDID1.4 logic can not be applied

Hmm. Could use two bools then.

> > +};
> > +
> >  #define LEVEL_DMT	0
> >  #define LEVEL_GTF	1
> >  #define LEVEL_GTF2	2
> > @@ -2018,22 +2023,31 @@ struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> >  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 (!r[0] && !r[1] && !r[2] && r[3] == EDID_DETAIL_MONITOR_RANGE) {
> > +		if (edid->features & BIT(0) && r[10] == BIT(2))
> > +			closure->support_rb = (r[15] & 0x10) ? 1 : 0;
> 
> With the bool the ternary operator is not needed. Also should maybe 
> be |= in case we have multiple range descriptors? Not sure that is
> legal.
> 
> > +	}
> >  }
> >  
> >  /* 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,
> > +		.support_rb = -1,
> > +	};
> > +
> >  	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.support_rb >= 0)
> > +			return closure.support_rb;
> >  	}
> >  
> > -	return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
> > +	return true;
> 
> Why are we now assuming rb for all pre 1.4 EDIDs?
> 
> ==> ITE: Today, most of the monitor are LCD and LCD monitor do not require long blanking intervals as a retrace period so default support reduced-blanking timings.

You can't assume such things. Someone out there is surely still using
something that doesn't do reduced blanking.

> 
> >  }
> >  
> >  static void
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville SyrjÃlÃ
> Intel

-- 
Ville SyrjÃlÃ
Intel