[PATCH] drm/i915: Fix braces in conditonal branches

[PATCH] drm/i915: Fix braces in conditonal branches

[Dhinakaran Pandiyan]

No functional change, just adding braces to all branches of conditional
statement because one of them already had.
---
 drivers/gpu/drm/i915/intel_audio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index d32f586..26a795f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -335,11 +335,11 @@ 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)
+		if (!acomp) {
 			rate = 0;
-		else if (port >= PORT_A && port <= PORT_E)
+		} else if (port >= PORT_A && port <= PORT_E) {
 			rate = acomp->aud_sample_rate[port];
-		else {
+		} else {
 			DRM_ERROR("invalid port: %d\n", port);
 			rate = 0;
 		}
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix braces in conditonal branches

[Chris Wilson]

On Tue, Aug 09, 2016 at 03:06:10PM -0700, Dhinakaran Pandiyan wrote:
> No functional change, just adding braces to all branches of conditional
> statement because one of them already had.
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index d32f586..26a795f 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -335,11 +335,11 @@ 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)
> +		if (!acomp) {
>  			rate = 0;
> -		else if (port >= PORT_A && port <= PORT_E)
> +		} else if (port >= PORT_A && port <= PORT_E) {
>  			rate = acomp->aud_sample_rate[port];
> -		else {
> +		} else {
>  			DRM_ERROR("invalid port: %d\n", port);
>  			rate = 0;
>  		}

Or you could improve scoping of the locals and eliminate a few lines
whilst adding more information to the debug:

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index d32f586..98d4576 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,18 @@ 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);
-               if (n != 0)
+               DRM_DEBUG_KMS("port %d audio rate %d => N=%x\n", port, rate, n);
+               if (n)
                        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);


-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix braces in conditonal branches

[Pandiyan, Dhinakaran]

On Tue, 2016-08-09 at 23:08 +0100, Chris Wilson wrote:
> On Tue, Aug 09, 2016 at 03:06:10PM -0700, Dhinakaran Pandiyan wrote:
> > No functional change, just adding braces to all branches of conditional
> > statement because one of them already had.
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index d32f586..26a795f 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -335,11 +335,11 @@ 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)
> > +		if (!acomp) {
> >  			rate = 0;
> > -		else if (port >= PORT_A && port <= PORT_E)
> > +		} else if (port >= PORT_A && port <= PORT_E) {
> >  			rate = acomp->aud_sample_rate[port];
> > -		else {
> > +		} else {
> >  			DRM_ERROR("invalid port: %d\n", port);
> >  			rate = 0;
> >  		}
> 
> Or you could improve scoping of the locals and eliminate a few lines
> whilst adding more information to the debug:
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index d32f586..98d4576 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,18 @@ 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);
> -               if (n != 0)
> +               DRM_DEBUG_KMS("port %d audio rate %d => N=%x\n", port, rate, n);
> +               if (n)
>                         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);
> 
> 

This looks a lot cleaner. 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for drm/i915: Fix braces in conditonal branches

[Patchwork]

== Series Details ==

Series: drm/i915: Fix braces in conditonal branches
URL   : https://patchwork.freedesktop.org/series/10875/
State : failure

== Summary ==

Series 10875v1 drm/i915: Fix braces in conditonal branches
http://patchwork.freedesktop.org/api/1.0/series/10875/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (ro-skl3-i5-6260u)
                pass       -> FAIL       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> FAIL       (ro-hsw-i7-4770r)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:244  pass:187  dwarn:28  dfail:0   fail:3   skip:26 
fi-skl-i5-6260u  total:244  pass:224  dwarn:4   dfail:0   fail:2   skip:14 
fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:219  dwarn:1   dfail:0   fail:1   skip:19 
ro-bdw-i7-5557U  total:240  pass:220  dwarn:1   dfail:0   fail:0   skip:19 
ro-bdw-i7-5600u  total:240  pass:206  dwarn:1   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:213  dwarn:0   dfail:0   fail:1   skip:26 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
ro-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1808/

