diff mbox

drm/i915: remove check for aux irq

Message ID 20180425215524.27613-1-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lucas De Marchi April 25, 2018, 9:55 p.m. UTC
This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
GMBUS and AUX interrupts on gen4/g4x").

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 +--
 drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |  1 -
 drivers/gpu/drm/i915/intel_psr.c |  2 +-
 4 files changed, 9 insertions(+), 19 deletions(-)

Comments

Rodrigo Vivi April 25, 2018, 11:24 p.m. UTC | #1
On Wed, Apr 25, 2018 at 02:55:24PM -0700, Lucas De Marchi wrote:
> This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> GMBUS and AUX interrupts on gen4/g4x").
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
>  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>  drivers/gpu/drm/i915/intel_psr.c |  2 +-
>  4 files changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8444ca8d5aa3..09e1c2289ea1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2545,7 +2545,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
>  
>  /*
> - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> + * gmbus irq on gen4 seems to be able to generate legacy interrupts
>   * even when in MSI mode. This results in spurious interrupt warnings if the
>   * legacy irq no. is shared with another device. The kernel then disables that
>   * interrupt source and so prevents the other device from working properly.
> @@ -2553,7 +2553,6 @@ intel_info(const struct drm_i915_private *dev_priv)
>   * Since we don't enable MSI anymore on gen4, we can always use GMBUS/AUX
>   * interrupts.
>   */
> -#define HAS_AUX_IRQ(dev_priv)   true
>  #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
>  
>  /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..fd417473b6a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -936,7 +936,7 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
>  }
>  
>  static uint32_t
> -intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> +intel_dp_aux_wait_done(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
>  	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> @@ -944,14 +944,10 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  	bool done;
>  
>  #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> -	if (has_aux_irq)
> -		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> -					  msecs_to_jiffies_timeout(10));
> -	else
> -		done = wait_for(C, 10) == 0;
> +	done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> +				  msecs_to_jiffies_timeout(10));
>  	if (!done)
> -		DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
> -			  has_aux_irq);
> +		DRM_ERROR("dp aux hw did not signal timeout!\n");
>  #undef C
>  
>  	return status;
> @@ -1016,7 +1012,6 @@ static uint32_t skl_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  }
>  
>  static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> -				     bool has_aux_irq,
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider)
>  {
> @@ -1037,7 +1032,7 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  
>  	return DP_AUX_CH_CTL_SEND_BUSY |
>  	       DP_AUX_CH_CTL_DONE |
> -	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       DP_AUX_CH_CTL_INTERRUPT |
>  	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  	       timeout |
>  	       DP_AUX_CH_CTL_RECEIVE_ERROR |
> @@ -1047,13 +1042,12 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  }
>  
>  static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> -				      bool has_aux_irq,
>  				      int send_bytes,
>  				      uint32_t unused)
>  {
>  	return DP_AUX_CH_CTL_SEND_BUSY |
>  	       DP_AUX_CH_CTL_DONE |
> -	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       DP_AUX_CH_CTL_INTERRUPT |
>  	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  	       DP_AUX_CH_CTL_TIME_OUT_MAX |
>  	       DP_AUX_CH_CTL_RECEIVE_ERROR |
> @@ -1076,7 +1070,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  	int i, ret, recv_bytes;
>  	uint32_t status;
>  	int try, clock = 0;
> -	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
>  	bool vdd;
>  
>  	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> @@ -1131,7 +1124,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>  		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> -							  has_aux_irq,
>  							  send_bytes,
>  							  aux_clock_divider);
>  
> @@ -1148,7 +1140,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  			/* Send the command and wait for it to complete */
>  			I915_WRITE(ch_ctl, send_ctl);
>  
> -			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
> +			status = intel_dp_aux_wait_done(intel_dp);
>  
>  			/* Clear done status and any errors */
>  			I915_WRITE(ch_ctl,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9bba0354ccd3..7cc5deed9511 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1135,7 +1135,6 @@ struct intel_dp {
>  	 * register with to kick off an AUX transaction.
>  	 */
>  	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
> -				     bool has_aux_irq,
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider);
>  
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 0d548292dd09..fd348f1b53ef 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -328,7 +328,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>  
>  	/* Start with bits set for DDI_AUX_CTL register */
> -	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, 0, sizeof(aux_msg),
> +	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>  					     aux_clock_divider);
>  
>  	/* Select only valid bits for SRD_AUX_CTL */
> -- 
> 2.14.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala April 26, 2018, 1:43 p.m. UTC | #2
On Wed, Apr 25, 2018 at 02:55:24PM -0700, Lucas De Marchi wrote:
> This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> GMBUS and AUX interrupts on gen4/g4x").
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
>  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>  drivers/gpu/drm/i915/intel_psr.c |  2 +-
>  4 files changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8444ca8d5aa3..09e1c2289ea1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2545,7 +2545,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
>  
>  /*
> - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> + * gmbus irq on gen4 seems to be able to generate legacy interrupts

Why are you removing vital information from the comment?

>   * even when in MSI mode. This results in spurious interrupt warnings if the
>   * legacy irq no. is shared with another device. The kernel then disables that
>   * interrupt source and so prevents the other device from working properly.
> @@ -2553,7 +2553,6 @@ intel_info(const struct drm_i915_private *dev_priv)
>   * Since we don't enable MSI anymore on gen4, we can always use GMBUS/AUX
>   * interrupts.
>   */
> -#define HAS_AUX_IRQ(dev_priv)   true
>  #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
>  
>  /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..fd417473b6a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -936,7 +936,7 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
>  }
>  
>  static uint32_t
> -intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> +intel_dp_aux_wait_done(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
>  	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> @@ -944,14 +944,10 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  	bool done;
>  
>  #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> -	if (has_aux_irq)
> -		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> -					  msecs_to_jiffies_timeout(10));
> -	else
> -		done = wait_for(C, 10) == 0;
> +	done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> +				  msecs_to_jiffies_timeout(10));
>  	if (!done)
> -		DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
> -			  has_aux_irq);
> +		DRM_ERROR("dp aux hw did not signal timeout!\n");
>  #undef C
>  
>  	return status;
> @@ -1016,7 +1012,6 @@ static uint32_t skl_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  }
>  
>  static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> -				     bool has_aux_irq,
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider)
>  {
> @@ -1037,7 +1032,7 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  
>  	return DP_AUX_CH_CTL_SEND_BUSY |
>  	       DP_AUX_CH_CTL_DONE |
> -	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       DP_AUX_CH_CTL_INTERRUPT |
>  	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  	       timeout |
>  	       DP_AUX_CH_CTL_RECEIVE_ERROR |
> @@ -1047,13 +1042,12 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  }
>  
>  static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> -				      bool has_aux_irq,
>  				      int send_bytes,
>  				      uint32_t unused)
>  {
>  	return DP_AUX_CH_CTL_SEND_BUSY |
>  	       DP_AUX_CH_CTL_DONE |
> -	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       DP_AUX_CH_CTL_INTERRUPT |
>  	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  	       DP_AUX_CH_CTL_TIME_OUT_MAX |
>  	       DP_AUX_CH_CTL_RECEIVE_ERROR |
> @@ -1076,7 +1070,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  	int i, ret, recv_bytes;
>  	uint32_t status;
>  	int try, clock = 0;
> -	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
>  	bool vdd;
>  
>  	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> @@ -1131,7 +1124,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>  		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> -							  has_aux_irq,
>  							  send_bytes,
>  							  aux_clock_divider);
>  
> @@ -1148,7 +1140,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  			/* Send the command and wait for it to complete */
>  			I915_WRITE(ch_ctl, send_ctl);
>  
> -			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
> +			status = intel_dp_aux_wait_done(intel_dp);
>  
>  			/* Clear done status and any errors */
>  			I915_WRITE(ch_ctl,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9bba0354ccd3..7cc5deed9511 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1135,7 +1135,6 @@ struct intel_dp {
>  	 * register with to kick off an AUX transaction.
>  	 */
>  	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
> -				     bool has_aux_irq,
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider);
>  
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 0d548292dd09..fd348f1b53ef 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -328,7 +328,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>  
>  	/* Start with bits set for DDI_AUX_CTL register */
> -	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, 0, sizeof(aux_msg),
> +	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>  					     aux_clock_divider);
>  
>  	/* Select only valid bits for SRD_AUX_CTL */
> -- 
> 2.14.3
Lucas De Marchi April 26, 2018, 3:22 p.m. UTC | #3
On Thu, Apr 26, 2018 at 04:43:38PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 25, 2018 at 02:55:24PM -0700, Lucas De Marchi wrote:
> > This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> > GMBUS and AUX interrupts on gen4/g4x").
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
> >  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
> >  drivers/gpu/drm/i915/intel_drv.h |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c |  2 +-
> >  4 files changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8444ca8d5aa3..09e1c2289ea1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2545,7 +2545,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> >  	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> >  
> >  /*
> > - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> > + * gmbus irq on gen4 seems to be able to generate legacy interrupts
> 
> Why are you removing vital information from the comment?

Because it wouldn't match the code anymore. We always use aux irq.

Lucas De Marchi
Ville Syrjala April 26, 2018, 3:27 p.m. UTC | #4
On Thu, Apr 26, 2018 at 08:22:12AM -0700, Lucas De Marchi wrote:
> On Thu, Apr 26, 2018 at 04:43:38PM +0300, Ville Syrjälä wrote:
> > On Wed, Apr 25, 2018 at 02:55:24PM -0700, Lucas De Marchi wrote:
> > > This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> > > GMBUS and AUX interrupts on gen4/g4x").
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
> > >  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 -
> > >  drivers/gpu/drm/i915/intel_psr.c |  2 +-
> > >  4 files changed, 9 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8444ca8d5aa3..09e1c2289ea1 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2545,7 +2545,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> > >  	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> > >  
> > >  /*
> > > - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> > > + * gmbus irq on gen4 seems to be able to generate legacy interrupts
> > 
> > Why are you removing vital information from the comment?
> 
> Because it wouldn't match the code anymore. We always use aux irq.

The comment is documenting the hardware behaviour. We don't want to lose
that information.
Lucas De Marchi April 26, 2018, 3:42 p.m. UTC | #5
On Thu, Apr 26, 2018 at 06:27:26PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 26, 2018 at 08:22:12AM -0700, Lucas De Marchi wrote:
> > On Thu, Apr 26, 2018 at 04:43:38PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 25, 2018 at 02:55:24PM -0700, Lucas De Marchi wrote:
> > > > This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> > > > GMBUS and AUX interrupts on gen4/g4x").
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 -
> > > >  drivers/gpu/drm/i915/intel_psr.c |  2 +-
> > > >  4 files changed, 9 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 8444ca8d5aa3..09e1c2289ea1 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2545,7 +2545,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> > > >  	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> > > >  
> > > >  /*
> > > > - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> > > > + * gmbus irq on gen4 seems to be able to generate legacy interrupts
> > > 
> > > Why are you removing vital information from the comment?
> > 
> > Because it wouldn't match the code anymore. We always use aux irq.
> 
> The comment is documenting the hardware behaviour. We don't want to lose
> that information.

IMO it's confusing to have the comment saying one thing and then code
not following it. Reading it again I see the second paragraph you added
actually document the code and the first the HW behavior. Maybe starting
the second paragraph with a "However" would make it clearer.  Or I can just
drop this change in the comment.

thanks
Lucas De Marchi
Ville Syrjala April 26, 2018, 3:50 p.m. UTC | #6
On Thu, Apr 26, 2018 at 08:42:54AM -0700, Lucas De Marchi wrote:
> On Thu, Apr 26, 2018 at 06:27:26PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 26, 2018 at 08:22:12AM -0700, Lucas De Marchi wrote:
> > > On Thu, Apr 26, 2018 at 04:43:38PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Apr 25, 2018 at 02:55:24PM -0700, Lucas De Marchi wrote:
> > > > > This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> > > > > GMBUS and AUX interrupts on gen4/g4x").
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
> > > > >  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++---------------
> > > > >  drivers/gpu/drm/i915/intel_drv.h |  1 -
> > > > >  drivers/gpu/drm/i915/intel_psr.c |  2 +-
> > > > >  4 files changed, 9 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 8444ca8d5aa3..09e1c2289ea1 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -2545,7 +2545,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> > > > >  	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> > > > >  
> > > > >  /*
> > > > > - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> > > > > + * gmbus irq on gen4 seems to be able to generate legacy interrupts
> > > > 
> > > > Why are you removing vital information from the comment?
> > > 
> > > Because it wouldn't match the code anymore. We always use aux irq.
> > 
> > The comment is documenting the hardware behaviour. We don't want to lose
> > that information.
> 
> IMO it's confusing to have the comment saying one thing and then code
> not following it. Reading it again I see the second paragraph you added
> actually document the code and the first the HW behavior. Maybe starting
> the second paragraph with a "However" would make it clearer.  Or I can just
> drop this change in the comment.

Or you can move the relevant parts of the comment to the place where
we do the "MSI or not to MSI" decision.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8444ca8d5aa3..09e1c2289ea1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2545,7 +2545,7 @@  intel_info(const struct drm_i915_private *dev_priv)
 	 IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
 
 /*
- * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
+ * gmbus irq on gen4 seems to be able to generate legacy interrupts
  * even when in MSI mode. This results in spurious interrupt warnings if the
  * legacy irq no. is shared with another device. The kernel then disables that
  * interrupt source and so prevents the other device from working properly.
@@ -2553,7 +2553,6 @@  intel_info(const struct drm_i915_private *dev_priv)
  * Since we don't enable MSI anymore on gen4, we can always use GMBUS/AUX
  * interrupts.
  */
