Hi @Jani Nikula > -----Original Message----- > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Sent: 26 June 2023 22:48 > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@xxxxxxxxx>; > intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [RFC 3/3] drm/i915/display: Add wrapper to Compute > SAD > > On Mon, 26 Jun 2023, Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx> > 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@xxxxxxxxx> > > --- > > 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