Re: [PATCH 2/4] drm/i915: Factor out a function returning the AUX_CTL value to start a send

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

On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> Also, move that computation outside of the for loop that tries 5 times,
> this value doesn't change between tries.

Some general bikeshedding...

I really wish everyone would write commit messages that work independent
of the patch subject. Just like a newspaper article should make sense
and tell a story also in the absence of a headline.

I know, Daniel usually churns out commit messages that are prime
examples of the *opposite* of this, so *sigh*. And,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

on the patch.

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08e8e34..a9a05d2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -413,6 +413,36 @@ static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	return index ? 0 : 100;
>  }
>  
> +static uint32_t i9xx_get_aux_send_ctl(struct intel_dp *intel_dp,
> +				      bool has_aux_irq,
> +				      int send_bytes,
> +				      uint32_t aux_clock_divider)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	uint32_t precharge, timeout;
> +
> +	if (IS_GEN6(dev))
> +		precharge = 3;
> +	else
> +		precharge = 5;
> +
> +	if (IS_BROADWELL(dev) && intel_dp->aux_ch_ctl_reg == DPA_AUX_CH_CTL)
> +		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
> +	else
> +		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
> +
> +	return DP_AUX_CH_CTL_SEND_BUSY |
> +	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       timeout |
> +	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> +	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> +	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
> +	       DP_AUX_CH_CTL_DONE |
> +	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
> +	       DP_AUX_CH_CTL_RECEIVE_ERROR;
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -426,9 +456,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	uint32_t aux_clock_divider;
>  	int i, ret, recv_bytes;
>  	uint32_t status;
> -	int try, precharge, clock = 0;
> +	int try, clock = 0;
>  	bool has_aux_irq = true;
> -	uint32_t timeout;
>  
>  	/* dp aux is extremely sensitive to irq latency, hence request the
>  	 * lowest possible wakeup latency and so prevent the cpu from going into
> @@ -438,16 +467,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> -	if (IS_GEN6(dev))
> -		precharge = 3;
> -	else
> -		precharge = 5;
> -
> -	if (IS_BROADWELL(dev) && ch_ctl == DPA_AUX_CH_CTL)
> -		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
> -	else
> -		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
> -
>  	intel_aux_display_runtime_get(dev_priv);
>  
>  	/* Try to wait for any previous AUX channel activity */
> @@ -472,6 +491,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	}
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> +		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
> +						     has_aux_irq,
> +						     send_bytes,
> +						     aux_clock_divider);
> +
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
> @@ -480,16 +504,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  					   pack_aux(send + i, send_bytes - i));
>  
>  			/* Send the command and wait for it to complete */
> -			I915_WRITE(ch_ctl,
> -				   DP_AUX_CH_CTL_SEND_BUSY |
> -				   (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> -				   timeout |
> -				   (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> -				   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> -				   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
> -				   DP_AUX_CH_CTL_DONE |
> -				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -				   DP_AUX_CH_CTL_RECEIVE_ERROR);
> +			I915_WRITE(ch_ctl, send_ctl);
>  
>  			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
>  
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center