[RFC][PATCH] drm: add helper extracting SADs from EDID

[RFC][PATCH] drm: add helper extracting SADs from EDID

[Rafał Miłecki]

Some devices (ATI/AMD cards) don't want passing ELD struct to the
hardware but just require filling specific registers and then
hardware/firmware does the rest. In such a cases we need to read info
from SAD blocks and put them in the correct registers.
---
 drivers/gpu/drm/drm_edid.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |   24 +++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e2acfdb..7c9e799 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2511,6 +2511,61 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
 EXPORT_SYMBOL(drm_edid_to_eld);
 
 /**
+ * drm_edid_to_sad - extracts SADs from EDID
+ * @edid: EDID to parse
+ *
+ * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.
+ */
+struct cea_sad *drm_edid_to_sad(struct edid *edid)
+{
+	struct cea_sad *sads = NULL;
+	int i, start, end, dbl;
+	u8 *db, *cea;
+
+	cea = drm_find_cea_extension(edid);
+	if (!cea) {
+		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
+		return NULL;
+	}
+
+	if (cea_revision(cea) < 3) {
+		DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
+		return NULL;
+	}
+
+	if (cea_db_offsets(cea, &start, &end)) {
+		DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
+		return NULL;
+	}
+
+	for_each_cea_db(cea, i, start, end) {
+		db = &cea[i];
+		dbl = cea_db_payload_len(db);
+
+		if (cea_db_tag(db) == AUDIO_BLOCK) {
+			int count = dbl / 3; /* SAD is 3B */
+			int j;
+
+			sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);
+			if (!sads)
+				return NULL;
+
+			for (j = 0; j < count; j++) {
+				u8 *sad = &db[1 + j * 3];
+
+				sads[j].format = (sad[0] & 0x78) >> 3;
+				sads[j].channels = sad[0] & 0x7;
+				sads[j].freq = sad[1] & 0x7F;
+				sads[j].byte2 = sad[2];
+			}
+		}
+	}
+
+	return sads;
+}
+EXPORT_SYMBOL(drm_edid_to_sad);
+
+/**
  * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
  * @connector: connector associated with the HDMI/DP sink
  * @mode: the display mode
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 5da1b4a..b36d052 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -244,12 +244,36 @@ struct edid {
 
 #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
 
+#define SAD_FORMAT_LPCM			0x01
+#define SAD_FORMAT_AC3			0x02
+#define SAD_FORMAT_MPEG1		0x03
+#define SAD_FORMAT_MP3			0x04
+#define SAD_FORMAT_MPEG2		0x05
+#define SAD_FORMAT_AAC			0x06
+#define SAD_FORMAT_DTS			0x07
+#define SAD_FORMAT_ATRAC		0x08
+#define SAD_FORMAT_ONE_BIT_AUDIO	0x09
+#define SAD_FORMAT_DOLBY_DIGITAL	0x0a
+#define SAD_FORMAT_DTS_HD		0x0b
+#define SAD_FORMAT_MAT_MLP		0x0c
+#define SAD_FORMAT_DST			0x0d
+#define SAD_FORMAT_WMA_PRO		0x0e
+
+/* Short Audio Descriptor */
+struct cea_sad {
+	u8 format;
+	u8 channels; /* max number of channels - 1 */
+	u8 freq;
+	u8 byte2; /* meaning depends on format */
+};
+
 struct drm_encoder;
 struct drm_connector;
 struct drm_display_mode;
 struct hdmi_avi_infoframe;
 
 void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid);
+struct cea_sad *drm_edid_to_sad(struct edid *edid);
 int drm_av_sync_delay(struct drm_connector *connector,
 		      struct drm_display_mode *mode);
 struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
-- 
1.7.10.4

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Boszormenyi Zoltan]

Hi,

will there be a followup patch where this function is actually used?

Best regards,
Zoltán Böszörményi

