[PATCH 4/6] handle detailed timing operation in xf86Crtc.c
Ma Ling
ling.ma at intel.com
Mon Dec 29 00:56:06 PST 2008
On Thu, 2008-12-25 at 10:46 +0800, Wu, Fengguang wrote:
> On Sat, Dec 20, 2008 at 04:41:25AM +0200, Ma, Ling wrote:
> > handle detailed timing operation,e.g. set up Monitor from detailed timing block
> > from EDID and cea ext
> >
> > ---
> > hw/xfree86/modes/xf86Crtc.c | 125 ++++++++++++++++++++++++++++---------------
> > 1 files changed, 82 insertions(+), 43 deletions(-)
> >
> > diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
> > index 309eb99..3eb7cb6 100644
> > --- a/hw/xfree86/modes/xf86Crtc.c
> > +++ b/hw/xfree86/modes/xf86Crtc.c
> > @@ -1530,6 +1530,40 @@ GuessRangeFromModes(MonPtr mon, DisplayModePtr mode)
> > mon->vrefresh[0].lo = 58.0;
> > }
> >
> > +struct det_monrec_parameter {
> > + MonRec *mon_rec;
> > + int *max_clock;
> > + int *sync_source;
> > +};
> > +
> > +static void handle_detailed_monrec(struct detailed_monitor_section *det_mon,
> > + void *data)
> > +{
> > + enum { sync_config, sync_edid, sync_default };
>
> Convert them to SYS_CONFIG, ... in new code?
in new code they have the same type
>
> > + struct det_monrec_parameter *p;
> > + p = (struct det_monrec_parameter *)data;
>
> The above two lines can be merged.
retain to avoid too long line
>
> > +
> > + if (det_mon->type == DS_RANGES) {
> > + struct monitor_ranges *ranges = &det_mon->section.ranges;
>
> indentation and whitespace
fixed
>
> > + if (p->mon_rec->nHsync == 0 && ranges->max_h) {
> > + p->mon_rec->hsync[p->mon_rec->nHsync].lo = ranges->min_h;
> > + p->mon_rec->hsync[p->mon_rec->nHsync].hi = ranges->max_h;
> > + p->mon_rec->nHsync++;
> > + if (*p->sync_source == sync_default)
> > + *p->sync_source = sync_edid;
> > + }
> > + if (p->mon_rec->nVrefresh == 0 && ranges->max_v) {
> > + p->mon_rec->vrefresh[p->mon_rec->nVrefresh].lo = ranges->min_v;
> > + p->mon_rec->vrefresh[p->mon_rec->nVrefresh].hi = ranges->max_v;
> > + p->mon_rec->nVrefresh++;
> > + if (*p->sync_source == sync_default)
> > + *p->sync_source = sync_edid;
> > + }
> > + if (ranges->max_clock * 1000 > *p->max_clock)
> > + *p->max_clock = ranges->max_clock * 1000;
> > + }
> > +}
> > +
> > void
> > xf86ProbeOutputModes (ScrnInfoPtr scrn, int maxX, int maxY)
> > {
> > @@ -1610,40 +1644,20 @@ xf86ProbeOutputModes (ScrnInfoPtr scrn, int maxX, int maxY)
> >
> > if (edid_monitor)
> > {
> > - int i;
> > - Bool set_hsync = mon_rec.nHsync == 0;
> > - Bool set_vrefresh = mon_rec.nVrefresh == 0;
> > - struct disp_features *features = &edid_monitor->features;
> > + struct det_monrec_parameter p;
> > + struct disp_features *features = &edid_monitor->features;
>
> Inconsistent indentation.
fixed
>
> >
> > /* if display is not continuous-frequency, don't add default modes */
> > if (!GTF_SUPPORTED(features->msc))
> > add_default_modes = FALSE;
> > -
> > - for (i = 0; i < sizeof (edid_monitor->det_mon) / sizeof (edid_monitor->det_mon[0]); i++)
> > - {
> > - if (edid_monitor->det_mon[i].type == DS_RANGES)
> > - {
> > - struct monitor_ranges *ranges = &edid_monitor->det_mon[i].section.ranges;
> > - if (set_hsync && ranges->max_h)
> > - {
> > - mon_rec.hsync[mon_rec.nHsync].lo = ranges->min_h;
> > - mon_rec.hsync[mon_rec.nHsync].hi = ranges->max_h;
> > - mon_rec.nHsync++;
> > - if (sync_source == sync_default)
> > - sync_source = sync_edid;
> > - }
> > - if (set_vrefresh && ranges->max_v)
> > - {
> > - mon_rec.vrefresh[mon_rec.nVrefresh].lo = ranges->min_v;
> > - mon_rec.vrefresh[mon_rec.nVrefresh].hi = ranges->max_v;
> > - mon_rec.nVrefresh++;
> > - if (sync_source == sync_default)
> > - sync_source = sync_edid;
> > - }
> > - if (ranges->max_clock * 1000 > max_clock)
> > - max_clock = ranges->max_clock * 1000;
> > - }
> > - }
> > +
> > + p.mon_rec = &mon_rec;
> > + p.max_clock = &max_clock;
> > + p.sync_source = (int *)&sync_source;
>
> Why not define the same type for p.sync_source and sync_source?
in new code they have the same type
>
> > + xf86ForEachDetailedBlock(edid_monitor,
> > + handle_detailed_monrec,
> > + &p);
> > }
> >
> > if (xf86GetOptValFreq (output->options, OPTION_MIN_CLOCK,
> > @@ -2888,6 +2902,36 @@ xf86OutputSetEDIDProperty (xf86OutputPtr output, void *data, int data_len)
> >
> > #endif
> >
> > +/* Pull out a phyiscal size from a detailed timing if available. */
> > +struct det_phySize_parameter {
> > + xf86OutputPtr output;
> > + ddc_quirk_t quirks;
> > + Bool ret;
> > +};
> > +
> > +void static handle_detailed_physical_size(struct detailed_monitor_section
>
> static void
fixed
>
> > + *det_mon, void *data)
> > +{
> > + struct det_phySize_parameter *p;
> > + p = (struct det_phySize_parameter *)data;
> > +
> > + if (p->ret == TRUE )
> > + return ;
>
> This trick is awkward. I'd rather not converting the original code to
> xf86ForEachDetailedBlock at all.
>
> > +
> > + xf86DetTimingApplyQuirks(det_mon, p->quirks,
> > + p->output->MonInfo->features.hsize,
> > + p->output->MonInfo->features.vsize);
>
> Is this a newly added feature?
In fact it is not new feature, original code will do the work before it
call this function, now because all detailed timing block is dynamically
fetched from EDID, we have to apply quirk when we really need detailed
timing block.
> > +
>
> trailing whitespace
fixed
>
> > + if (det_mon->type == DT &&
> > + det_mon->section.d_timings.h_size != 0 &&
> > + det_mon->section.d_timings.v_size != 0) {
> > +
> > + p->output->mm_width = det_mon->section.d_timings.h_size;
> > + p->output->mm_height = det_mon->section.d_timings.v_size;
> > + p->ret = TRUE;
> > + }
> > +}
> > +
> > /**
> > * Set the EDID information for the specified output
> > */
> > @@ -2931,19 +2975,14 @@ xf86OutputSetEDID (xf86OutputPtr output, xf86MonPtr edid_mon)
> > xf86OutputSetEDIDProperty (output, edid_mon ? edid_mon->rawData : NULL, size);
> > #endif
> >
> > - if (edid_mon)
> > - {
> > - /* Pull out a phyiscal size from a detailed timing if available. */
> > - for (i = 0; i < 4; i++) {
> > - if (edid_mon->det_mon[i].type == DT &&
> > - edid_mon->det_mon[i].section.d_timings.h_size != 0 &&
> > - edid_mon->det_mon[i].section.d_timings.v_size != 0)
> > - {
> > - output->mm_width = edid_mon->det_mon[i].section.d_timings.h_size;
> > - output->mm_height = edid_mon->det_mon[i].section.d_timings.v_size;
> > - break;
> > - }
> > - }
> > + if (edid_mon) {
> > +
> > + struct det_phySize_parameter p;
> > + p.output = output;
> > + p.quirks = xf86DDCDetectQuirks(scrn->scrnIndex,edid_mon, FALSE);
> > + p.ret = FALSE;
> > + xf86ForEachDetailedBlock(edid_mon,
> > + handle_detailed_physical_size, &p);
> >
> > /* if no mm size is available from a detailed timing, check the max size field */
> > if ((!output->mm_width || !output->mm_height) &&
> > --
> > 1.5.4.4
> >
> >
> >
More information about the xorg
mailing list