-#define HAS_AUX_IRQ(dev_priv)   true
 #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
 
 /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..fd417473b6a9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -936,7 +936,7 @@  intel_dp_check_edp(struct intel_dp *intel_dp)
 }
 
 static uint32_t
-intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
+intel_dp_aux_wait_done(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
 	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
@@ -944,14 +944,10 @@  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 	bool done;
 
 #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
-	if (has_aux_irq)
-		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
-					  msecs_to_jiffies_timeout(10));
-	else
-		done = wait_for(C, 10) == 0;
+	done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
+				  msecs_to_jiffies_timeout(10));
 	if (!done)
-		DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
-			  has_aux_irq);
+		DRM_ERROR("dp aux hw did not signal timeout!\n");
 #undef C
 
 	return status;
@@ -1016,7 +1012,6 @@  static uint32_t skl_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 }
 
 static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
-				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider)
 {
@@ -1037,7 +1032,7 @@  static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
 
 	return DP_AUX_CH_CTL_SEND_BUSY |
 	       DP_AUX_CH_CTL_DONE |
-	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
+	       DP_AUX_CH_CTL_INTERRUPT |
 	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
 	       timeout |
 	       DP_AUX_CH_CTL_RECEIVE_ERROR |
@@ -1047,13 +1042,12 @@  static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
 }
 
 static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
