[PATCH 1/3] xfree86: Add a xf86MonitorFindHDMIBlock()

[walter harms]

Am 07.08.2013 18:04, schrieb Damien Lespiau:
> The HDMI CEA vendor specific block has some interesting information,
> such as the maximum TMDS dot clock.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> Tested-by: Cancan Feng <cancan.feng at intel.com>
> ---
>  hw/xfree86/ddc/interpret_edid.c | 88 +++++++++++++++++++++++++++++++++++++++++
>  hw/xfree86/ddc/xf86DDC.h        |  2 +
>  2 files changed, 90 insertions(+)
> 
> diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
> index e6b4d5b..10c1c3a 100644
> --- a/hw/xfree86/ddc/interpret_edid.c
> +++ b/hw/xfree86/ddc/interpret_edid.c
> @@ -332,6 +332,94 @@ xf86ForEachVideoBlock(xf86MonPtr mon, handle_video_fn fn, void *data)
>      }
>  }
>  
> +static void
> +cea_db_offsets(Uchar *cea, int *start, int *end)
> +{
> +    /* Data block offset in CEA extension block */
> +    *start = CEA_EXT_MIN_DATA_OFFSET;
> +    *end = cea[2];
> +    if (*end == 0 || *end > CEA_EXT_MAX_DATA_OFFSET)
> +        *end = CEA_EXT_MAX_DATA_OFFSET;
> +    if (*end < CEA_EXT_MIN_DATA_OFFSET)
> +        *end = CEA_EXT_MIN_DATA_OFFSET;
> +}

The name of the function does not match what it does. the rule of least surprise
is helpful , please find a better name.


> +static int
> +cea_db_len(Uchar *db)
> +{
> +    return db[0] & 0x1f;
> +}

there seems no need for microoptimisation. simply:

cea_db_len(Uchar db)
{
	return db & 0x1f
}

> +static int
> +cea_db_tag(Uchar *db)
> +{
> +    return db[0] >> 5;
> +}

see above

> +typedef void (*handle_cea_db_fn) (Uchar *, void *);
> +
> +static void
> +cea_for_each_db(xf86MonPtr mon, handle_cea_db_fn fn, void *data)
> +{
> +    int i;
> +
> +    if (!mon)
> +        return;
> +
> +    if (!(mon->flags & EDID_COMPLETE_RAWDATA))
> +        return;
> +
> +    if (!mon->no_sections)
> +        return;
> +
> +    if (!mon->rawData)
> +        return;
> +
> +    for (i = 0; i < mon->no_sections; i++) {
> +        int start, end, offset;
> +        Uchar *ext;
> +
> +        ext = mon->rawData + EDID1_LEN * (i + 1);
> +        if (ext[EXT_TAG] != CEA_EXT)
> +            continue;
> +
> +        cea_db_offsets(ext, &start, &end);
> +        for (offset = start;
> +             offset < end && offset + cea_db_len(&(ext)[(i)]) < end;
	this guess you can simplify that:
  	cea_db_len is always >=0
	cea_db_len(ext[i]) < end-offset
	

> +             offset += cea_db_len(&ext[offset]) + 1)
> +                fn(&ext[offset], data);
> +    }
> +}


	


> +struct find_hdmi_block_data {
> +    struct cea_data_block *hdmi;
> +};
> +
> +static void find_hdmi_block(Uchar *db, void *data)
> +{
> +    struct find_hdmi_block_data *result = data;
> +    int oui;
> +
> +    if (cea_db_tag(db) != CEA_VENDOR_BLK)
> +        return;
> +
> +    if (cea_db_len(db) < 5)
> +        return;
> +
> +    oui = (db[3] << 16) + (db[2] << 8) + db[1];
> +    if (oui == IEEE_ID_HDMI)
> +        result->hdmi = (struct cea_data_block *)db;
> +}
> +
> +struct cea_data_block *xf86MonitorFindHDMIBlock(xf86MonPtr mon)
> +{
> +    struct find_hdmi_block_data result = { NULL };
> +
> +    cea_for_each_db(mon, find_hdmi_block, &result);
> +
> +    return result.hdmi;
> +}
> +
>  xf86MonPtr
>  xf86InterpretEEDID(int scrnIndex, Uchar * block)
>  {
> diff --git a/hw/xfree86/ddc/xf86DDC.h b/hw/xfree86/ddc/xf86DDC.h
> index bdc7648..de8e718 100644
> --- a/hw/xfree86/ddc/xf86DDC.h
> +++ b/hw/xfree86/ddc/xf86DDC.h
> @@ -98,4 +98,6 @@ typedef void (*handle_video_fn) (struct cea_video_block *, void *);
>  
>  void xf86ForEachVideoBlock(xf86MonPtr, handle_video_fn, void *);
>  
> +struct cea_data_block *xf86MonitorFindHDMIBlock(xf86MonPtr mon);
> +
>  #endif

just my 2 cents
re,
 wh