[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
Golani, Mitulkumar Ajitkumar
mitulkumar.ajitkumar.golani at intel.com
Wed Jun 28 16:43:15 UTC 2023
Hi @Jani Nikula
> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: 26 June 2023 22:48
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>;
> intel-gfx at 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 at 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 at 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
More information about the Intel-gfx
mailing list