fa68dcb drm-intel-nightly: 2016y-08m-10d-09h-40m-37s UTC integration manifest
207d24e drm/i915: Fix braces in conditonal branches

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

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;
+
+		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);
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/dp/i915: Clean up clock configuration for HDMI audio

[Dhinakaran Pandiyan]

No functional change, just clean up. Debug messages now print out clock
units. Additionally, the configuration bits, which are 1:1 mapped to the
clock frequency and don't convey much information are not printed out.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 3efce0e..9465f54 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -104,21 +104,17 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(hdmi_audio_clock); i++) {
-		if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock)
-			break;
-	}
-
-	if (i == ARRAY_SIZE(hdmi_audio_clock)) {
-		DRM_DEBUG_KMS("HDMI audio pixel clock setting for %d not found, falling back to defaults\n",
-			      adjusted_mode->crtc_clock);
-		i = 1;
+		if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) {
+			DRM_DEBUG_KMS("Configuring audio for HDMI pixel clk %dkHz\n",
+				      hdmi_audio_clock[i].clock);
+			return hdmi_audio_clock[i].config;
+		}
 	}
 
-	DRM_DEBUG_KMS("Configuring HDMI audio for pixel clock %d (0x%08x)\n",
-		      hdmi_audio_clock[i].clock,
-		      hdmi_audio_clock[i].config);
+	DRM_DEBUG_KMS("No config. for HDMI pixel clk %dkHz, using default %dkHz\n",
+		      adjusted_mode->crtc_clock, hdmi_audio_clock[1].clock);
 
-	return hdmi_audio_clock[i].config;
+	return hdmi_audio_clock[1].config;
 }
 
 static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Eliminate redundant local variable definition

[Dhinakaran Pandiyan]

No functional change, just clean up.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 9465f54..587d53a 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -351,9 +351,7 @@ static void ilk_audio_codec_disable(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
-	struct intel_digital_port *intel_dig_port =
-		enc_to_dig_port(&encoder->base);
-	enum port port = intel_dig_port->port;
+	enum port port = enc_to_dig_port(&encoder->base)->port;
 	enum pipe pipe = intel_crtc->pipe;
 	uint32_t tmp, eldv;
 	i915_reg_t aud_config, aud_cntrl_st2;
@@ -399,13 +397,10 @@ static void ilk_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);
-	struct intel_digital_port *intel_dig_port =
-		enc_to_dig_port(&encoder->base);
-	enum port port = intel_dig_port->port;
+	enum port port = enc_to_dig_port(&encoder->base)->port;
 	enum pipe pipe = intel_crtc->pipe;
 	uint8_t *eld = connector->eld;
-	uint32_t eldv;
-	uint32_t tmp;
+	uint32_t tmp, eldv;
 	int len, i;
 	i915_reg_t hdmiw_hdmiedid, aud_config, aud_cntl_st, aud_cntrl_st2;
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/dp/i915: Clean up clock configuration for HDMI audio

[Chris Wilson]

On Wed, Aug 10, 2016 at 11:41:12PM -0700, Dhinakaran Pandiyan wrote:
> No functional change, just clean up. Debug messages now print out clock
> units. Additionally, the configuration bits, which are 1:1 mapped to the
> clock frequency and don't convey much information are not printed out.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Eliminate redundant local variable definition

[Chris Wilson]

On Wed, Aug 10, 2016 at 11:41:13PM -0700, Dhinakaran Pandiyan wrote:
> No functional change, just clean up.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

A quick conversion to kernel types would also be appreciated.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/dp/i915: Clean up clock configuration for HDMI audio

[Jani Nikula]

On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> No functional change, just clean up. Debug messages now print out clock
> units. Additionally, the configuration bits, which are 1:1 mapped to the
> clock frequency and don't convey much information are not printed out.

IMO the cleanups here are subjective, i.e. another day someone might
come in and rewrite it back to having just one code path for return
instead of many. And someone might prefer early returns for errors.

If you really wanted to clean this up, you could abstract the config
lookup based on clock, and it would be a less subjective improvement.

