Re: [igt-dev] [PATCH i-g-t 3/5] lib/igt_eld: parse Short Audio Descriptors

[Martin Peres <martin.peres@linux.intel.com>]

On 31/05/2019 14:21, Simon Ser wrote:
> Each valid ELD entry can contain zero, one or more Short Audio Descriptor
> blocks. These are exposed in sadN_* fields (N being the index of the SAD).
> 
> We need to parse them to be able to check that ALSA has properly processed
> them.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_edid.h |  2 ++
>  lib/igt_eld.c  | 85 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/igt_eld.h  | 14 +++++++++
>  3 files changed, 100 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index 7edd7e38f41e..1d0c6aa29578 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -30,6 +30,8 @@
>  
>  #include <stdint.h>
>  
> +#include <xf86drmMode.h>
> +

Are you sure this change belongs to this commit? Can't find anything
here that would need this header.

>  struct est_timings {
>  	uint8_t t1;
>  	uint8_t t2;
> diff --git a/lib/igt_eld.c b/lib/igt_eld.c
> index a198001a56d7..732bbabd2d7c 100644
> --- a/lib/igt_eld.c
> +++ b/lib/igt_eld.c
> @@ -35,6 +35,7 @@
>  
>  #define ELD_PREFIX "eld#"
>  #define ELD_DELIM " \t"
> +#define SAD_FMT "sad%d_%ms"
>  
>  /**
>   * EDID-Like Data (ELD) is metadata parsed and exposed by ALSA for HDMI and
> @@ -43,13 +44,87 @@
>   * on).
>   */
>  
> +static enum cea_sad_format parse_sad_coding_type(const char *value)
> +{
> +	if (strcmp(value, "LPCM") == 0)
> +		return CEA_SAD_FORMAT_PCM;
> +	else
> +		return 0;
> +}
> +
> +static enum cea_sad_sampling_rate parse_sad_rate(const char *value)
> +{
> +	switch (atoi(value)) {
> +	case 32000:
> +		return CEA_SAD_SAMPLING_RATE_32KHZ;
> +	case 44100:
> +		return CEA_SAD_SAMPLING_RATE_44KHZ;
> +	case 48000:
> +		return CEA_SAD_SAMPLING_RATE_48KHZ;
> +	case 88000:
> +		return CEA_SAD_SAMPLING_RATE_88KHZ;
> +	case 96000:
> +		return CEA_SAD_SAMPLING_RATE_96KHZ;
> +	case 176000:
> +		return CEA_SAD_SAMPLING_RATE_176KHZ;
> +	case 192000:
> +		return CEA_SAD_SAMPLING_RATE_192KHZ;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static enum cea_sad_pcm_sample_size parse_sad_bit(const char *value)
> +{
> +	switch (atoi(value)) {
> +	case 16:
> +		return CEA_SAD_SAMPLE_SIZE_16;
> +	case 20:
> +		return CEA_SAD_SAMPLE_SIZE_20;
> +	case 24:
> +		return CEA_SAD_SAMPLE_SIZE_24;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static void parse_sad_field(struct eld_sad *sad, const char *key, char *value)
> +{
> +	char *tok;
> +
> +	/* Some fields are prefixed with the raw hex value, strip it */
> +	if (value[0] == '[') {
> +		value = strchr(value, ' ');
> +		igt_assert(value != NULL);
> +		value++; /* skip the space */
> +	}
> +
> +	/* Single-value fields */
> +	if (strcmp(key, "coding_type") == 0)
> +		sad->coding_type = parse_sad_coding_type(value);
> +	else if (strcmp(key, "channels") == 0)
> +		sad->channels = atoi(value);
> +
> +	/* Multiple-value fields */
> +	tok = strtok(value, " ");
> +	while (tok) {
> +		if (strcmp(key, "rates") == 0)
> +			sad->rates |= parse_sad_rate(tok);
> +		else if (strcmp(key, "bits") == 0)
> +			sad->bits |= parse_sad_bit(tok);
> +
> +		tok = strtok(NULL, " ");
> +	}
> +}
> +
>  static bool eld_parse_entry(const char *path, struct eld_entry *eld)
>  {
>  	FILE *f;
>  	char buf[1024];
> -	char *key, *value;
> +	char *key, *value, *sad_key;

Poor key, why are you sad? :D

More seriously, a little documentation in this file about the acronyms
would be appreciated!

>  	size_t len;
>  	bool monitor_present;
> +	int sad_index;
>  
>  	memset(eld, 0, sizeof(*eld));
>  
> @@ -76,6 +151,14 @@ static bool eld_parse_entry(const char *path, struct eld_entry *eld)
>  		else if (strcmp(key, "monitor_name") == 0)
>  			snprintf(eld->monitor_name, sizeof(eld->monitor_name),
>  				 "%s", value);
> +		else if (strcmp(key, "sad_count") == 0)
> +			eld->sads_len = atoi(value);
> +		else if (sscanf(key, "sad%d_%ms", &sad_index, &sad_key) == 2) {
> +			igt_assert(sad_index < ELD_SADS_CAP);
> +			igt_assert(sad_index < eld->sads_len);

Good that you added that! This patch looks good:

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>

> +			parse_sad_field(&eld->sads[sad_index], sad_key, value);
> +			free(sad_key);
> +		}
>  	}
>  
>  	if (ferror(f) != 0) {
> diff --git a/lib/igt_eld.h b/lib/igt_eld.h
> index 21fe3537acf9..e16187884d4b 100644
> --- a/lib/igt_eld.h
> +++ b/lib/igt_eld.h
> @@ -30,9 +30,23 @@
>  
>  #include <stdbool.h>
>  
> +#include "igt_edid.h"
> +
> +#define ELD_SADS_CAP 4
> +
> +/** eld_sad: Short Audio Descriptor */
> +struct eld_sad {
> +	enum cea_sad_format coding_type;
> +	int channels;
> +	unsigned int rates; /* enum cea_sad_sampling_rate */
> +	unsigned int bits; /* enum cea_sad_pcm_sample_size */
> +};
> +
>  struct eld_entry {
>  	bool valid;
>  	char monitor_name[16];
> +	size_t sads_len;
> +	struct eld_sad sads[ELD_SADS_CAP];
>  };
>  
>  bool eld_has_igt(void);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev