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

allen.chen at ite.com.tw allen.chen at ite.com.tw
Tue Dec 10 08:01:56 UTC 2019


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 at bootlin.com; linux-kernel at vger.kernel.org; dri-devel at lists.freedesktop.org; airlied at linux.ie; pihsun at chromium.org; sean at 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 at linux.intel.com]
Sent: Tuesday, December 03, 2019 4:02 PM
To: Allen Chen (陳柏宇)
Cc: Jau-Chih Tseng (曾昭智); maxime.ripard at bootlin.com; linux-kernel at vger.kernel.org; dri-devel at lists.freedesktop.org; airlied at linux.ie; pihsun at chromium.org; sean at 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 at 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 at 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 at 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 at ite.com.tw>

>

>> Reported-by: kbuild test robot <lkp at 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 at 01D5A9EA.9B7364D0]

>

>

>

> [cid:image005.jpg at 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 at 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 at linux.intel.com<mailto:ville.syrjala at linux.intel.com>>'s suggestion.

[cid:image001.png at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191210/214aa368/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 13750 bytes
Desc: image001.png
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191210/214aa368/attachment-0001.png>


More information about the dri-devel mailing list