| Message ID | 1574761572-26585-1-git-send-email-allen.chen@ite.com.tw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic | expand |
On Tue, 26 Nov 2019, allen <allen.chen@ite.com.tw> 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@ite.com.tw> > Reported-by: kbuild test robot <lkp@intel.com> > --- > 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)) { > + closure->valid_support_rb = true; > + closure->support_rb = (r[15] & 0x10) ? true : false; > + } > + } > } > > /* 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; Are you planning on extending the closure use somehow? I did not look up the spec, but purely on the code changes alone, you could just move the edid->features bit check at this level, and not pass it down, and nothing would change. I.e. don't iterate the EDID at all if the bit is not set. You also don't actually use the distinction between valid_support_rb vs. support_rb for anything, so the closure just adds code. BR, Jani. > } > > return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
Hi Jani Nikula Thanks for your suggestion and I have replied two comments below. -----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, November 27, 2019 6:29 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@ite.com.tw> 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@ite.com.tw> > Reported-by: kbuild test robot <lkp@intel.com> > --- > 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)) { > + closure->valid_support_rb = true; > + closure->support_rb = (r[15] & 0x10) ? true : false; > + } > + } > } > > /* 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; Are you planning on extending the closure use somehow? I did not look up the spec, ==> iTE: as the picture below, from VESA E-EDID standard [cid:image003.jpg@01D5A9EA.9B7364D0] [cid:image005.jpg@01D5A9EA.9B7364D0] if EDID bit 0 in feature support byte = 1 && detailed timing descriptor byte 10 = 0x04 then the CVT timing supported. [cid:image009.jpg@01D5A9EA.9B7364D0] If CVT timing supported then we can check byte 15 bit 4 to determine whether the reduced-blanking timings suported or not. If CVT timing not supported then we can not use byte 15 to judge. but purely on the code changes alone, you could just move the edid->features bit check at this level, and not pass it down, and nothing would change. I.e. don't iterate the EDID at all if the bit is not set. ð iTE: We still have to check whether detailed timing descriptor byte 10 = 0x04 or not, so it is hard to check at this level You also don't actually use the distinction between valid_support_rb vs. support_rb for anything, so the closure just adds code. BR, Jani. > } > > return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0); -- Jani Nikula, Intel Open Source Graphics Center
On Tue, 03 Dec 2019, <allen.chen@ite.com.tw> wrote: > Hi Jani Nikula > > > > Thanks for your suggestion and I have replied two comments below. Please read my question again. BR, Jani. > > > > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Wednesday, November 27, 2019 6:29 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@ite.com.tw> 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@ite.com.tw> > >> Reported-by: kbuild test robot <lkp@intel.com> > >> --- > >> 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)) { > >> + closure->valid_support_rb = true; > >> + closure->support_rb = (r[15] & 0x10) ? true : false; > >> + } > >> + } > >> } > >> > >> /* 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; > > > > Are you planning on extending the closure use somehow? > > > > I did not look up the spec, > > > > ==> iTE: as the picture below, from VESA E-EDID standard > > [cid:image003.jpg@01D5A9EA.9B7364D0] > > > > [cid:image005.jpg@01D5A9EA.9B7364D0] > > > > if EDID bit 0 in feature support byte = 1 && detailed timing descriptor byte 10 = 0x04 then the CVT timing supported. > > > > [cid:image009.jpg@01D5A9EA.9B7364D0] > > > > If CVT timing supported then we can check byte 15 bit 4 to determine whether the reduced-blanking timings suported or not. > > If CVT timing not supported then we can not use byte 15 to judge. > > but purely on the code changes alone, you > > could just move the edid->features bit check at this level, and not pass > > it down, and nothing would change. I.e. don't iterate the EDID at all if > > the bit is not set. > > > > ð iTE: We still have to check whether detailed timing descriptor byte 10 = 0x04 or not, so it is hard to check at this level > > You also don't actually use the distinction between valid_support_rb > > vs. support_rb for anything, so the closure just adds code. > > > > BR, > > Jani. > > > > > >> } > >> > >> return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0); > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
Hi Jani Nikula Thanks for your suggestion and I have replied one comment below. -----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, December 03, 2019 4:02 PM To: Allen Chen (陳柏宇) Cc: Jau-Chih Tseng (曾昭智); maxime.ripard@bootlin.com; linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; airlied@linux.ie; pihsun@chromium.org; sean@poorly.run Subject: RE: [PATCH] drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic On Tue, 03 Dec 2019, <allen.chen@ite.com.tw> wrote: > Hi Jani Nikula > > > > Thanks for your suggestion and I have replied two comments below. Please read my question again. BR, Jani. > > > > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Wednesday, November 27, 2019 6:29 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@ite.com.tw> 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@ite.com.tw> > >> Reported-by: kbuild test robot <lkp@intel.com> > >> --- > >> 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)) { > >> + closure->valid_support_rb = true; > >> + closure->support_rb = (r[15] & 0x10) ? true : false; > >> + } > >> + } > >> } > >> > >> /* 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; > > > > Are you planning on extending the closure use somehow? > > > > I did not look up the spec, > > > > ==> iTE: as the picture below, from VESA E-EDID standard > > [cid:image003.jpg@01D5A9EA.9B7364D0] > > > > [cid:image005.jpg@01D5A9EA.9B7364D0] > > > > if EDID bit 0 in feature support byte = 1 && detailed timing descriptor byte 10 = 0x04 then the CVT timing supported. > > > > [cid:image009.jpg@01D5A9EA.9B7364D0] > > > > If CVT timing supported then we can check byte 15 bit 4 to determine whether the reduced-blanking timings suported or not. > > If CVT timing not supported then we can not use byte 15 to judge. > > but purely on the code changes alone, you > > could just move the edid->features bit check at this level, and not pass > > it down, and nothing would change. I.e. don't iterate the EDID at all if > > the bit is not set. > > > > ð iTE: We still have to check whether detailed timing descriptor byte 10 = 0x04 or not, so it is hard to check at this level > > You also don't actually use the distinction between valid_support_rb > > vs. support_rb for anything, so the closure just adds code. ==> ITE: From Ville Syrjälä <ville.syrjala@linux.intel.com<mailto:ville.syrjala@linux.intel.com>>'s suggestion. [cid:image001.png@01D5AB4D.AE02ACA0] > > > > BR, > > Jani. > > > > > >> } > >> > >> return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0); > > > > -- > > Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center
Hi Jani Sorry to bother you. May this patch is OK to be upstream? If have any suggestion, I will try to modify the code to meet the upstream standard. From: Allen Chen (陳柏宇) Sent: Thursday, December 05, 2019 9:24 AM To: 'Jani Nikula' Cc: Jau-Chih Tseng (曾昭智); maxime.ripard@bootlin.com; linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; airlied@linux.ie; pihsun@chromium.org; sean@poorly.run Subject: RE: [PATCH] drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic Hi Jani Nikula Thanks for your suggestion and I have replied one comment below. -----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, December 03, 2019 4:02 PM To: Allen Chen (陳柏宇) Cc: Jau-Chih Tseng (曾昭智); maxime.ripard@bootlin.com; linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; airlied@linux.ie; pihsun@chromium.org; sean@poorly.run Subject: RE: [PATCH] drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic On Tue, 03 Dec 2019, <allen.chen@ite.com.tw> wrote: > Hi Jani Nikula > > > > Thanks for your suggestion and I have replied two comments below. Please read my question again. BR, Jani. > > > > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Wednesday, November 27, 2019 6:29 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@ite.com.tw> 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@ite.com.tw> > >> Reported-by: kbuild test robot <lkp@intel.com> > >> --- > >> 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)) { > >> + closure->valid_support_rb = true; > >> + closure->support_rb = (r[15] & 0x10) ? true : false; > >> + } > >> + } > >> } > >> > >> /* 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; > > > > Are you planning on extending the closure use somehow? > > > > I did not look up the spec, > > > > ==> iTE: as the picture below, from VESA E-EDID standard > > [cid:image003.jpg@01D5A9EA.9B7364D0] > > > > [cid:image005.jpg@01D5A9EA.9B7364D0] > > > > if EDID bit 0 in feature support byte = 1 && detailed timing descriptor byte 10 = 0x04 then the CVT timing supported. > > > > [cid:image009.jpg@01D5A9EA.9B7364D0] > > > > If CVT timing supported then we can check byte 15 bit 4 to determine whether the reduced-blanking timings suported or not. > > If CVT timing not supported then we can not use byte 15 to judge. > > but purely on the code changes alone, you > > could just move the edid->features bit check at this level, and not pass > > it down, and nothing would change. I.e. don't iterate the EDID at all if > > the bit is not set. > > > > ð iTE: We still have to check whether detailed timing descriptor byte 10 = 0x04 or not, so it is hard to check at this level > > You also don't actually use the distinction between valid_support_rb > > vs. support_rb for anything, so the closure just adds code. ==> ITE: From Ville Syrjälä <ville.syrjala@linux.intel.com<mailto:ville.syrjala@linux.intel.com>>'s suggestion. [cid:image001.png@01D5AF72.CEA3EE00] > > > > BR, > > Jani. > > > > > >> } > >> > >> return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0); > > > > -- > > Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center
On Tue, 26 Nov 2019, allen <allen.chen@ite.com.tw> 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@ite.com.tw> > Reported-by: kbuild test robot <lkp@intel.com> > --- > 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);
Hi Jani Thanks for your help and I will follow your suggestion to modify the patch. -----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 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@ite.com.tw> 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@ite.com.tw> > Reported-by: kbuild test robot <lkp@intel.com> > --- > 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);
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)) { + closure->valid_support_rb = true; + closure->support_rb = (r[15] & 0x10) ? true : false; + } + } } /* 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; } return ((edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
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@ite.com.tw> Reported-by: kbuild test robot <lkp@intel.com> --- drivers/gpu/drm/drm_edid.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-)