| Message ID | 1470897673-29292-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 10, 2016 at 11:41:11PM -0700, Dhinakaran Pandiyan 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> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Probably worth getting a second opinion on this. -Chris
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. > } > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
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. -Chris
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.
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; + + rate = 0; + if (acomp && port >= PORT_A && port <= PORT_E) { rate = acomp->aud_sample_rate[port]; - else { - DRM_ERROR("invalid port: %d\n", port); - 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"); } I915_WRITE(HSW_AUD_CFG(pipe), tmp);
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(-)