[PATCH] to detail timing of EDID extension

Ma, Ling ling.ma at intel.com
Wed Nov 12 04:42:34 PST 2008


Hi ajax
Thanks for your comments!

This is updated version:
In this version I divide patch into 5 parts:
[patch 1/5] define extension structure & implementation in interpret_edid.c
[patch 2/5] detailed timing implementation in xf86EdidModes.c
[patch 3/5] detailed timing implementation in xf86Crtc.c
[patch 4/5] detailed timing implementation in xf86Configure.c
[patch 5/5] detailed timing implementation in print_edid.c

Any comments are welcome!

Thanks
Ma Ling


>-----Original Message-----
>From: Adam Jackson [mailto:ajax at nwnk.net]
>Sent: 2008年11月11日 23:54
>To: Ma, Ling
>Cc: xorg at lists.freedesktop.org
>Subject: Re: [PATCH] to detail timing of EDID extension
>
>On Tue, 2008-11-11 at 11:45 +0800, Ma, Ling wrote:
>> Hi
>> Hi All
>> In order not to pollute ABI xf86Monitor when we append new extensions
>> such as CEA, VTB ,DI, LS, ... I did this patch , in which I moved  all
>> original and extension detail timing operations into the unified
>> interface (xf86ForEachDetailedBlock), then handle them respectively. I
>> have finished test on my G45 & BenQ G2400W connected by HDMI.
>
>A few remarks:
>
>> +    xf86ForEachDetailedBlock(ConfiguredMonitor, handle_detailed_input,
>> +                             (void *)ptr);
>
>No need for the explicit (void *) here.  The prototype already says it's
>void *, and any pointer can be passed as void * without warning.
>
>> @@ -64,12 +64,12 @@ handle_edid_quirks(xf86MonPtr m)
>>       * similar.  Strictly we should refuse to round up too far, but let's
>>       * see how well this works.
>>       */
>> -    for (i = 0; i < 4; i++) {
>> -       if (m->det_mon[i].type == DS_RANGES) {
>> -           ranges = &m->det_mon[i].section.ranges;
>> -           for (j = 0; j < 4; j++) {
>> -               if (m->det_mon[j].type == DT) {
>> -                   preferred_timing = &m->det_mon[j].section.d_timings;
>> +    for (i = 0; i < det_mon_num; i++) {
>> +       if (det_mon[i].type == DS_RANGES) {
>> +           ranges = &det_mon[i].section.ranges;
>> +           for (j = 0; j < det_mon_num; j++) {
>> +               if (det_mon[j].type == DT) {
>> +                   preferred_timing = &det_mon[j].section.d_timings;
>>                     if (!ranges->max_clock) continue; /* zero is legal */
>>                     if (ranges->max_clock * 1000000 <
>preferred_timing->clock) {
>>                         xf86Msg(X_WARNING,
>
>This is buggy.  You've made it so det_mon_num is the total number of
>detailed sections in all blocks, including extensions, but not changed
>the size of the det_mon array.  So this for loop will walk off the end
>of the det_mon array.
>
>This (and the other places where you iterate over det_mon_num) ought to
>look more like:
>
>---
>@@ -52,11 +52,29 @@ static void get_detailed_timing_section(Uchar*, struct
> static Bool validate_version(int scrnIndex, struct edid_version *);
>
> static void
>+find_ranges_section(struct detailed_monitor_section *det, void *ranges)
>+{
>+    if (det->type == DS_RANGES && !clock)
>+       *ranges = &det->section.ranges;
>+}
>+
>+static void
>+find_max_detailed_clock(struct detailed_monitor_section *det, void *ret)
>+{
>+    int *clock = ret;
>+
>+    if (det->type == DT) {
>+       struct detailed_timings *t = &det->section.d_timings;
>+       *clock = max(*clock, t->clock);
>+    }
>+}
>+
>+static void
> handle_edid_quirks(xf86MonPtr m)
> {
>     int i, j;
>     struct detailed_timings *preferred_timing;
>-    struct monitor_ranges *ranges;
>+    struct monitor_ranges *ranges = NULL;
>
>     /*
>      * max_clock is only encoded in EDID in tens of MHz, so occasionally we
>@@ -64,25 +82,14 @@ handle_edid_quirks(xf86MonPtr m)
>      * similar.  Strictly we should refuse to round up too far, but let's
>      * see how well this works.
>      */
>-    for (i = 0; i < 4; i++) {
>-       if (m->det_mon[i].type == DS_RANGES) {
>-           ranges = &m->det_mon[i].section.ranges;
>-           for (j = 0; j < 4; j++) {
>-               if (m->det_mon[j].type == DT) {
>-                   preferred_timing = &m->det_mon[j].section.d_timings;
>-                   if (!ranges->max_clock) continue; /* zero is legal */
>-                   if (ranges->max_clock * 1000000 < preferred_timing->clock)
>{
>-                       xf86Msg(X_WARNING,
>-                           "EDID preferred timing clock %.2fMHz exceeds "
>-                           "claimed max %dMHz, fixing\n",
>-                           preferred_timing->clock / 1.0e6,
>-                           ranges->max_clock);
>-                       ranges->max_clock =
>-                           (preferred_timing->clock+999999)/1000000;
>-                       return;
>-                   }
>-               }
>-           }
>+    xf86ForEachDetailedBlock(m, find_ranges_section, &ranges);
>+    if (ranges && ranges->max_clock) {
>+       int clock = 0;
>+       xf86ForEachDetailedBlock(m, find_max_detailed_clock, &clock);
>+       if (clock && (ranges->max_clock * 1e6 < clock)) {
>+           xf86Msg(X_WARNING, "EDID timing clock %.2f exceeds claimed max "
>+                   "%dMHz, fixing\n", clock / 1.0e6, ranges->max_clock);
>+           ranges->max_clock = (clock+999999)/1e6;
>        }
>     }
>
>---
>
>Admittedly I'm fixing one or two other bugs in the process there, but
>you get the idea.  You really shouldn't ever be explicitly iterating
>from zero to det_mon_num.
>
>Also, calling edid_quirks() from xf86GetDetailTiming seems... confused.
>
>> +static void handle_detailed_hvsize(struct detailed_monitor_section
>*det_mon,
>> +                                   void *data)
>> +{
>> +    float timing_aspect;
>> +    struct parameter{
>> +        int real_hsize;
>> +        int real_vsize;
>> +        float target_aspect;
>> +    }*p = (struct parameter *)data;
>
>Ouch.  Please move these 'parameter' structs out to file scope from
>function scope, and with slightly more descriptive names.  The way
>you've done it here it's possible to get the structure definitions out
>of sync between the caller and callee.
>
>> +    det_mon_num = xf86GetDetailTiming(ext, ver, det_mon,
>CEA_EXT_DET_TIMING_NUM);
>
>This is the only caller.  Why pass CEA_EXT_DET_TIMING_NUM as a
>parameter?
>
>> -xf86MonitorSupportsReducedBlanking(xf86MonPtr DDC)
>> +xf86MonitorSupportsReducedBlanking(xf86MonPtr DDC, int scrnIndex)
>
>You don't need this, xf86MonPtr has scrnIndex as the first member
>already.
>
>> +void static handle_detailed_rblank(struct detailed_monitor_section
>*det_mon,
>> +                                   void *data)
>> +{
>> +    struct parameter{
>> +       xf86MonPtr  DDC;
>> +       int scrnIndex;
>> +        Bool ret;
>> +    }*p = (struct parameter *)data;
>> +
>> +    xf86DetTimingApplyQuirks(p->DDC, det_mon, p->scrnIndex, 1);
>> +    if (det_mon->type == DS_RANGES)
>> +        if (det_mon->section.ranges.supported_blanking & CVT_REDUCED)
>> +            p->ret = TRUE;
>> +}
>
>A bit overcomplicated.  DetTimingApplyQuirks won't ever change DS_RANGES
>descriptors at the moment.  You could just as easily do:
>
>    if (det_mon->type == DS_RANGES)
>        if (det_mon->section.ranges.supported_blanking & CVT_REDUCED)
>            *(Bool *)data = TRUE:
>
>and call it as
>
>    Bool ret;
>    xf86ForEachDetailedBlock(DDC, handle_detailed_rblank, &ret);
>    if (ret)
>        return TRUE;
>
>> +    struct parameter{
>> +        MonPtr *Monitor;
>> +       Bool *have_hsync;
>> +       Bool *have_vrefresh;
>> +       Bool *have_maxpixclock;
>> +        ddc_quirk_t *quirks;
>> +        int *scrnIndex;
>> +    }p = {&Monitor, &have_hsync, &have_vrefresh,
>> +        &have_maxpixclock, &quirks, &scrnIndex};
>
>Gasp.  Why all the pointers?  It's a bit clearer to change that to
>MonPtr, Bool, Bool, Bool... and have handle_detailed_monset modify it in
>place.
>
>- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_5_DetTiming_print_edid.patch
Type: application/octet-stream
Size: 9783 bytes
Desc: patch_5_DetTiming_print_edid.patch
URL: <http://lists.x.org/archives/xorg/attachments/20081112/ec3bbfc8/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_1_DetTiming_interpret.patch
Type: application/octet-stream
Size: 10825 bytes
Desc: patch_1_DetTiming_interpret.patch
URL: <http://lists.x.org/archives/xorg/attachments/20081112/ec3bbfc8/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_2_DetTiming_xf86EdidModes.patch
Type: application/octet-stream
Size: 9974 bytes
Desc: patch_2_DetTiming_xf86EdidModes.patch
URL: <http://lists.x.org/archives/xorg/attachments/20081112/ec3bbfc8/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_3_DetTiming_xf86Crtc.patch
Type: application/octet-stream
Size: 5459 bytes
Desc: patch_3_DetTiming_xf86Crtc.patch
URL: <http://lists.x.org/archives/xorg/attachments/20081112/ec3bbfc8/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_4_DetTiming_xf86Configure.patch
Type: application/octet-stream
Size: 2548 bytes
Desc: patch_4_DetTiming_xf86Configure.patch
URL: <http://lists.x.org/archives/xorg/attachments/20081112/ec3bbfc8/attachment-0004.obj>


More information about the xorg mailing list