2013-04-07 12:52 keltezéssel, Rafał Miłecki írta:
> Some devices (ATI/AMD cards) don't want passing ELD struct to the
> hardware but just require filling specific registers and then
> hardware/firmware does the rest. In such a cases we need to read info
> from SAD blocks and put them in the correct registers.
> ---
>   drivers/gpu/drm/drm_edid.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
>   include/drm/drm_edid.h     |   24 +++++++++++++++++++
>   2 files changed, 79 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e2acfdb..7c9e799 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2511,6 +2511,61 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>   EXPORT_SYMBOL(drm_edid_to_eld);
>   
>   /**
> + * drm_edid_to_sad - extracts SADs from EDID
> + * @edid: EDID to parse
> + *
> + * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.
> + */
> +struct cea_sad *drm_edid_to_sad(struct edid *edid)
> +{
> +	struct cea_sad *sads = NULL;
> +	int i, start, end, dbl;
> +	u8 *db, *cea;
> +
> +	cea = drm_find_cea_extension(edid);
> +	if (!cea) {
> +		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> +		return NULL;
> +	}
> +
> +	if (cea_revision(cea) < 3) {
> +		DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
> +		return NULL;
> +	}
> +
> +	if (cea_db_offsets(cea, &start, &end)) {
> +		DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
> +		return NULL;
> +	}
> +
> +	for_each_cea_db(cea, i, start, end) {
> +		db = &cea[i];
> +		dbl = cea_db_payload_len(db);
> +
> +		if (cea_db_tag(db) == AUDIO_BLOCK) {
> +			int count = dbl / 3; /* SAD is 3B */
> +			int j;
> +
> +			sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);
> +			if (!sads)
> +				return NULL;
> +
> +			for (j = 0; j < count; j++) {
> +				u8 *sad = &db[1 + j * 3];
> +
> +				sads[j].format = (sad[0] & 0x78) >> 3;
> +				sads[j].channels = sad[0] & 0x7;
> +				sads[j].freq = sad[1] & 0x7F;
> +				sads[j].byte2 = sad[2];
> +			}
> +		}
> +	}
> +
> +	return sads;
> +}
> +EXPORT_SYMBOL(drm_edid_to_sad);
> +
> +/**
>    * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
>    * @connector: connector associated with the HDMI/DP sink
>    * @mode: the display mode
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 5da1b4a..b36d052 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -244,12 +244,36 @@ struct edid {
>   
>   #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
>   
> +#define SAD_FORMAT_LPCM			0x01
> +#define SAD_FORMAT_AC3			0x02
> +#define SAD_FORMAT_MPEG1		0x03
> +#define SAD_FORMAT_MP3			0x04
> +#define SAD_FORMAT_MPEG2		0x05
> +#define SAD_FORMAT_AAC			0x06
> +#define SAD_FORMAT_DTS			0x07
> +#define SAD_FORMAT_ATRAC		0x08
> +#define SAD_FORMAT_ONE_BIT_AUDIO	0x09
> +#define SAD_FORMAT_DOLBY_DIGITAL	0x0a
> +#define SAD_FORMAT_DTS_HD		0x0b
> +#define SAD_FORMAT_MAT_MLP		0x0c
> +#define SAD_FORMAT_DST			0x0d
> +#define SAD_FORMAT_WMA_PRO		0x0e
> +
> +/* Short Audio Descriptor */
> +struct cea_sad {
> +	u8 format;
> +	u8 channels; /* max number of channels - 1 */
> +	u8 freq;
> +	u8 byte2; /* meaning depends on format */
> +};
> +
>   struct drm_encoder;
>   struct drm_connector;
>   struct drm_display_mode;
>   struct hdmi_avi_infoframe;
>   
>   void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid);
> +struct cea_sad *drm_edid_to_sad(struct edid *edid);
>   int drm_av_sync_delay(struct drm_connector *connector,
>   		      struct drm_display_mode *mode);
>   struct drm_connector *drm_select_eld(struct drm_encoder *encoder,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Rafał Miłecki]

2013/4/7 Boszormenyi Zoltan <zboszor@pr.hu>:
> will there be a followup patch where this function is actually used?

Sure :) It's just early patch post, with RFC, to see if I'm doing it
the right way.

-- 
Rafał
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Rafał Miłecki]

2013/4/7 Rafał Miłecki <zajec5@gmail.com>:
> Some devices (ATI/AMD cards) don't want passing ELD struct to the
> hardware but just require filling specific registers and then
> hardware/firmware does the rest. In such a cases we need to read info
> from SAD blocks and put them in the correct registers.

If you wish to see how exactly it works on radeon, see for example evergreend.h:
AZ_F0_CODEC_PIN0_CONTROL_AUDIO_DESCRIPTOR0
AZ_F0_CODEC_PIN0_CONTROL_AUDIO_DESCRIPTOR1
& friends.

-- 
Rafał
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 4465 bytes --]

Am Sonntag, den 07.04.2013, 12:52 +0200 schrieb Rafał Miłecki:
> Some devices (ATI/AMD cards) don't want passing ELD struct to the

