Re: [PATCH 1/3] drm/i915: Clean up hsw_audio_codec_enable()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 11 Aug 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Aug 11, 2016 at 10:47:06AM +0300, Jani Nikula wrote:
>> On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 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@chris-wilson.co.uk>
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > ---
>> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx