On Thu, 11 Aug 2016, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> 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.
*shrug* I think the separate messages on errors stand out better.
J.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
