| Message ID | 20230626163819.2759500-4-mitulkumar.ajitkumar.golani@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Get optimal audio frequency and channels | expand |
On Mon, 26 Jun 2023, Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> 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 <mitulkumar.ajitkumar.golani@intel.com> > --- > 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(&i915->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(&i915->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_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; > }
Hi @Jani Nikula > -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: 26 June 2023 22:48 > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>; > 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 <mitulkumar.ajitkumar.golani@intel.com> > 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 <mitulkumar.ajitkumar.golani@intel.com> > > --- > > 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(&i915->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(&i915->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_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; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center
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(&i915->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(&i915->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; }
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 <mitulkumar.ajitkumar.golani@intel.com> --- 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(+)