Hi Kai,
> -----Original Message-----
> From: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Sent: 19 June 2023 16:50
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; jyri.sarha@xxxxxxxxxxxxxxx
> Subject: Re: [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
