[PATCH] to detail timing of EDID extension

[Ma, Ling]

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.

Any comments are welcome!
Thanks
Ma Ling 


>-----Original Message-----
>From: Adam Jackson [mailto:ajax at nwnk.net]
>Sent: 2008年11月6日 23:29
>To: Ma, Ling
>Cc: xorg at lists.freedesktop.org
>Subject: Re: FW: [PATCH] to EDID extension
>
>On Thu, 2008-11-06 at 20:42 +0800, Ma, Ling wrote:
>> Hi ALL
>>
>> I updated EDID extension Patch, it includes parsing detail timing
>> descriptor (DTDs) and short video descriptors(SVDs), then inserting
>> them into mode list, meanwhile common functions-xf86OutputGetELD and
>> xf86OutputGetMaxTmdsClock, so different video drivers all can call
>> them to generate ELD to sent to audio and MAX-TMDS-CLOCK to validate
>> TMDS clock by self.
>
>Thanks for looking into this.
>
>> +#define VENDOR_OFFSET(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
>
>I'm actually kind of shocked we don't have offsetof() already defined.
>
>> +struct cea_audio_blk {
>> +    Uchar descs[3];
>> +}__attribute__ ((packed));
>> +struct hdmi {
>> +  Uchar  Support_flags;
>> +  Uchar  Max_TMDS_Clock;
>> +  Uchar  Latency_Present;
>> +  Uchar  Video_Latency;
>> +  Uchar  Audio_Latency;
>> +  Uchar  Interlaced_Video_Latency;
>> +  Uchar  Interlaced_Audio_Latency;
>> +
>> +}__attribute__ ((packed));
>
>The X server does not assume gcc, don't use __attribute__ directly.  And
>as a rule, there's no good reason to use ((packed)).  Also, camel case
>or underscore separation, not both.
>
>> @@ -541,7 +679,8 @@ typedef struct {
>>    struct disp_features features;
>>    struct established_timings timings1;
>>    struct std_timings timings2[8];
>> -  struct detailed_monitor_section det_mon[4];
>> +  struct detailed_monitor_section det_mon[10];
>> +  int det_mon_num;
>>    unsigned long flags;
>>    int no_sections;
>>    Uchar *rawData;
>
>I would strongly prefer not to add additional extension information to
>xf86Monitor.  It's an ABI structure, so if we took this patch as-is,
>we'd have to change the ABI again when we add DI-EXT or VTB-EXT support,
>both of which also allow the monitor to define additional timings.  Also
>there's nothing prohibiting more than one CEA extension block on a
>monitor; the patch as-is would only look at the first, which is not
>correct.
>
>Ideally drivers would never inspect xf86Monitor directly, in fact.  I
>much prefer an xf86MonitorIsHDMI() kind of interface where we just walk
>the EDID block directly in the server to answer these kinds of
>questions.
>
>Internally to the server we do walk xf86Monitor directly to compute mode
>lists and EDID quirks and such.  I'd prefer to see that done more like:
>
>typedef void (*detailed_lambda)(struct detailed_monitor_section *det, void
>*closure);
>
>void xf86ForEachDetailedBlock(xf86MonPtr *mon, detailed_lambda *l, void
>*closure)
>{
>    int i;
>
>    for (i = 0; i < DET_TIMINGS; i++)
>        l(&mon->det_mon[i], closure);
>
>    /* for each extension block */
>        /* if it's a type that has detailed timings */
>            /* for each detailed block in the the extension block */
>                l(&det_block, closure);
>}
>
>> +    if ((EDID_CEA_EXTENSION_FLG & output->MonInfo->flags) == 0) {
>> +        goto end;
>> +    }
>> +
>> +    type.body_type = CEA_EXT;
>> +    type.data_type = CEA_VENDOR_BLK;
>> +    blk = (struct cea_data_blk *)xf86DDCGetCEA( output->MonInfo,&type);
>> +
>> +    if (blk != NULL) {
>> +        if (VENDOR_OFFSET(struct cea_vendor_blk, hdmi.Max_TMDS_Clock) <=
>> +           blk->len) {
>> +               ret = blk->u.vendor.hdmi.Max_TMDS_Clock *
>HDMI_MAX_TMDS_UNIT;
>> +        }
>> +    }
>> +end:
>> +    return ret;
>
>Minor style nit here, try not to use the 'goto out' form if you're not
>doing significant unwinding at the end.
>
>> +    eld = (struct eld_data_fixed_fields *)xcalloc(1, eld_len);
>
>Don't cast the return value of malloc or calloc.  It doesn't _do_
>anything besides make it harder to read.
>
>Also, even with the spec in front of me, I have no idea what "ELD" is
>supposed to mean.  It looks like it's a combination of the audio and
>speaker allocation blocks?
>
>- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cea_externsion_detail_timing.patch-0.1
Type: application/octet-stream
Size: 39472 bytes
Desc: cea_externsion_detail_timing.patch-0.1
URL: <http://lists.x.org/archives/xorg/attachments/20081111/9756a07c/attachment.obj>