Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Hi @Jani Nikula
> -Original Message-
> From: Jani Nikula
> Sent: 26 June 2023 22:48
> To: Golani, Mitulkumar Ajitkumar ;
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute
> SAD
>
> On Mon, 26 Jun 2023, Mitul Golani
> wrote:
> > Compute SADs that takes into account the supported rate and channel
> > based on the capabilities of the audio source. This wrapper function
> > should encapsulate the logic for determining the supported rate and
> > channel and should return a set of SADs that are compatible with the
> > source.
> >
> > --v1:
> > - call intel_audio_compute_eld in this commit as it is defined here
> >
> > --v2:
> > - Handle case when max frequency is less than 32k.
> > - remove drm prefix.
> > - name change for parse_sad to eld_to_sad.
> >
> > --v3:
> > - Use signed int wherever required.
> > - add debug trace when channel is limited.
> >
> > Signed-off-by: Mitul Golani
> > ---
> > drivers/gpu/drm/i915/display/intel_audio.c | 69
> > ++ drivers/gpu/drm/i915/display/intel_audio.h |
> > 1 + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> > 3 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index e20ffc8e9654..1a1c773c1d41 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -794,6 +794,75 @@ bool intel_audio_compute_config(struct
> intel_encoder *encoder,
> > return true;
> > }
> >
> > +static int sad_to_channels(const u8 *sad) {
> > + return 1 + (sad[0] & 0x7);
> > +}
> > +
> > +static inline u8 *eld_to_sad(u8 *eld)
>
> Please don't use inline.
>
> > +{
> > + int ver, mnl;
> > +
> > + ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >>
> DRM_ELD_VER_SHIFT;
> > + if (ver != 2 && ver != 31)
> > + return NULL;
> > +
> > + mnl = drm_eld_mnl(eld);
> > + if (mnl > 16)
> > + return NULL;
> > +
> > + return eld + DRM_ELD_CEA_SAD(mnl, 0);
>
> Feels like this should be in drm_edid.[ch]... but hey, it actually is!
> drm_eld_sad().
As now, eld is added to crtc_state as well, intention was to overwrite
source eld. drm_eld_sad is returning const *. Due to which created
function local to intel_audio.c.
Also addressed rest other comments to this patch.
Thanks,
MItul
>
> > +}
> > +
> > +static int get_supported_freq_mask(struct intel_crtc_state
> > +*crtc_state) {
> > + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> > +192000, 0};
>
> This needs a comment referencing the source spec, as apparently the order
> is significant.
>
> > + int mask = 0;
> > +
> > + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
>
> int index, and please declare it separately instead of within the for.
>
> > + mask |= 1 << index;
>
> This means the first one is always supported. Is that the case? What if
> max_frequency is 0?
>
> > + if (crtc_state->audio.max_frequency !=
> audio_freq_hz[index])
> > + continue;
> > + else
> > + break;
>
> Maybe you mean:
>
> if (audio_freq_hz[index] > crtc_state-
> >audio.max_frequency)
> break;
>
> mask |= 1 << index;
>
> > + }
> > +
> > + return mask;
> > +}
> > +
> > +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state) {
> > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > + u8 *eld, *sad;
> > + int index, mask = 0;
> > +
> > + eld = crtc_state->eld;
> > + if (!eld) {
> > + drm_err(>drm, "failed to locate eld\n");
>
> It doesn't need to be an error.
>
> > + return;
> > + }
> > +
> > + sad = (u8 *)eld_to_sad(eld);
>
> Unnecessary cast.
>
> > + if (sad) {
>
> if (!sad)
> return;
>
> and reduce indent below.
>
> > + mask = get_supported_freq_mask(crtc_state);
> > +
> > + for (index = 0; index < drm_eld_sad_count(eld); index++, sad
> += 3) {
> > + /*
> > +* Respect source restricitions. Limit capabilities to a
> subset that is
> > +
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate
and channel based on the capabilities of the audio source.
This wrapper function should encapsulate the logic for
determining the supported rate and channel and should
return a set of SADs that are compatible with the source.
--v1:
- call intel_audio_compute_eld in this commit as it is
defined here
--v2:
- Handle case when max frequency is less than 32k.
- remove drm prefix.
- name change for parse_sad to eld_to_sad.
--v3:
- Use signed int wherever required.
- add debug trace when channel is limited.
--v4:
- remove inline from eld_to_sad.
- declare index outside of for loop with int type.
- Correct mask value calculation.
- remove drm_err, instead just return if eld parsing failed.
- remove unncessary typecast
- reduce indentation while parsing sad
- use intel_audio_compute_eld as static and call bandwidth
calculation just before that.
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 73 +-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index 79377e33a59b..9f4fc3f9b224 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -806,6 +806,77 @@ static void calc_audio_config_params(struct
intel_crtc_state *pipe_config)
pipe_config->audio.max_channel_count = 0;
}
+static int sad_to_channels(const u8 *sad)
+{
+ return 1 + (sad[0] & 0x7);
+}
+
+static u8 *eld_to_sad(u8 *eld)
+{
+ int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static int get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+ int rate[] = { 32000, 44100, 48000, 88000, 96000, 176000, 192000 };
+ int mask = 0, index;
+
+ for (index = 0; index < ARRAY_SIZE(rate); index++) {
+ if (rate[index] > crtc_state->audio.max_rate)
+ break;
+
+ mask |= 1 << index;
+
+ if (crtc_state->audio.max_rate != rate[index])
+ continue;
+ }
+
+ return mask;
+}
+
+static void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+ u8 *eld, *sad;
+ int index, mask = 0;
+
+ eld = crtc_state->eld;
+ if (!eld)
+ return;
+
+ sad = eld_to_sad(eld);
+ if (!sad)
+ return;
+
+ calc_audio_config_params(crtc_state);
+
+ mask = get_supported_freq_mask(crtc_state);
+ for (index = 0; index < drm_eld_sad_count(eld); index++, sad += 3) {
+ /*
+* Respect source restricitions. Limit capabilities to a subset
that is
+* supported both by the source and the sink.
+*/
+ if (sad_to_channels(sad) >=
crtc_state->audio.max_channel_count) {
+ sad[0] &= ~0x7;
+ sad[0] |= crtc_state->audio.max_channel_count - 1;
+ drm_dbg_kms(>drm, "Channel count is limited to
%d\n",
+ crtc_state->audio.max_channel_count - 1);
+ }
+
+ sad[1] &= mask;
+ }
+}
+
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state)
@@ -827,7 +898,7 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
- calc_audio_config_params(crtc_state);
+ intel_audio_compute_eld(crtc_state);
return true;
}
--
2.25.1
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate and channel
based on the capabilities of the audio source. This wrapper function
should encapsulate the logic for determining the supported rate and
channel and should return a set of SADs that are compatible with the
source.
--v1:
- call intel_audio_compute_eld in this commit as it is defined here
--v2:
- Handle case when max frequency is less than 32k.
- remove drm prefix.
- name change for parse_sad to eld_to_sad.
--v3:
- Use signed int wherever required.
- add debug trace when channel is limited.
--v4:
- remove inline from eld_to_sad.
- declare index outside of for loop with int type.
- Correct mask value calculation.
- remove drm_err, instead just return if eld parsing failed.
- remove unncessary typecast
- reduce indentation while parsing sad
- use intel_audio_compute_eld as static and call bandwidth calculation just
before that.
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 73 +-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index 79377e33a59b..eca9452849ff 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -806,6 +806,77 @@ static void calc_audio_config_params(struct
intel_crtc_state *pipe_config)
pipe_config->audio.max_channel_count = 0;
}
+static int sad_to_channels(const u8 *sad)
+{
+return 1 + (sad[0] & 0x7);
+}
+
+static u8 *eld_to_sad(u8 *eld)
+{
+ int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static int get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+int rate[] = {32000, 44100, 48000, 88000, 96000, 176000, 192000};
+int mask = 0, index;
+
+for (index = 0; index < ARRAY_SIZE(rate); index++) {
+if (rate[index] > crtc_state->audio.max_rate)
+break;
+
+mask |= 1 << index;
+
+if (crtc_state->audio.max_rate != rate[index])
+continue;
+}
+
+return mask;
+}
+
+static void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+u8 *eld, *sad;
+int index, mask = 0;
+
+eld = crtc_state->eld;
+if (!eld)
+return;
+
+sad = eld_to_sad(eld);
+if (!sad)
+return;
+
+ calc_audio_config_params(crtc_state);
+
+mask = get_supported_freq_mask(crtc_state);
+for (index = 0; index < drm_eld_sad_count(eld); index++, sad += 3) {
+/*
+ * Respect source restricitions. Limit capabilities to a
subset that is
+ * supported both by the source and the sink.
+ */
+if (sad_to_channels(sad) >=
crtc_state->audio.max_channel_count) {
+sad[0] &= ~0x7;
+sad[0] |= crtc_state->audio.max_channel_count - 1;
+drm_dbg_kms(>drm, "Channel count is limited to
%d\n",
+crtc_state->audio.max_channel_count - 1);
+}
+
+sad[1] &= mask;
+}
+}
+
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state)
@@ -827,7 +898,7 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
- calc_audio_config_params(crtc_state);
+ intel_audio_compute_eld(crtc_state);
return true;
}
--
2.25.1
Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
On Mon, 26 Jun 2023, Mitul Golani wrote:
> Compute SADs that takes into account the supported rate and channel
> based on the capabilities of the audio source. This wrapper function
> should encapsulate the logic for determining the supported rate and
> channel and should return a set of SADs that are compatible with the
> source.
>
> --v1:
> - call intel_audio_compute_eld in this commit as it is defined here
>
> --v2:
> - Handle case when max frequency is less than 32k.
> - remove drm prefix.
> - name change for parse_sad to eld_to_sad.
>
> --v3:
> - Use signed int wherever required.
> - add debug trace when channel is limited.
>
> Signed-off-by: Mitul Golani
> ---
> drivers/gpu/drm/i915/display/intel_audio.c | 69 ++
> drivers/gpu/drm/i915/display/intel_audio.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> 3 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> b/drivers/gpu/drm/i915/display/intel_audio.c
> index e20ffc8e9654..1a1c773c1d41 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -794,6 +794,75 @@ bool intel_audio_compute_config(struct intel_encoder
> *encoder,
> return true;
> }
>
> +static int sad_to_channels(const u8 *sad)
> +{
> + return 1 + (sad[0] & 0x7);
> +}
> +
> +static inline u8 *eld_to_sad(u8 *eld)
Please don't use inline.
> +{
> + int ver, mnl;
> +
> + ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
> + if (ver != 2 && ver != 31)
> + return NULL;
> +
> + mnl = drm_eld_mnl(eld);
> + if (mnl > 16)
> + return NULL;
> +
> + return eld + DRM_ELD_CEA_SAD(mnl, 0);
Feels like this should be in drm_edid.[ch]... but hey, it actually is!
drm_eld_sad().
> +}
> +
> +static int get_supported_freq_mask(struct intel_crtc_state *crtc_state)
> +{
> + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> 192000, 0};
This needs a comment referencing the source spec, as apparently the
order is significant.
> + int mask = 0;
> +
> + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
int index, and please declare it separately instead of within the for.
> + mask |= 1 << index;
This means the first one is always supported. Is that the case? What if
max_frequency is 0?
> + if (crtc_state->audio.max_frequency != audio_freq_hz[index])
> + continue;
> + else
> + break;
Maybe you mean:
if (audio_freq_hz[index] > crtc_state->audio.max_frequency)
break;
mask |= 1 << index;
> + }
> +
> + return mask;
> +}
> +
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> + u8 *eld, *sad;
> + int index, mask = 0;
> +
> + eld = crtc_state->eld;
> + if (!eld) {
> + drm_err(>drm, "failed to locate eld\n");
It doesn't need to be an error.
> + return;
> + }
> +
> + sad = (u8 *)eld_to_sad(eld);
Unnecessary cast.
> + if (sad) {
if (!sad)
return;
and reduce indent below.
> + mask = get_supported_freq_mask(crtc_state);
> +
> + for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
> 3) {
> + /*
> + * Respect source restricitions. Limit capabilities to
> a subset that is
> + * supported both by the source and the sink.
> + */
> + if (sad_to_channels(sad) >=
> crtc_state->audio.max_channel) {
> + sad[0] &= ~0x7;
> + sad[0] |= crtc_state->audio.max_channel - 1;
> + drm_dbg_kms(>drm, "Channel count is
> limited to %d\n",
> + crtc_state->audio.max_channel - 1);
> + }
> +
> + sad[1] &= mask;
> + }
> + }
> +}
This should also be static within intel_audio.c.
> +
> /**
> * intel_audio_codec_enable - Enable the audio codec for HD audio
> * @encoder: encoder on which to enable audio
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.h
> b/drivers/gpu/drm/i915/display/intel_audio.h
> index be3edf9c4982..a0162cdc7999 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> @@ -17,6 +17,7 @@ struct intel_encoder;
> #define MAX_CHANNEL_COUNT8
>
> void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
> bool intel_audio_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate and channel
based on the capabilities of the audio source. This wrapper function
should encapsulate the logic for determining the supported rate and
channel and should return a set of SADs that are compatible with the
source.
--v1:
- call intel_audio_compute_eld in this commit as it is defined here
--v2:
- Handle case when max frequency is less than 32k.
- remove drm prefix.
- name change for parse_sad to eld_to_sad.
--v3:
- Use signed int wherever required.
- add debug trace when channel is limited.
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 69 ++
drivers/gpu/drm/i915/display/intel_audio.h | 1 +
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
3 files changed, 72 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index e20ffc8e9654..1a1c773c1d41 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -794,6 +794,75 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
return true;
}
+static int sad_to_channels(const u8 *sad)
+{
+ return 1 + (sad[0] & 0x7);
+}
+
+static inline u8 *eld_to_sad(u8 *eld)
+{
+ int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static int get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+ int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
192000, 0};
+ int mask = 0;
+
+ for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
+ mask |= 1 << index;
+ if (crtc_state->audio.max_frequency != audio_freq_hz[index])
+ continue;
+ else
+ break;
+ }
+
+ return mask;
+}
+
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+ u8 *eld, *sad;
+ int index, mask = 0;
+
+ eld = crtc_state->eld;
+ if (!eld) {
+ drm_err(>drm, "failed to locate eld\n");
+ return;
+ }
+
+ sad = (u8 *)eld_to_sad(eld);
+ if (sad) {
+ mask = get_supported_freq_mask(crtc_state);
+
+ for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
3) {
+ /*
+* Respect source restricitions. Limit capabilities to
a subset that is
+* supported both by the source and the sink.
+*/
+ if (sad_to_channels(sad) >=
crtc_state->audio.max_channel) {
+ sad[0] &= ~0x7;
+ sad[0] |= crtc_state->audio.max_channel - 1;
+ drm_dbg_kms(>drm, "Channel count is
limited to %d\n",
+ crtc_state->audio.max_channel - 1);
+ }
+
+ sad[1] &= mask;
+ }
+ }
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @encoder: encoder on which to enable audio
diff --git a/drivers/gpu/drm/i915/display/intel_audio.h
b/drivers/gpu/drm/i915/display/intel_audio.h
index 09697c770dbf..4979156f8156 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.h
+++ b/drivers/gpu/drm/i915/display/intel_audio.h
@@ -17,6 +17,7 @@ struct intel_encoder;
#define MAX_CHANNEL_COUNT 8
void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 6a4d477e8a15..daaa08c0ee47 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2402,6 +2402,8 @@ int intel_hdmi_compute_config(struct intel_encoder
*encoder,
return -EINVAL;
}
+ intel_audio_compute_eld(pipe_config);
+
return 0;
}
--
2.25.1
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate and channel
based on the capabilities of the audio source. This wrapper function
should encapsulate the logic for determining the supported rate and
channel and should return a set of SADs that are compatible with the
source.
--v1:
- call intel_audio_compute_eld in this commit as it is defined here
--v2:
- Handle case when max frequency is less than 32k.
- remove drm prefix.
- name change for parse_sad to eld_to_sad.
--v3:
- Use signed int wherever required.
- add debug trace when channel is limited.
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 69 ++
drivers/gpu/drm/i915/display/intel_audio.h | 1 +
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
3 files changed, 72 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index e20ffc8e9654..1a1c773c1d41 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -794,6 +794,75 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
return true;
}
+static int sad_to_channels(const u8 *sad)
+{
+ return 1 + (sad[0] & 0x7);
+}
+
+static inline u8 *eld_to_sad(u8 *eld)
+{
+ int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static int get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+ int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
192000, 0};
+ int mask = 0;
+
+ for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
+ mask |= 1 << index;
+ if (crtc_state->audio.max_frequency != audio_freq_hz[index])
+ continue;
+ else
+ break;
+ }
+
+ return mask;
+}
+
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+ u8 *eld, *sad;
+ int index, mask = 0;
+
+ eld = crtc_state->eld;
+ if (!eld) {
+ drm_err(>drm, "failed to locate eld\n");
+ return;
+ }
+
+ sad = (u8 *)eld_to_sad(eld);
+ if (sad) {
+ mask = get_supported_freq_mask(crtc_state);
+
+ for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
3) {
+ /*
+* Respect source restricitions. Limit capabilities to
a subset that is
+* supported both by the source and the sink.
+*/
+ if (sad_to_channels(sad) >=
crtc_state->audio.max_channel) {
+ sad[0] &= ~0x7;
+ sad[0] |= crtc_state->audio.max_channel - 1;
+ drm_dbg_kms(>drm, "Channel count is
limited to %d\n",
+ crtc_state->audio.max_channel - 1);
+ }
+
+ sad[1] &= mask;
+ }
+ }
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @encoder: encoder on which to enable audio
diff --git a/drivers/gpu/drm/i915/display/intel_audio.h
b/drivers/gpu/drm/i915/display/intel_audio.h
index be3edf9c4982..a0162cdc7999 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.h
+++ b/drivers/gpu/drm/i915/display/intel_audio.h
@@ -17,6 +17,7 @@ struct intel_encoder;
#define MAX_CHANNEL_COUNT 8
void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 6a4d477e8a15..daaa08c0ee47 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2402,6 +2402,8 @@ int intel_hdmi_compute_config(struct intel_encoder
*encoder,
return -EINVAL;
}
+ intel_audio_compute_eld(pipe_config);
+
return 0;
}
--
2.25.1
Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Hi Kai,
> -Original Message-
> From: Kai Vehmanen
> Sent: 19 June 2023 16:50
> To: Golani, Mitulkumar Ajitkumar
> Cc: intel-gfx@lists.freedesktop.org; jyri.sa...@linux.intel.com
> Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute
> SAD
>
> Hi,
>
> [+Jyri]
>
> On Fri, 9 Jun 2023, Mitul Golani wrote:
>
> > Compute SADs that takes into account the supported rate and channel
> > based on the capabilities of the audio source. This wrapper function
> > should encapsulate the logic for determining the supported rate and
> > channel and should return a set of SADs that are compatible with the
> > source.
>
> In general looks good. A few minor comments inline:
>
> > +static u8 get_supported_freq_mask(struct intel_crtc_state
> > +*crtc_state) {
> > + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> 192000, 0};
> > + u8 mask = 0;
> > +
> > + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
>
> Minor nitpick: the use of "u8" in many places seems a bit misleading. It
> seems for many places (like the "index" here), you can just use int.
> But right, the SAD mask is 8bit, so maybe the get_support_freq_mask() is still
> warranted to return a u8 mask.
Thanks for inputs. Few more places where I could have avoided using u8. I will
push the correction with new revision.
>
> > +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state) {
> > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > + u8 *eld, *sad, index, mask = 0;
> > +
> > + eld = crtc_state->eld;
> > + if (!eld) {
> > + drm_err(>drm, "failed to locate eld\n");
> > + return;
> > + }
> > +
> > + sad = (u8 *)parse_sad(eld);
> > + if (sad) {
> > + mask = get_supported_freq_mask(crtc_state);
> > +
> > + for (index = 0; index < drm_eld_sad_count(eld); index++, sad
> += 3) {
> > + /*
> > +* Respect to source restrictions. If source limit is
> greater than sink
> > +* capabilities then follow to sink's highest supported
> rate.
> > +*/
>
> Minor: maybe reword "Respect source restricitions. Limit capabilities to a
> subset that is supported both by the source and the sink."?
Thanks for inputs. Few more places where I could have avoided using u8. I will
push the correction with new revision.
>
> > + if (drm_sad_to_channels(sad) >= crtc_state-
> >audio.max_channel) {
> > + sad[0] &= ~0x7;
> > + sad[0] |= crtc_state->audio.max_channel - 1;
>
> Can we add a debug trace here in case the channel count is limited?
>
> Br, Kai
Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Hi,
[+Jyri]
On Fri, 9 Jun 2023, Mitul Golani wrote:
> Compute SADs that takes into account the supported rate and channel
> based on the capabilities of the audio source. This wrapper function
> should encapsulate the logic for determining the supported rate and
> channel and should return a set of SADs that are compatible with the
> source.
In general looks good. A few minor comments inline:
> +static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
> +{
> + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> 192000, 0};
> + u8 mask = 0;
> +
> + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
Minor nitpick: the use of "u8" in many places seems a bit misleading. It
seems for many places (like the "index" here), you can just use int.
But right, the SAD mask is 8bit, so maybe the get_support_freq_mask()
is still warranted to return a u8 mask.
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> + u8 *eld, *sad, index, mask = 0;
> +
> + eld = crtc_state->eld;
> + if (!eld) {
> + drm_err(>drm, "failed to locate eld\n");
> + return;
> + }
> +
> + sad = (u8 *)parse_sad(eld);
> + if (sad) {
> + mask = get_supported_freq_mask(crtc_state);
> +
> + for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
> 3) {
> + /*
> + * Respect to source restrictions. If source limit is
> greater than sink
> + * capabilities then follow to sink's highest
> supported rate.
> + */
Minor: maybe reword "Respect source restricitions. Limit capabilities to a
subset that is supported both by the source and the sink."?
> + if (drm_sad_to_channels(sad) >=
> crtc_state->audio.max_channel) {
> + sad[0] &= ~0x7;
> + sad[0] |= crtc_state->audio.max_channel - 1;
Can we add a debug trace here in case the channel count is limited?
Br, Kai
Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Hello Kai,
> -Original Message-
> From: Golani, Mitulkumar Ajitkumar
>
> Sent: Thursday, June 15, 2023 12:37 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Shankar, Uma ; Borah, Chaitanya Kumar
> ; Golani, Mitulkumar Ajitkumar
> ; Nautiyal, Ankit K
>
> Subject: [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
>
> Compute SADs that takes into account the supported rate and channel based
> on the capabilities of the audio source. This wrapper function should
> encapsulate the logic for determining the supported rate and channel and
> should return a set of SADs that are compatible with the source.
>
> --v1:
> - call intel_audio_compute_eld in this commit as it is defined here
>
> Signed-off-by: Mitul Golani
> ---
> drivers/gpu/drm/i915/display/intel_audio.c | 66 ++
> drivers/gpu/drm/i915/display/intel_audio.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> 3 files changed, 69 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> b/drivers/gpu/drm/i915/display/intel_audio.c
> index e20ffc8e9654..a6a58b0f0717 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -794,6 +794,72 @@ bool intel_audio_compute_config(struct
> intel_encoder *encoder,
> return true;
> }
>
> +static unsigned int drm_sad_to_channels(const u8 *sad) {
> + return 1 + (sad[0] & 0x7);
> +}
> +
> +static inline u8 *parse_sad(u8 *eld)
> +{
> + unsigned int ver, mnl;
> +
> + ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >>
> DRM_ELD_VER_SHIFT;
> + if (ver != 2 && ver != 31)
> + return NULL;
> +
> + mnl = drm_eld_mnl(eld);
> + if (mnl > 16)
> + return NULL;
> +
> + return eld + DRM_ELD_CEA_SAD(mnl, 0);
> +}
> +
> +static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
> +{
> + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> 192000, 0};
> + u8 mask = 0;
> +
> + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
> + mask |= 1 << index;
> + if (crtc_state->audio.max_frequency != audio_freq_hz[index])
> + continue;
> + else
> + break;
> + }
> +
> + return mask;
> +}
> +
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state) {
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> + u8 *eld, *sad, index, mask = 0;
> +
> + eld = crtc_state->eld;
> + if (!eld) {
> + drm_err(>drm, "failed to locate eld\n");
> + return;
> + }
> +
> + sad = (u8 *)parse_sad(eld);
> + if (sad) {
> + mask = get_supported_freq_mask(crtc_state);
> +
> + for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
> 3) {
> + /*
> + * Respect to source restrictions. If source limit is
> greater than sink
> + * capabilities then follow to sink's highest supported
> rate.
> + */
> + if (drm_sad_to_channels(sad) >= crtc_state-
> >audio.max_channel) {
> + sad[0] &= ~0x7;
> + sad[0] |= crtc_state->audio.max_channel - 1;
> + }
> +
> + sad[1] &= mask;
We would like to hear your opinion on this implementation from audio driver
perspective.
Regards
Chaitanya
> + }
> + }
> +}
> +
> /**
> * intel_audio_codec_enable - Enable the audio codec for HD audio
> * @encoder: encoder on which to enable audio diff --git
> a/drivers/gpu/drm/i915/display/intel_audio.h
> b/drivers/gpu/drm/i915/display/intel_audio.h
> index 07d034a981e9..2ec7fafd9711 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> @@ -14,6 +14,7 @@ struct intel_crtc_state; struct intel_encoder;
>
> void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
> bool intel_audio_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state *conn_state); diff
> --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 0188a600f9f5..beafeff494f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2403,6 +2403,8 @@ int intel_hdmi_compute_config(struct
> intel_encoder *encoder,
> return -EINVAL;
> }
>
> + intel_audio_compute_eld(pipe_config);
> +
> return 0;
> }
>
> --
> 2.25.1
Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Hi Chaitanya,
> -Original Message-
> From: Borah, Chaitanya Kumar
> Sent: 15 June 2023 09:30
> To: Golani, Mitulkumar Ajitkumar ;
> intel-gfx@lists.freedesktop.org
> Cc: Shankar, Uma ; Nautiyal, Ankit K
>
> Subject: RE: [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
>
> Hello Mitul,
>
> > -Original Message-
> > From: Golani, Mitulkumar Ajitkumar
> >
> > Sent: Friday, June 9, 2023 11:12 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Shankar, Uma ; Borah, Chaitanya Kumar
> > ; Golani, Mitulkumar Ajitkumar
> > ; Nautiyal, Ankit K
> >
> > Subject: [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
> >
> > Compute SADs that takes into account the supported rate and channel
> > based on the capabilities of the audio source. This wrapper function
> > should encapsulate the logic for determining the supported rate and
> > channel and should return a set of SADs that are compatible with the
> source.
> >
> > --v1:
> > - call intel_audio_compute_eld in this commit as it is defined here
> >
> > Signed-off-by: Mitul Golani
> > ---
> > drivers/gpu/drm/i915/display/intel_audio.c | 66
> > ++ drivers/gpu/drm/i915/display/intel_audio.h | 1
> > + drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> > 3 files changed, 69 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index e20ffc8e9654..a6a58b0f0717 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -794,6 +794,72 @@ bool intel_audio_compute_config(struct
> > intel_encoder *encoder,
> > return true;
> > }
> >
> > +static unsigned int drm_sad_to_channels(const u8 *sad) {
> > + return 1 + (sad[0] & 0x7);
> > +}
> > +
>
> We can do away with the drm_ prefix here.
Thanks for pointing out. Somehow missed while migrating. Pushed fix with new
revision.
>
> > +static inline u8 *parse_sad(u8 *eld)
> > +{
>
> Nit: eld_to_sad() could be a better name here.
Sure. Corrected in new revision set.
>
> > + unsigned int ver, mnl;
> > +
> > + ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >>
> > DRM_ELD_VER_SHIFT;
> > + if (ver != 2 && ver != 31)
> > + return NULL;
> > +
> > + mnl = drm_eld_mnl(eld);
> > + if (mnl > 16)
> > + return NULL;
> > +
> > + return eld + DRM_ELD_CEA_SAD(mnl, 0); }
> > +
> > +static u8 get_supported_freq_mask(struct intel_crtc_state
> > +*crtc_state) {
> > + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> > 192000, 0};
>
> Please check if we really need this trailing 0 here.
>
> To cover the case where the maximum rate is set to 0Hz(init value) we can
> have a check of
>
> if (crtc_state->audio.max_frequency < 32000)
>
>
> Regards
>
> Chaitanya
Right Good catch. It would have sent 0xff in that case. Corrected and sent with
new revision.
Thanks,
Mitul
>
> > + u8 mask = 0;
> > +
> > + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
> > + mask |= 1 << index;
> > + if (crtc_state->audio.max_frequency !=
> audio_freq_hz[index])
> > + continue;
> > + else
> > + break;
> > + }
> > +
> > + return mask;
> > +}
> > +
> > +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state) {
> > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > + u8 *eld, *sad, index, mask = 0;
> > +
> > + eld = crtc_state->eld;
> > + if (!eld) {
> > + drm_err(>drm, "failed to locate eld\n");
> > + return;
> > + }
> > +
> > + sad = (u8 *)parse_sad(eld);
> > + if (sad) {
> > + mask = get_supported_freq_mask(crtc_state);
> > +
> > + for (index = 0; index < drm_eld_sad_count(eld); index++, sad
> = 3) {
> > + /*
> > +* Respect to source restrictions. If source limit is
> > greater than sink
> > +* capabilities then follow to sink's highest supported
> > rate.
> > +*/
> > + if (drm_sad_to_channels(sad) >= crtc_state-
> > >audio.max_channel) {
> > + sad[0] &= ~0x7;
> > + sad[0] |= crtc_state->audio.max_channel - 1;
> > + }
> > +
> > + sad[1] &= mask;
> > + }
> > + }
> > +}
> > +
> > /**
> > * intel_audio_codec_enable - Enable the audio codec for HD audio
> > * @encoder: encoder on which to enable audio diff --git
> > a/drivers/gpu/drm/i915/display/intel_audio.h
> > b/drivers/gpu/drm/i915/display/intel_audio.h
> > index 07d034a981e9..2ec7fafd9711 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.h
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> > @@ -14,6 +14,7 @@ struct intel_crtc_state; struct intel_encoder;
> >
> > void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
> > +void intel_audio_compute_eld(struct
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate and channel
based on the capabilities of the audio source. This wrapper function
should encapsulate the logic for determining the supported rate and
channel and should return a set of SADs that are compatible with the
source.
--v1:
- call intel_audio_compute_eld in this commit as it is defined here
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 66 ++
drivers/gpu/drm/i915/display/intel_audio.h | 1 +
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
3 files changed, 69 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index e20ffc8e9654..a6a58b0f0717 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -794,6 +794,72 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
return true;
}
+static unsigned int drm_sad_to_channels(const u8 *sad)
+{
+ return 1 + (sad[0] & 0x7);
+}
+
+static inline u8 *parse_sad(u8 *eld)
+{
+ unsigned int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+ int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
192000, 0};
+ u8 mask = 0;
+
+ for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
+ mask |= 1 << index;
+ if (crtc_state->audio.max_frequency != audio_freq_hz[index])
+ continue;
+ else
+ break;
+ }
+
+ return mask;
+}
+
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+ u8 *eld, *sad, index, mask = 0;
+
+ eld = crtc_state->eld;
+ if (!eld) {
+ drm_err(>drm, "failed to locate eld\n");
+ return;
+ }
+
+ sad = (u8 *)parse_sad(eld);
+ if (sad) {
+ mask = get_supported_freq_mask(crtc_state);
+
+ for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
3) {
+ /*
+* Respect to source restrictions. If source limit is
greater than sink
+* capabilities then follow to sink's highest
supported rate.
+*/
+ if (drm_sad_to_channels(sad) >=
crtc_state->audio.max_channel) {
+ sad[0] &= ~0x7;
+ sad[0] |= crtc_state->audio.max_channel - 1;
+ }
+
+ sad[1] &= mask;
+ }
+ }
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @encoder: encoder on which to enable audio
diff --git a/drivers/gpu/drm/i915/display/intel_audio.h
b/drivers/gpu/drm/i915/display/intel_audio.h
index 07d034a981e9..2ec7fafd9711 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.h
+++ b/drivers/gpu/drm/i915/display/intel_audio.h
@@ -14,6 +14,7 @@ struct intel_crtc_state;
struct intel_encoder;
void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 0188a600f9f5..beafeff494f8 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2403,6 +2403,8 @@ int intel_hdmi_compute_config(struct intel_encoder
*encoder,
return -EINVAL;
}
+ intel_audio_compute_eld(pipe_config);
+
return 0;
}
--
2.25.1
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate and channel
based on the capabilities of the audio source. This wrapper function
should encapsulate the logic for determining the supported rate and
channel and should return a set of SADs that are compatible with the
source.
--v1:
- call intel_audio_compute_eld in this commit as it is defined here
--v2:
- Handle case when max frequency is less than 32k.
- remove drm prefix.
- name change for parse_sad to eld_to_sad.
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 69 ++
drivers/gpu/drm/i915/display/intel_audio.h | 1 +
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
3 files changed, 72 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index e20ffc8e9654..335bfce18994 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -794,6 +794,75 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
return true;
}
+static unsigned int sad_to_channels(const u8 *sad)
+{
+ return 1 + (sad[0] & 0x7);
+}
+
+static inline u8 *eld_to_sad(u8 *eld)
+{
+ unsigned int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+ int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
192000};
+ u8 mask = 0;
+
+ if(crtc_state->audio.max_frequency < 32000)
+ return mask;
+
+ for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
+ mask |= 1 << index;
+ if (crtc_state->audio.max_frequency != audio_freq_hz[index])
+ continue;
+ else
+ break;
+ }
+
+ return mask;
+}
+
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+ u8 *eld, *sad, index, mask = 0;
+
+ eld = crtc_state->eld;
+ if (!eld) {
+ drm_err(>drm, "failed to locate eld\n");
+ return;
+ }
+
+ sad = (u8 *)eld_to_sad(eld);
+ if (sad) {
+ mask = get_supported_freq_mask(crtc_state);
+
+ for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
3) {
+ /*
+* Respect to source restrictions. If source limit is
greater than sink
+* capabilities then follow to sink's highest
supported rate.
+*/
+ if (sad_to_channels(sad) >=
crtc_state->audio.max_channel) {
+ sad[0] &= ~0x7;
+ sad[0] |= crtc_state->audio.max_channel - 1;
+ }
+
+ sad[1] &= mask;
+ }
+ }
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @encoder: encoder on which to enable audio
diff --git a/drivers/gpu/drm/i915/display/intel_audio.h
b/drivers/gpu/drm/i915/display/intel_audio.h
index 07d034a981e9..2ec7fafd9711 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.h
+++ b/drivers/gpu/drm/i915/display/intel_audio.h
@@ -14,6 +14,7 @@ struct intel_crtc_state;
struct intel_encoder;
void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 0188a600f9f5..beafeff494f8 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2403,6 +2403,8 @@ int intel_hdmi_compute_config(struct intel_encoder
*encoder,
return -EINVAL;
}
+ intel_audio_compute_eld(pipe_config);
+
return 0;
}
--
2.25.1
Re: [Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Hello Mitul,
> -Original Message-
> From: Golani, Mitulkumar Ajitkumar
> Sent: Friday, June 9, 2023 11:12 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Shankar, Uma ; Borah, Chaitanya Kumar
> ; Golani, Mitulkumar Ajitkumar
> ; Nautiyal, Ankit K
>
> Subject: [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
>
> Compute SADs that takes into account the supported rate and channel based
> on the capabilities of the audio source. This wrapper function should
> encapsulate the logic for determining the supported rate and channel and
> should return a set of SADs that are compatible with the source.
>
> --v1:
> - call intel_audio_compute_eld in this commit as it is defined here
>
> Signed-off-by: Mitul Golani
> ---
> drivers/gpu/drm/i915/display/intel_audio.c | 66 ++
> drivers/gpu/drm/i915/display/intel_audio.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> 3 files changed, 69 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> b/drivers/gpu/drm/i915/display/intel_audio.c
> index e20ffc8e9654..a6a58b0f0717 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -794,6 +794,72 @@ bool intel_audio_compute_config(struct
> intel_encoder *encoder,
> return true;
> }
>
> +static unsigned int drm_sad_to_channels(const u8 *sad) {
> + return 1 + (sad[0] & 0x7);
> +}
> +
We can do away with the drm_ prefix here.
> +static inline u8 *parse_sad(u8 *eld)
> +{
Nit: eld_to_sad() could be a better name here.
> + unsigned int ver, mnl;
> +
> + ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >>
> DRM_ELD_VER_SHIFT;
> + if (ver != 2 && ver != 31)
> + return NULL;
> +
> + mnl = drm_eld_mnl(eld);
> + if (mnl > 16)
> + return NULL;
> +
> + return eld + DRM_ELD_CEA_SAD(mnl, 0);
> +}
> +
> +static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
> +{
> + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> 192000, 0};
Please check if we really need this trailing 0 here.
To cover the case where the maximum rate is set to 0Hz(init value) we can have
a check of
if (crtc_state->audio.max_frequency < 32000)
Regards
Chaitanya
> + u8 mask = 0;
> +
> + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
> + mask |= 1 << index;
> + if (crtc_state->audio.max_frequency != audio_freq_hz[index])
> + continue;
> + else
> + break;
> + }
> +
> + return mask;
> +}
> +
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state) {
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> + u8 *eld, *sad, index, mask = 0;
> +
> + eld = crtc_state->eld;
> + if (!eld) {
> + drm_err(>drm, "failed to locate eld\n");
> + return;
> + }
> +
> + sad = (u8 *)parse_sad(eld);
> + if (sad) {
> + mask = get_supported_freq_mask(crtc_state);
> +
> + for (index = 0; index < drm_eld_sad_count(eld); index++, sad
> += 3) {
> + /*
> + * Respect to source restrictions. If source limit is
> greater than sink
> + * capabilities then follow to sink's highest supported
> rate.
> + */
> + if (drm_sad_to_channels(sad) >= crtc_state-
> >audio.max_channel) {
> + sad[0] &= ~0x7;
> + sad[0] |= crtc_state->audio.max_channel - 1;
> + }
> +
> + sad[1] &= mask;
> + }
> + }
> +}
> +
> /**
> * intel_audio_codec_enable - Enable the audio codec for HD audio
> * @encoder: encoder on which to enable audio diff --git
> a/drivers/gpu/drm/i915/display/intel_audio.h
> b/drivers/gpu/drm/i915/display/intel_audio.h
> index 07d034a981e9..2ec7fafd9711 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> @@ -14,6 +14,7 @@ struct intel_crtc_state; struct intel_encoder;
>
> void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
> bool intel_audio_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state *conn_state); diff
> --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 0188a600f9f5..beafeff494f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2403,6 +2403,8 @@ int intel_hdmi_compute_config(struct intel_encoder
> *encoder,
> return -EINVAL;
> }
>
> + intel_audio_compute_eld(pipe_config);
> +
> return 0;
> }
>
> --
> 2.25.1
[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Compute SADs that takes into account the supported rate and channel
based on the capabilities of the audio source. This wrapper function
should encapsulate the logic for determining the supported rate and
channel and should return a set of SADs that are compatible with the
source.
--v1:
- call intel_audio_compute_eld in this commit as it is defined here
Signed-off-by: Mitul Golani
---
drivers/gpu/drm/i915/display/intel_audio.c | 66 ++
drivers/gpu/drm/i915/display/intel_audio.h | 1 +
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
3 files changed, 69 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
b/drivers/gpu/drm/i915/display/intel_audio.c
index e20ffc8e9654..a6a58b0f0717 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -794,6 +794,72 @@ bool intel_audio_compute_config(struct intel_encoder
*encoder,
return true;
}
+static unsigned int drm_sad_to_channels(const u8 *sad)
+{
+ return 1 + (sad[0] & 0x7);
+}
+
+static inline u8 *parse_sad(u8 *eld)
+{
+ unsigned int ver, mnl;
+
+ ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+ if (ver != 2 && ver != 31)
+ return NULL;
+
+ mnl = drm_eld_mnl(eld);
+ if (mnl > 16)
+ return NULL;
+
+ return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
+{
+ int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
192000, 0};
+ u8 mask = 0;
+
+ for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
+ mask |= 1 << index;
+ if (crtc_state->audio.max_frequency != audio_freq_hz[index])
+ continue;
+ else
+ break;
+ }
+
+ return mask;
+}
+
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+ u8 *eld, *sad, index, mask = 0;
+
+ eld = crtc_state->eld;
+ if (!eld) {
+ drm_err(>drm, "failed to locate eld\n");
+ return;
+ }
+
+ sad = (u8 *)parse_sad(eld);
+ if (sad) {
+ mask = get_supported_freq_mask(crtc_state);
+
+ for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
3) {
+ /*
+* Respect to source restrictions. If source limit is
greater than sink
+* capabilities then follow to sink's highest
supported rate.
+*/
+ if (drm_sad_to_channels(sad) >=
crtc_state->audio.max_channel) {
+ sad[0] &= ~0x7;
+ sad[0] |= crtc_state->audio.max_channel - 1;
+ }
+
+ sad[1] &= mask;
+ }
+ }
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @encoder: encoder on which to enable audio
diff --git a/drivers/gpu/drm/i915/display/intel_audio.h
b/drivers/gpu/drm/i915/display/intel_audio.h
index 07d034a981e9..2ec7fafd9711 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.h
+++ b/drivers/gpu/drm/i915/display/intel_audio.h
@@ -14,6 +14,7 @@ struct intel_crtc_state;
struct intel_encoder;
void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
+void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
bool intel_audio_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 0188a600f9f5..beafeff494f8 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2403,6 +2403,8 @@ int intel_hdmi_compute_config(struct intel_encoder
*encoder,
return -EINVAL;
}
+ intel_audio_compute_eld(pipe_config);
+
return 0;
}
--
2.25.1
