Hi @Jani Nikula

Thanks for reviewing patch series.

> -----Original Message-----
> From: Jani Nikula <jani.nik...@linux.intel.com>
> Sent: 18 August 2023 14:08
> To: Golani, Mitulkumar Ajitkumar
> <mitulkumar.ajitkumar.gol...@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: jyri.sa...@linux.intel.com
> Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915/display: Add wrapper to
> Compute SAD
> 
> On Thu, 17 Aug 2023, Mitul Golani
> <mitulkumar.ajitkumar.gol...@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.
> >
> > --v4:
> > - remove inline from eld_to_sad.
> > - declare index outside of for loop with int type.
> > - Correct mask value calculation.
> > - remove drm_err, instead just return if eld parsing failed.
> > - remove unncessary typecast
> > - reduce indentation while parsing sad
> > - use intel_audio_compute_eld as static and call bandwidth calculation
> > just before that.
> >
> > --v9:
> > - Handling the case when, sink supported channel is less than max
> > supported. In that case, rate needs to be calibrate in accordance with
> > available bandwidth.
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.gol...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_audio.c | 107
> > ++++++++++++++++++++-
> >  1 file changed, 106 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index 79377e33a59b..c90ac2608eef 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -806,6 +806,111 @@ static void calc_audio_config_params(struct
> intel_crtc_state *pipe_config)
> >     pipe_config->audio.max_channel_count = 0;  }
> >
> 
> This was added in previous patch:
> 
>  +static void calc_audio_config_params(struct intel_crtc_state *pipe_config)
> +{
>  +    struct drm_display_mode *adjusted_mode = &pipe_config-
> >hw.adjusted_mode;
>  +    int channel_count;
>  +    int index, rate[] = { 192000, 176000, 96000, 88000, 48000, 44100,
> 32000 };
>  +    int audio_req_bandwidth, available_blank_bandwidth, vblank,
> hblank;
>  +
>  +    hblank = adjusted_mode->htotal - adjusted_mode->hdisplay;
>  +    vblank = adjusted_mode->vtotal - adjusted_mode->vdisplay;
>  +    available_blank_bandwidth = hblank * vblank *
>  +                                drm_mode_vrefresh(adjusted_mode) *
> pipe_config->pipe_bpp;
>  +    for (channel_count = MAX_CHANNEL_COUNT; channel_count > 0;
> channel_count--) {
>  +            for (index = 0; index < ARRAY_SIZE(rate); index++) {
>  +                    audio_req_bandwidth =
> calc_audio_bw(channel_count,
>  +                                                        rate[index]);
>  +                    if (audio_req_bandwidth <
> available_blank_bandwidth) {
>  +                            pipe_config->audio.max_rate = rate[index];
>  +                            pipe_config->audio.max_channel_count =
> channel_count;
>  +                            return;
>  +                    }
>  +            }
>  +    }
>  +
>  +    pipe_config->audio.max_rate = 0;
>  +    pipe_config->audio.max_channel_count = 0;
>  +}
> 
> And this here:

Thanks I have removed duplicated code for calibration and calculation,
and merged them to one to avoid redundancy.

> 
> > +static void calibrate_audio_config_params(struct intel_crtc_state
> > +*pipe_config, int channel) {
> > +   struct drm_display_mode *adjusted_mode = &pipe_config-
> >hw.adjusted_mode;
> > +   int channel_count;
> > +   int index, rate[] = { 192000, 176000, 96000, 88000, 48000, 44100,
> 32000 };
> > +   int audio_req_bandwidth, available_blank_bandwidth, vblank,
> hblank;
> > +
> > +   hblank = adjusted_mode->htotal - adjusted_mode->hdisplay;
> > +   vblank = adjusted_mode->vtotal - adjusted_mode->vdisplay;
> > +   available_blank_bandwidth = hblank * vblank *
> > +                               drm_mode_vrefresh(adjusted_mode) *
> pipe_config->pipe_bpp;
> > +
> > +   for (index = 0; index < ARRAY_SIZE(rate); index++) {
> > +           audio_req_bandwidth = calc_audio_bw(channel_count,
> > +                                               rate[index]);
> > +           if (audio_req_bandwidth < available_blank_bandwidth) {
> > +                   pipe_config->audio.max_rate = rate[index];
> > +                   pipe_config->audio.max_channel_count =
> channel_count;
> > +                   return;
> > +           }
> > +   }
> > +
> > +   pipe_config->audio.max_rate = 0;
> > +   pipe_config->audio.max_channel_count = 0; }
> 
> This kind of stuff needs to be deduplicated.
> 
> > +
> > +static int sad_to_channels(const u8 *sad) {
> > +   return 1 + (sad[0] & 0x7);
> > +}
> > +
> > +static u8 *eld_to_sad(u8 *eld)
> > +{
> > +   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); }
> 
> I'm still not happy with the copy-paste here. The parsing details should be
> hidden in drm_edid.c and that should be the Single Point of Truth how to
> deal with this.
> 

Thanks, I have added  wrapper function adhering to
const correctness while accessing SAD information. Please suggest 
if any other approach we can try.

> > +
> > +static int get_supported_freq_mask(struct intel_crtc_state
> > +*crtc_state) {
> > +   int rate[] = { 32000, 44100, 48000, 88000, 96000, 176000, 192000 };
> 
> This is like the third copy of the same array already... and the values are
> also present in the existing dp/hdmi aud arrays in intel_audio.c.
> 
> And those have 88200 instead of 88000, so which one is correct?

This was mistake while validating previous patches. I have corrected
the same with reference to HDMI spec.

> 
> > +   int mask = 0, index;
> > +
> > +   for (index = 0; index < ARRAY_SIZE(rate); index++) {
> > +           if (rate[index] > crtc_state->audio.max_rate)
> > +                   break;
> > +
> > +           mask |= 1 << index;
> > +
> > +           if (crtc_state->audio.max_rate != rate[index])
> > +                   continue;
> > +   }
> > +
> > +   return mask;
> > +}
> > +
> > +static 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)
> > +           return;
> > +
> > +   sad = eld_to_sad(eld);
> > +   if (!sad)
> > +           return;
> > +
> > +   calc_audio_config_params(crtc_state);
> > +
> > +   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_count) {
> > +                   sad[0] &= ~0x7;
> 
> Also not happy with magic values here.

Thanks, defined the magic value.

Regards,
Mitul

> 
> > +                   sad[0] |= crtc_state->audio.max_channel_count - 1;
> > +                   drm_dbg_kms(&i915->drm, "Channel count is
> limited to %d\n",
> > +                               crtc_state->audio.max_channel_count - 1);
> > +           } else {
> > +                   /*
> > +                    * calibrate rate when, sink supported channel
> > +                    * count is slight less than max supported
> > +                    * channel count.
> > +                    */
> > +                   calibrate_audio_config_params(crtc_state,
> sad_to_channels(sad));
> > +                   mask = get_supported_freq_mask(crtc_state);
> > +           }
> > +
> > +           sad[1] &= mask;
> > +   }
> > +}
> > +
> >  bool intel_audio_compute_config(struct intel_encoder *encoder,
> >                             struct intel_crtc_state *crtc_state,
> >                             struct drm_connector_state *conn_state)
> @@ -827,7 +932,7 @@ bool
> > intel_audio_compute_config(struct intel_encoder *encoder,
> >
> >     crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode)
> /
> > 2;
> >
> > -   calc_audio_config_params(crtc_state);
> > +   intel_audio_compute_eld(crtc_state);
> >
> >     return true;
> >  }
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

Reply via email to