[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