[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD

Thu Jun 15 07:09:45 UTC 2023

Hi Chaitanya,

> -----Original Message-----
> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>
> Sent: 15 June 2023 09:30
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>;
> intel-gfx at lists.freedesktop.org
> Cc: Shankar, Uma <uma.shankar at intel.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal at intel.com>
> Subject: RE: [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD
> 
> Hello Mitul,
> 
> > -----Original Message-----
> > From: Golani, Mitulkumar Ajitkumar
> > <mitulkumar.ajitkumar.golani at intel.com>
> > Sent: Friday, June 9, 2023 11:12 PM
> > To: intel-gfx at lists.freedesktop.org
> > Cc: Shankar, Uma <uma.shankar at intel.com>; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah at intel.com>; Golani, Mitulkumar Ajitkumar
> > <mitulkumar.ajitkumar.golani at intel.com>; Nautiyal, Ankit K
> > <ankit.k.nautiyal at intel.com>
> > 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 <mitulkumar.ajitkumar.golani at intel.com>
> > ---
> >  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(&i915->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