want to pass

> hardware but just require filling specific registers and then

at end: »then *the* hardware«

(I think)

> hardware/firmware does the rest. In such a cases we need to read info

• »In such cases« (without »a«)
• the info

> from SAD blocks and put them in the correct registers.

Maybe mention the function name in the commit message (even summary)
too.

> ---
>  drivers/gpu/drm/drm_edid.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |   24 +++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e2acfdb..7c9e799 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2511,6 +2511,61 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  EXPORT_SYMBOL(drm_edid_to_eld);
>  
>  /**
> + * drm_edid_to_sad - extracts SADs from EDID
> + * @edid: EDID to parse
> + *
> + * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.
> + */
> +struct cea_sad *drm_edid_to_sad(struct edid *edid)
> +{
> +	struct cea_sad *sads = NULL;

No need to set it NULL, as it gets assigned later on? Looks like in the
end there is a corner case, where nothing gets assigned in the
`for_each_cea_db` loop. So the compiler is going to complain.

> +	int i, start, end, dbl;
> +	u8 *db, *cea;
> +
> +	cea = drm_find_cea_extension(edid);
> +	if (!cea) {
> +		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> +		return NULL;
> +	}
> +
> +	if (cea_revision(cea) < 3) {
> +		DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
> +		return NULL;
> +	}
> +
> +	if (cea_db_offsets(cea, &start, &end)) {
> +		DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
> +		return NULL;
> +	}

Could the above be put in some helper too:
`is_cea_valid_for_version(cea, version)`?

> +	for_each_cea_db(cea, i, start, end) {
> +		db = &cea[i];
> +		dbl = cea_db_payload_len(db);
> +
> +		if (cea_db_tag(db) == AUDIO_BLOCK) {
> +			int count = dbl / 3; /* SAD is 3B */
> +			int j;
> +
> +			sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);
> +			if (!sads)

Add an error too? »kzallac failed« or something like this?

> +				return NULL;
> +
> +			for (j = 0; j < count; j++) {
> +				u8 *sad = &db[1 + j * 3];
> +
> +				sads[j].format = (sad[0] & 0x78) >> 3;
> +				sads[j].channels = sad[0] & 0x7;
> +				sads[j].freq = sad[1] & 0x7F;
> +				sads[j].byte2 = sad[2];
> +			}
> +		}
> +	}
> +
> +	return sads;
> +}
> +EXPORT_SYMBOL(drm_edid_to_sad);
> +
> +/**
>   * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
>   * @connector: connector associated with the HDMI/DP sink
>   * @mode: the display mode
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 5da1b4a..b36d052 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -244,12 +244,36 @@ struct edid {
>  
>  #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
>  
> +#define SAD_FORMAT_LPCM			0x01
> +#define SAD_FORMAT_AC3			0x02

At least in my MUA indentation looks different.

> +#define SAD_FORMAT_MPEG1		0x03
> +#define SAD_FORMAT_MP3			0x04
> +#define SAD_FORMAT_MPEG2		0x05
> +#define SAD_FORMAT_AAC			0x06
> +#define SAD_FORMAT_DTS			0x07
> +#define SAD_FORMAT_ATRAC		0x08
> +#define SAD_FORMAT_ONE_BIT_AUDIO	0x09
> +#define SAD_FORMAT_DOLBY_DIGITAL	0x0a
> +#define SAD_FORMAT_DTS_HD		0x0b
> +#define SAD_FORMAT_MAT_MLP		0x0c
> +#define SAD_FORMAT_DST			0x0d
> +#define SAD_FORMAT_WMA_PRO		0x0e
> +
> +/* Short Audio Descriptor */
> +struct cea_sad {
> +	u8 format;
> +	u8 channels; /* max number of channels - 1 */
> +	u8 freq;
> +	u8 byte2; /* meaning depends on format */
> +};
> +
>  struct drm_encoder;
>  struct drm_connector;
>  struct drm_display_mode;
>  struct hdmi_avi_infoframe;
>  
>  void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid);
> +struct cea_sad *drm_edid_to_sad(struct edid *edid);
>  int drm_av_sync_delay(struct drm_connector *connector,
>  		      struct drm_display_mode *mode);
>  struct drm_connector *drm_select_eld(struct drm_encoder *encoder,

Otherwise this looks good.


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Rafał Miłecki]

Thanks for comments!