-				      bool has_aux_irq,
 				      int send_bytes,
 				      uint32_t unused)
 {
 	return DP_AUX_CH_CTL_SEND_BUSY |
 	       DP_AUX_CH_CTL_DONE |
-	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
+	       DP_AUX_CH_CTL_INTERRUPT |
 	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
 	       DP_AUX_CH_CTL_TIME_OUT_MAX |
 	       DP_AUX_CH_CTL_RECEIVE_ERROR |
@@ -1076,7 +1070,6 @@  intel_dp_aux_xfer(struct intel_dp *intel_dp,
 	int i, ret, recv_bytes;
 	uint32_t status;
 	int try, clock = 0;
-	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
 	bool vdd;
 
 	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
@@ -1131,7 +1124,6 @@  intel_dp_aux_xfer(struct intel_dp *intel_dp,
 
 	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
 		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
-							  has_aux_irq,
 							  send_bytes,
 							  aux_clock_divider);
 
@@ -1148,7 +1140,7 @@  intel_dp_aux_xfer(struct intel_dp *intel_dp,
 			/* Send the command and wait for it to complete */
 			I915_WRITE(ch_ctl, send_ctl);
 
-			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
+			status = intel_dp_aux_wait_done(intel_dp);
 
 			/* Clear done status and any errors */
 			I915_WRITE(ch_ctl,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9bba0354ccd3..7cc5deed9511 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1135,7 +1135,6 @@  struct intel_dp {
 	 * register with to kick off an AUX transaction.
 	 */
 	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
-				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
 
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 0d548292dd09..fd348f1b53ef 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -328,7 +328,7 @@  static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
 	/* Start with bits set for DDI_AUX_CTL register */
-	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, 0, sizeof(aux_msg),
+	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
 					     aux_clock_divider);
 
 	/* Select only valid bits for SRD_AUX_CTL */