[PATCH 2/6] implement unified interface and detailed timing operation in interpret_edid.c
Ma Ling
ling.ma at intel.com
Mon Dec 29 00:32:23 PST 2008
On Wed, 2008-12-24 at 21:22 +0800, Wu, Fengguang wrote:
> On Sat, Dec 20, 2008 at 04:28:08AM +0200, Ma, Ling wrote:
> > implement unified interface and detailed timing operation
>
> It looks like this and some following patches mixed code refectoring
> with new features. Could you split them out, create one patch for one
> logical task and afford more descriptions for each logical changes?
>
> Nitpicks follow...
fixed, In my latest version I extracted unified interface as dependent
one - [PATCH 3/7]
>
> > ---
> > hw/xfree86/ddc/interpret_edid.c | 312 ++++++++++++++++++++++++++-------------
> > 1 files changed, 208 insertions(+), 104 deletions(-)
> >
> > diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
> > index c0e3df9..de3aa0c 100644
> > --- a/hw/xfree86/ddc/interpret_edid.c
> > +++ b/hw/xfree86/ddc/interpret_edid.c
> > @@ -41,8 +41,11 @@ static void get_display_section(Uchar*, struct disp_features *,
> > static void get_established_timing_section(Uchar*, struct established_timings *);
> > static void get_std_timing_section(Uchar*, struct std_timings *,
> > struct edid_version *);
> > +
> > +static void fetch_detailed_block(Uchar *c, struct edid_version *ver,
> > + struct detailed_monitor_section *det_mon);
> > static void get_dt_md_section(Uchar *, struct edid_version *,
> > - struct detailed_monitor_section *det_mon);
> > + struct detailed_monitor_section *det_mon);
> > static void copy_string(Uchar *, Uchar *);
> > static void get_dst_timing_section(Uchar *, struct std_timings *,
> > struct edid_version *);
> > @@ -52,11 +55,27 @@ static void get_detailed_timing_section(Uchar*, struct detailed_timings *);
> > 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 && det->section.ranges.max_clock)
> > + *(struct monitor_ranges **)ranges = &det->section.ranges;
> > +}
> > +
> > +static void
> > +find_max_detailed_clock(struct detailed_monitor_section *det, void *ret)
> > +{
> > + int *clock = ret;
>
> This variable could be eliminated.
fixed.
>
> > +
> > + 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,28 +83,116 @@ 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;
> > + }
> > + }
>
> It looks like the above lines change behavior(s) slightly. If so, can
> you describe it in the changelog?
no difference, the original intention is to find range descriptor.
>
> > +}
> > +
> > +static int GetCEADetailTiming(Uchar *blk, xf86MonPtr mon,
>
> I'd prefer this_name_style for static functions.
>
> > + struct detailed_monitor_section *det_mon)
> > +
>
> Trailing whitespace.
fixed
>
> > +{
> > + int num;
> > + int dt_offset = ((struct cea_ext_body *)blk)->dt_offset;
> > +
> > + num = 0;
>
> Inconsistent styles in the above lines.
fixed
>
> > + if (dt_offset < CEA_EXT_MIN_DATA_OFFSET)
> > + return num;
>
> return 0 is more straightforward and less error prone.
num will make code consistent :)
>
> > + while (dt_offset < (CEA_EXT_MAX_DATA_OFFSET - DET_TIMING_INFO_LEN) &&
> > + num < CEA_EXT_DET_TIMING_NUM) {
> > +
> > + fetch_detailed_block(blk + dt_offset, &mon->ver, det_mon + num);
> > +
> > + num = num + 1 ;
>
> for(;;) could be better?
fixed
>
> > + _NEXT_DT_MD_SECTION(dt_offset);
> > + }
> > +
> > + return num;
>
> Return a more obvious CEA_EXT_DET_TIMING_NUM?
in fact we only need to return real detailed timing number.
>
> > +}
> > +
> > +
> > +static void HandleCEADetailedBlock(Uchar *ext, xf86MonPtr mon,
> > + handle_detailed_fn fn,
> > + void *data)
> > +{
> > + int i;
> > + struct detailed_monitor_section det_mon[CEA_EXT_DET_TIMING_NUM];
> > + int det_mon_num;
> > +
> > + det_mon_num = GetCEADetailTiming(ext, mon, det_mon);
> > +
> > + for (i = 0; i < det_mon_num; i = i + 1)
> > + fn(det_mon + i , data);
> ~ redundant space
>
fixed
> > +}
> > +
> > +void xf86ForEachDetailedBlock(xf86MonPtr mon,
> > + handle_detailed_fn fn,
> > + void *data)
> > +{
> > + int i;
> > + Uchar *ext;
> > +
> > + if (mon == NULL)
> > + return;
> > +
> > + for (i = 0; i < DET_TIMINGS; i++)
> > + fn(mon->det_mon + i, data);
> > +
> > + for (i = 0; i < mon->no_sections; i = i + 1) {
>
> i++?
fixed
>
> > + ext = &mon->rawData[EDID1_LEN + i * EDID1_LEN];
>
> ext = mon->rawData + EDID1_LEN * (i + 1);
>
> > + switch (ext[EXT_TAG]){
> > + case CEA_EXT:
> > + HandleCEADetailedBlock(ext, mon, fn, data);
> > + break;
> > + case VTB_EXT:
> > + case DI_EXT:
> > + case LS_EXT:
> > + case MI_EXT:
> > + break;
> > + }
> > }
> > +}
> > +
> > +struct det_hv_perameter {
>
> spell => parameter
fixed
>
> > + int real_hsize;
> > + int real_vsize;
> > + float target_aspect;
> > +};
> > +
> > +static void handle_detailed_hvsize(struct detailed_monitor_section *det_mon,
> > + void *data)
> > +{
> > + struct det_hv_perameter *p = (struct det_hv_perameter *)data;
> > + float timing_aspect;
> >
> > + if (det_mon->type == DT) {
> > + struct detailed_timings *timing;
> > + timing = &det_mon->section.d_timings;
>
> Merge the above two lines?
retaining looks like better.
>
> > +
> > + if (!timing->v_size)
> > + return;
> > +
> > + timing_aspect = (float)timing->h_size / (float)timing->v_size;
>
> One float cast is enough.
fixed
>
> > + if (fabs(1 - (timing_aspect / p->target_aspect)) < 0.05) {
> > + p->real_hsize = max(p->real_hsize, timing->h_size);
> > + p->real_vsize = max(p->real_vsize, timing->v_size);
> > + }
> > + }
> > +}
> > +
> > +static void encode_aspect_ratio(xf86MonPtr m)
> > +{
> > /*
> > * some monitors encode the aspect ratio instead of the physical size.
> > * try to find the largest detailed timing that matches that aspect
> > @@ -95,36 +202,24 @@ handle_edid_quirks(xf86MonPtr m)
> > (m->features.hsize == 16 && m->features.vsize == 10) ||
> > (m->features.hsize == 4 && m->features.vsize == 3) ||
> > (m->features.hsize == 5 && m->features.vsize == 4)) {
> > - int real_hsize = 0, real_vsize = 0;
> > - float target_aspect, timing_aspect;
> > -
> > - target_aspect = (float)m->features.hsize / (float)m->features.vsize;
> > - for (i = 0; i < 4; i++) {
> > - if (m->det_mon[i].type == DT) {
> > - struct detailed_timings *timing;
> > - timing = &m->det_mon[i].section.d_timings;
> > -
> > - if (!timing->v_size)
> > - continue;
> > -
> > - timing_aspect = (float)timing->h_size / (float)timing->v_size;
> > - if (fabs(1 - (timing_aspect / target_aspect)) < 0.05) {
> > - real_hsize = max(real_hsize, timing->h_size);
> > - real_vsize = max(real_vsize, timing->v_size);
> > - }
> > - }
> > - }
> > -
> > - if (!real_hsize || !real_vsize) {
> > +
> > + struct det_hv_perameter p;
> > + p.real_hsize = 0;
> > + p.real_vsize = 0;
> > + p.target_aspect = (float)m->features.hsize /(float)m->features.vsize;
>
> ditto
fixed
>
> > +
> > + xf86ForEachDetailedBlock(m, handle_detailed_hvsize, &p);
> > +
> > + if (!p.real_hsize || !p.real_vsize) {
> > m->features.hsize = m->features.vsize = 0;
> > - } else if ((m->features.hsize * 10 == real_hsize) &&
> > - (m->features.vsize * 10 == real_vsize)) {
> > + } else if ((m->features.hsize * 10 == p.real_hsize) &&
> > + (m->features.vsize * 10 == p.real_vsize)) {
> > /* exact match is just unlikely, should do a better check though */
> > m->features.hsize = m->features.vsize = 0;
> > } else {
> > /* convert mm to cm */
> > - m->features.hsize = (real_hsize + 5) / 10;
> > - m->features.vsize = (real_vsize + 5) / 10;
> > + m->features.hsize = (p.real_hsize + 5) / 10;
> > + m->features.vsize = (p.real_vsize + 5) / 10;
> > }
> >
> > xf86Msg(X_INFO, "Quirked EDID physical size to %dx%d cm\n",
> > @@ -151,10 +246,13 @@ xf86InterpretEDID(int scrnIndex, Uchar *block)
> > &m->timings1);
> > get_std_timing_section(SECTION(STD_TIMING_SECTION,block),m->timings2,
> > &m->ver);
> > - get_dt_md_section(SECTION(DET_TIMING_SECTION,block),&m->ver, m->det_mon);
> > + get_dt_md_section(SECTION(DET_TIMING_SECTION,block), &m->ver,
> > + m->det_mon);
> > +
> > m->no_sections = (int)*(char *)SECTION(NO_EDID,block);
> >
> > handle_edid_quirks(m);
> > + encode_aspect_ratio(m);
> >
> > return (m);
> >
> > @@ -285,65 +383,71 @@ get_std_timing_section(Uchar *c, struct std_timings *r,
> > }
> >
> > static void
> > +fetch_detailed_block(Uchar *c, struct edid_version *ver,
> > + struct detailed_monitor_section *det_mon)
> > +{
> > + if (ver->version == 1 && ver->revision >= 1 && IS_MONITOR_DESC) {
> > + switch (MONITOR_DESC_TYPE) {
> > + case SERIAL_NUMBER:
> > + det_mon->type = DS_SERIAL;
> > + copy_string(c,det_mon->section.serial);
>
> Whitespace after the comma for new code?
oh, I forgot, it will be done in next version.
>
> > + break;
> > + case ASCII_STR:
> > + det_mon->type = DS_ASCII_STR;
> > + copy_string(c,det_mon->section.ascii_data);
> > + break;
> > + case MONITOR_RANGES:
> > + det_mon->type = DS_RANGES;
> > + get_monitor_ranges(c,&det_mon->section.ranges);
> > + break;
> > + case MONITOR_NAME:
> > + det_mon->type = DS_NAME;
> > + copy_string(c,det_mon->section.name);
> > + break;
> > + case ADD_COLOR_POINT:
> > + det_mon->type = DS_WHITE_P;
> > + get_whitepoint_section(c,det_mon->section.wp);
> > + break;
> > + case ADD_STD_TIMINGS:
> > + det_mon->type = DS_STD_TIMINGS;
> > + get_dst_timing_section(c,det_mon->section.std_t, ver);
> > + break;
> > + case COLOR_MANAGEMENT_DATA:
> > + det_mon->type = DS_CMD;
> > + break;
> > + case CVT_3BYTE_DATA:
> > + det_mon->type = DS_CVT;
> > + get_cvt_timing_section(c, det_mon->section.cvt);
> > + break;
> > + case ADD_EST_TIMINGS:
> > + det_mon->type = DS_EST_III;
> > + break;
> > + case ADD_DUMMY:
> > + det_mon->type = DS_DUMMY;
> > + break;
> > + default:
> > + det_mon->type = DS_UNKOWN;
> > + break;
> > + }
> > + if (c[3] <= 0x0F) {
> > + det_mon->type = DS_VENDOR + c[3];
> > + }
> > + } else {
> > + det_mon->type = DT;
> > + get_detailed_timing_section(c, &det_mon->section.d_timings);
> > + }
> > +}
> > +
> > +static void
> > get_dt_md_section(Uchar *c, struct edid_version *ver,
> > struct detailed_monitor_section *det_mon)
> > {
> > - int i;
> > + int i;
> >
> > - for (i=0;i<DET_TIMINGS;i++) {
> > - if (ver->version == 1 && ver->revision >= 1 && IS_MONITOR_DESC) {
> > -
> > - switch (MONITOR_DESC_TYPE) {
> > - case SERIAL_NUMBER:
> > - det_mon[i].type = DS_SERIAL;
> > - copy_string(c,det_mon[i].section.serial);
> > - break;
> > - case ASCII_STR:
> > - det_mon[i].type = DS_ASCII_STR;
> > - copy_string(c,det_mon[i].section.ascii_data);
> > - break;
> > - case MONITOR_RANGES:
> > - det_mon[i].type = DS_RANGES;
> > - get_monitor_ranges(c,&det_mon[i].section.ranges);
> > - break;
> > - case MONITOR_NAME:
> > - det_mon[i].type = DS_NAME;
> > - copy_string(c,det_mon[i].section.name);
> > - break;
> > - case ADD_COLOR_POINT:
> > - det_mon[i].type = DS_WHITE_P;
> > - get_whitepoint_section(c,det_mon[i].section.wp);
> > - break;
> > - case ADD_STD_TIMINGS:
> > - det_mon[i].type = DS_STD_TIMINGS;
> > - get_dst_timing_section(c,det_mon[i].section.std_t, ver);
> > - break;
> > - case COLOR_MANAGEMENT_DATA:
> > - det_mon[i].type = DS_CMD;
> > - break;
> > - case CVT_3BYTE_DATA:
> > - det_mon[i].type = DS_CVT;
> > - get_cvt_timing_section(c, det_mon[i].section.cvt);
> > - break;
> > - case ADD_EST_TIMINGS:
> > - det_mon[i].type = DS_EST_III;
> > - break;
> > - case ADD_DUMMY:
> > - det_mon[i].type = DS_DUMMY;
> > - break;
> > - default:
> > - det_mon[i].type = DS_UNKOWN;
> > - break;
> > - }
> > - if (c[3] <= 0x0F) {
> > - det_mon[i].type = DS_VENDOR + c[3];
> > - }
> > - } else {
> > - det_mon[i].type = DT;
> > - get_detailed_timing_section(c,&det_mon[i].section.d_timings);
> > + for (i=0;i<DET_TIMINGS;i++) {
>
> for (i = 0; i < DET_TIMINGS; i++) {
>
> Please add spaces to make the new code more readable.
fixed
>
> > + fetch_detailed_block(c, ver, det_mon + i);
> > + NEXT_DT_MD_SECTION;
> > }
> > - NEXT_DT_MD_SECTION;
> > - }
> > }
> >
> > static void
> > --
> > 1.5.4.4
> >
> >
> >
More information about the xorg
mailing list