On Thu, Aug 11, 2016 at 10:47:06AM +0300, Jani Nikula wrote:
> On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> wrote:
> > No functional change, code clean up and improved debug.
> >
> > Chris suggested this code snippet while reviewing, I just made this into a
> > patch.
> >
> > Credits-to: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_audio.c | 28 ++++++++++++----------------
> > 1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index d32f586..3efce0e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -282,14 +282,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
> > struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > enum pipe pipe = intel_crtc->pipe;
> > - struct i915_audio_component *acomp = dev_priv->audio_component;
> > const uint8_t *eld = connector->eld;
> > - struct intel_digital_port *intel_dig_port =
> > - enc_to_dig_port(&encoder->base);
> > - enum port port = intel_dig_port->port;
> > uint32_t tmp;
> > int len, i;
> > - int n, rate;
> >
> > DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
> > pipe_name(pipe), drm_eld_size(eld));
> > @@ -335,19 +330,20 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
> >
> > tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > if (audio_rate_need_prog(intel_crtc, adjusted_mode)) {
> > - if (!acomp)
> > - rate = 0;
> > - else if (port >= PORT_A && port <= PORT_E)
> > + enum port port = enc_to_dig_port(&encoder->base)->port;
> > + struct i915_audio_component *acomp = dev_priv->audio_component;
> > + int rate, n;
>
> Moving these here is probably fine, although originally I was hoping we
> would always program the N, and thus wouldn't have this if
> (audio_rate_need_prog(intel_crtc, adjusted_mode)) block here at all. The
> idea was that we'd always use the same method instead of letting the hw
> set it automatically for some rates, and manually setting others.
>
> > +
> > + rate = 0;
> > + if (acomp && port >= PORT_A && port <= PORT_E) {
> > rate = acomp->aud_sample_rate[port];
> > - else {
> > - DRM_ERROR("invalid port: %d\n", port);
>
> I think I may have seen these in some dmesgs in otherwise unrelated
> bugs. I'd like to preserve this at least as a debug breadcrumb.
>
> > - rate = 0;
> > +
> > + n = audio_config_get_n(adjusted_mode, rate);
> > + DRM_DEBUG_KMS("port %d audio rate %d => N=%x\n",
> > + port, rate, n);
> > + if (n)
> > + tmp = audio_config_setup_n_reg(n, tmp);
> > }
> > - n = audio_config_get_n(adjusted_mode, rate);
> > - if (n != 0)
> > - tmp = audio_config_setup_n_reg(n, tmp);
> > - else
> > - DRM_DEBUG_KMS("no suitable N value is found\n");
>
> Same here, I'd like to preserve the debug.
The information is still there (both invalid port and unfound n), just as
a single debug.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
