[PATCH] to detail timing of EDID extension
Adam Jackson
ajax at nwnk.net
Tue Nov 11 07:53:54 PST 2008
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: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg/attachments/20081111/468989f1/attachment.pgp>
More information about the xorg
mailing list