2013/4/7 Paul Menzel <paulepanter@users.sourceforge.net>:
> Am Sonntag, den 07.04.2013, 12:52 +0200 schrieb Rafał Miłecki:
>> +struct cea_sad *drm_edid_to_sad(struct edid *edid)
>> +{
>> +     struct cea_sad *sads = NULL;
>
> No need to set it NULL, as it gets assigned later on? Looks like in the
> end there is a corner case, where nothing gets assigned in the
> `for_each_cea_db` loop. So the compiler is going to complain.

Right, I can drop that. Curious, my gcc didn't complain.


>> +     if (cea_revision(cea) < 3) {
>> +             DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
>> +             return NULL;
>> +     }
>> +
>> +     if (cea_db_offsets(cea, &start, &end)) {
>> +             DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
>> +             return NULL;
>> +     }
>
> Could the above be put in some helper too:
> `is_cea_valid_for_version(cea, version)`?

Not sure if it's worth a helper. But maybe I'll go and just use a single check?
if (cea_revision(cea) < 3 || cea_db_offsets(...))


>> +     for_each_cea_db(cea, i, start, end) {
>> +             db = &cea[i];
>> +             dbl = cea_db_payload_len(db);
>> +
>> +             if (cea_db_tag(db) == AUDIO_BLOCK) {
>> +                     int count = dbl / 3; /* SAD is 3B */
>> +                     int j;
>> +
>> +                     sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);
>> +                     if (!sads)
>
> Add an error too? »kzallac failed« or something like this?

Allocation failures are aloud enough on the lower level. I was
corrected few times already to don't put warnings when alloc fails. If
you want me to point to the discussions, I'll have to search though.


>> +#define SAD_FORMAT_LPCM                      0x01
>> +#define SAD_FORMAT_AC3                       0x02
>
> At least in my MUA indentation looks different.

It's just a matter of .patch format. Patch file has "+" sign at a
beginning of line and \t can look different when viewing patch. Please
apply a patch and check drm_edid.h then - it'll be OK.

-- 
Rafał
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 2586 bytes --]

Am Sonntag, den 07.04.2013, 14:11 +0200 schrieb Rafał Miłecki:
> Thanks for comments!
> 
> 2013/4/7 Paul Menzel <paulepanter@users.sourceforge.net>:
> > Am Sonntag, den 07.04.2013, 12:52 +0200 schrieb Rafał Miłecki:
> >> +struct cea_sad *drm_edid_to_sad(struct edid *edid)
> >> +{
> >> +     struct cea_sad *sads = NULL;
> >
> > No need to set it NULL, as it gets assigned later on? Looks like in the
> > end there is a corner case, where nothing gets assigned in the
> > `for_each_cea_db` loop. So the compiler is going to complain.
> 
> Right, I can drop that. Curious, my gcc didn't complain.

Oh, I did not check with GCC. But there is that corner case, if I am not
mistaken.

> >> +     if (cea_revision(cea) < 3) {
> >> +             DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     if (cea_db_offsets(cea, &start, &end)) {
> >> +             DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
> >> +             return NULL;
> >> +     }
> >
> > Could the above be put in some helper too:
> > `is_cea_valid_for_version(cea, version)`?
> 
> Not sure if it's worth a helper. But maybe I'll go and just use a single check?
> if (cea_revision(cea) < 3 || cea_db_offsets(...))

I kind of like the useful error messages, you put there. So if no
helper, then I’d prefer if you leave it as is.

> >> +     for_each_cea_db(cea, i, start, end) {
> >> +             db = &cea[i];
> >> +             dbl = cea_db_payload_len(db);
> >> +
> >> +             if (cea_db_tag(db) == AUDIO_BLOCK) {
> >> +                     int count = dbl / 3; /* SAD is 3B */
> >> +                     int j;
> >> +
> >> +                     sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);
> >> +                     if (!sads)
> >
> > Add an error too? »kzallac failed« or something like this?
> 
> Allocation failures are aloud enough on the lower level. I was
> corrected few times already to don't put warnings when alloc fails. If
> you want me to point to the discussions, I'll have to search though.

No need. I believe you.

> >> +#define SAD_FORMAT_LPCM                      0x01
> >> +#define SAD_FORMAT_AC3                       0x02
> >
> > At least in my MUA indentation looks different.
> 
> It's just a matter of .patch format. Patch file has "+" sign at a
> beginning of line and \t can look different when viewing patch. Please
> apply a patch and check drm_edid.h then - it'll be OK.

Thanks. Sorry for not checking that myself.


Thanks,

paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: add helper extracting SADs from EDID

[Ville Syrjälä]

On Sun, Apr 07, 2013 at 12:52:57PM +0200, Rafał Miłecki wrote:
> Some devices (ATI/AMD cards) don't want passing ELD struct to the
> hardware but just require filling specific registers and then
> hardware/firmware does the rest. In such a cases we need to read info
> from SAD blocks and put them in the correct registers.
> ---
>  drivers/gpu/drm/drm_edid.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |   24 +++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e2acfdb..7c9e799 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2511,6 +2511,61 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  EXPORT_SYMBOL(drm_edid_to_eld);
>  
>  /**
> + * drm_edid_to_sad - extracts SADs from EDID
> + * @edid: EDID to parse
> + *
> + * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.

The function returns an array of SADs with the zeroed SAD as a
terminator. That kind of magic needs clear documentation IMHO.

In fact I think it would be better if you returned the number of 
SADs too, instead of relying on the zero termination. With a
clear "number of SADs" return parameter it's less likely people
would misuse this by accident.

> + */
> +struct cea_sad *drm_edid_to_sad(struct edid *edid)
> +{
> +	struct cea_sad *sads = NULL;
> +	int i, start, end, dbl;
> +	u8 *db, *cea;
> +
> +	cea = drm_find_cea_extension(edid);
> +	if (!cea) {
> +		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> +		return NULL;
> +	}
> +
> +	if (cea_revision(cea) < 3) {
> +		DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
> +		return NULL;
> +	}
> +
> +	if (cea_db_offsets(cea, &start, &end)) {
> +		DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
> +		return NULL;
> +	}
> +
> +	for_each_cea_db(cea, i, start, end) {
> +		db = &cea[i];
> +		dbl = cea_db_payload_len(db);
> +
> +		if (cea_db_tag(db) == AUDIO_BLOCK) {
> +			int count = dbl / 3; /* SAD is 3B */
> +			int j;
> +
> +			sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);

Missing parens -> miscalculated size. Should really be kcalloc().

> +			if (!sads)
> +				return NULL;
> +
> +			for (j = 0; j < count; j++) {
> +				u8 *sad = &db[1 + j * 3];
> +
> +				sads[j].format = (sad[0] & 0x78) >> 3;
> +				sads[j].channels = sad[0] & 0x7;
> +				sads[j].freq = sad[1] & 0x7F;
> +				sads[j].byte2 = sad[2];
> +			}

If the EDID has multiple AUDIO_BLOCKs this will leak memory. 'break'
here would avoid that.

> +		}
> +	}
> +
> +	return sads;
> +}
> +EXPORT_SYMBOL(drm_edid_to_sad);
> +
> +/**
>   * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
>   * @connector: connector associated with the HDMI/DP sink
>   * @mode: the display mode
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 5da1b4a..b36d052 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -244,12 +244,36 @@ struct edid {
>  
>  #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
>  
> +#define SAD_FORMAT_LPCM			0x01
> +#define SAD_FORMAT_AC3			0x02
> +#define SAD_FORMAT_MPEG1		0x03
> +#define SAD_FORMAT_MP3			0x04
> +#define SAD_FORMAT_MPEG2		0x05
> +#define SAD_FORMAT_AAC			0x06
> +#define SAD_FORMAT_DTS			0x07
> +#define SAD_FORMAT_ATRAC		0x08
> +#define SAD_FORMAT_ONE_BIT_AUDIO	0x09
> +#define SAD_FORMAT_DOLBY_DIGITAL	0x0a
> +#define SAD_FORMAT_DTS_HD		0x0b
> +#define SAD_FORMAT_MAT_MLP		0x0c
> +#define SAD_FORMAT_DST			0x0d
> +#define SAD_FORMAT_WMA_PRO		0x0e
> +
> +/* Short Audio Descriptor */
> +struct cea_sad {
> +	u8 format;
> +	u8 channels; /* max number of channels - 1 */
> +	u8 freq;
> +	u8 byte2; /* meaning depends on format */
> +};
> +
>  struct drm_encoder;
>  struct drm_connector;
>  struct drm_display_mode;
>  struct hdmi_avi_infoframe;
>  
>  void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid);
> +struct cea_sad *drm_edid_to_sad(struct edid *edid);
>  int drm_av_sync_delay(struct drm_connector *connector,
>  		      struct drm_display_mode *mode);
>  struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
> -- 
> 1.7.10.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel