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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; airlied@xxxxxxxx; pihsun@xxxxxxxxxxxx; sean@xxxxxxxxxx
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@xxxxxxxxxxxxxxx]
Sent: Tuesday, December 03, 2019 4:02 PM
To: Allen Chen (
陳柏宇)
Cc: Jau-Chih Tseng (
曾昭智); maxime.ripard@xxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; airlied@xxxxxxxx; pihsun@xxxxxxxxxxxx; sean@xxxxxxxxxx
Subject: RE: [PATCH] drm/edid: fixup EDID 1.3 and 1.4 judge reduced-blanking timings logic

 

On Tue, 03 Dec 2019, <allen.chen@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx]

> 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@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 = "">

> 

>> +   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@xxxxxxxxxxxxxxx>'s suggestion.

> 

> 

> 

> BR,

> 

> Jani.

> 

> 

> 

> 

> 

>>    }

> 

>> 

> 

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

> 

> 

> 

> --

> 

> Jani Nikula, Intel Open Source Graphics Center

 

--

Jani Nikula, Intel Open Source Graphics Center

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux