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

[Golani, Mitulkumar Ajitkumar]

Hi Kai,

> -----Original Message-----
> From: Kai Vehmanen <kai.vehmanen at linux.intel.com>
> Sent: 19 June 2023 16:50
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; jyri.sarha at 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(&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.
> > +			 */
> 
> 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