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
Second: read detailed timing descriptor byte10 = 0x04 and
EDID byte18h bit0 = 1
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 <[email protected]>
Reported-by: kbuild test robot <[email protected]>
---
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;
+};
+
#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;
+ }
}
/* 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;
}
static void
--
1.9.1
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 <[email protected]>
> Reported-by: kbuild test robot <[email protected]>
> ---
> 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
> +};
> +
> #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?
> }
>
> static void
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
Hi Ville Syrjälä
Thanks for your suggestion and I have replied two comments below.
From: Ville Syrjälä [mailto:[email protected]]
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 <[email protected]>
> Reported-by: kbuild test robot <[email protected]>
> ---
> 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
> +};
> +
> #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.
> }
>
> static void
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
On Mon, Nov 11, 2019 at 01:43:52AM +0000, [email protected] wrote:
> Hi Ville Syrjälä
>
> Thanks for your suggestion and I have replied two comments below.
>
> From: Ville Syrjälä [mailto:[email protected]]
> 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 <[email protected]>
> > Reported-by: kbuild test robot <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
--
Ville Syrjälä
Intel