>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3efce0e..9465f54 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -104,21 +104,17 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(hdmi_audio_clock); i++) {
> -		if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock)
> -			break;
> -	}
> -
> -	if (i == ARRAY_SIZE(hdmi_audio_clock)) {
> -		DRM_DEBUG_KMS("HDMI audio pixel clock setting for %d not found, falling back to defaults\n",
> -			      adjusted_mode->crtc_clock);
> -		i = 1;
> +		if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) {
> +			DRM_DEBUG_KMS("Configuring audio for HDMI pixel clk %dkHz\n",
> +				      hdmi_audio_clock[i].clock);
> +			return hdmi_audio_clock[i].config;
> +		}
>  	}
>  
> -	DRM_DEBUG_KMS("Configuring HDMI audio for pixel clock %d (0x%08x)\n",
> -		      hdmi_audio_clock[i].clock,
> -		      hdmi_audio_clock[i].config);
> +	DRM_DEBUG_KMS("No config. for HDMI pixel clk %dkHz, using default %dkHz\n",

Please at least fix the typos.

> +		      adjusted_mode->crtc_clock, hdmi_audio_clock[1].clock);
>  
> -	return hdmi_audio_clock[i].config;
> +	return hdmi_audio_clock[1].config;
>  }
>  
>  static int audio_config_get_n(const struct drm_display_mode *mode, int rate)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

Re: [PATCH 3/3] drm/i915: Eliminate redundant local variable definition

[Pandiyan, Dhinakaran]

On Thu, 2016-08-11 at 08:09 +0100, Chris Wilson wrote:
> On Wed, Aug 10, 2016 at 11:41:13PM -0700, Dhinakaran Pandiyan wrote:
> > No functional change, just clean up.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A quick conversion to kernel types would also be appreciated.
> -Chris
> 
Thanks for the review.

Just so that I don't misunderstand, do you mean this?
 /s/uint32_t/u32  
 /s/uint8_t/u8 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/dp/i915: Clean up clock configuration for HDMI audio

[Pandiyan, Dhinakaran]

On Thu, 2016-08-11 at 10:52 +0300, Jani Nikula wrote:
> On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> > No functional change, just clean up. Debug messages now print out clock
> > units. Additionally, the configuration bits, which are 1:1 mapped to the
> > clock frequency and don't convey much information are not printed out.
> 
> IMO the cleanups here are subjective, i.e. another day someone might
> come in and rewrite it back to having just one code path for return
> instead of many. And someone might prefer early returns for errors.

Fair enough. The change in return style is subjective, I will drop this
patch. Thanks for pointing it out.

> If you really wanted to clean this up, you could abstract the config
> lookup based on clock, and it would be a less subjective improvement.
> 
I guess, this would involve an additional data structure. I will leave
it as it is.

> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 3efce0e..9465f54 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -104,21 +104,17 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
> >  	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(hdmi_audio_clock); i++) {
> > -		if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock)
> > -			break;
> > -	}
> > -
> > -	if (i == ARRAY_SIZE(hdmi_audio_clock)) {
> > -		DRM_DEBUG_KMS("HDMI audio pixel clock setting for %d not found, falling back to defaults\n",
> > -			      adjusted_mode->crtc_clock);
> > -		i = 1;
> > +		if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) {
> > +			DRM_DEBUG_KMS("Configuring audio for HDMI pixel clk %dkHz\n",
> > +				      hdmi_audio_clock[i].clock);
> > +			return hdmi_audio_clock[i].config;
> > +		}
> >  	}
> >  
> > -	DRM_DEBUG_KMS("Configuring HDMI audio for pixel clock %d (0x%08x)\n",
> > -		      hdmi_audio_clock[i].clock,
> > -		      hdmi_audio_clock[i].config);
> > +	DRM_DEBUG_KMS("No config. for HDMI pixel clk %dkHz, using default %dkHz\n",
> 
> Please at least fix the typos.
> 
Probably the abbreviation was not obvious. I guess, the clock frequency
units are standard in i915 too. Thanks for you time.

> > +		      adjusted_mode->crtc_clock, hdmi_audio_clock[1].clock);
> >  
> > -	return hdmi_audio_clock[i].config;
> > +	return hdmi_audio_clock[1].config;
> >  }
> >  
> >  static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx