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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center