FW: [PATCH] to EDID extension

[Adam Jackson]

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: 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/20081106/28c3c36e/attachment.pgp>