Re: [PATCH 04/14] drm/i915: Clean up TV pipe select bits

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 04/14] drm/i915: Clean up TV pipe select bits
Date: Tue, 20 Mar 2018 08:55:17 +0200	[thread overview]
Message-ID: <87a7v3z162.fsf@intel.com> (raw)
In-Reply-To: <20180302095512.19329-5-ville.syrjala@linux.intel.com>

On Fri, 02 Mar 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Parametrize the TV pipe select bits.
>
> For consistency with the new way of doing things, let's read out the
> pipe select bits even when the port is disable, even though we don't
> need that behaviour for asserts in this case.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  4 +++-
>  drivers/gpu/drm/i915/intel_tv.c | 18 +++++-------------
>  2 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0917fcbd618d..fadd0a285efa 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4762,7 +4762,9 @@ enum {
>  /* Enables the TV encoder */
>  # define TV_ENC_ENABLE			(1 << 31)
>  /* Sources the TV encoder input from pipe B instead of A. */
> -# define TV_ENC_PIPEB_SELECT		(1 << 30)
> +# define TV_ENC_PIPE_SEL(pipe)		((pipe) << 30)
> +# define TV_ENC_PIPE_SEL_MASK		(1 << 30)
> +# define TV_ENC_PIPE_SEL_SHIFT		30

Same old nitpick about masks and shifts before values.

>  /* Outputs composite video (DAC A only) */
>  # define TV_ENC_OUTPUT_COMPOSITE	(0 << 28)
>  /* Outputs SVideo video (DAC B/C) */
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 885fc3809f7f..24caf340a7a5 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -798,16 +798,12 @@ static struct intel_tv *intel_attached_tv(struct drm_connector *connector)
>  static bool
>  intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
>  {
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	u32 tmp = I915_READ(TV_CTL);
>  
> -	if (!(tmp & TV_ENC_ENABLE))
> -		return false;
> +	*pipe = (tmp & TV_ENC_PIPE_SEL_MASK) >> TV_ENC_PIPE_SEL_SHIFT;
>  
> -	*pipe = PORT_TO_PIPE(tmp);
> -
> -	return true;
> +	return tmp & TV_ENC_ENABLE;
>  }
>  
>  static void
> @@ -1024,8 +1020,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  		break;
>  	}
>  
> -	if (intel_crtc->pipe == 1)
> -		tv_ctl |= TV_ENC_PIPEB_SELECT;
> +	tv_ctl |= TV_ENC_PIPE_SEL(intel_crtc->pipe);
>  	tv_ctl |= tv_mode->oversample;
>  
>  	if (tv_mode->progressive)
> @@ -1151,10 +1146,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  	/* Poll for TV detection */
>  	tv_ctl &= ~(TV_ENC_ENABLE | TV_TEST_MODE_MASK);

You need to add TV_ENC_PIPE_SEL_MASK here.

With that fixed,

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

>  	tv_ctl |= TV_TEST_MODE_MONITOR_DETECT;
> -	if (intel_crtc->pipe == 1)
> -		tv_ctl |= TV_ENC_PIPEB_SELECT;
> -	else
> -		tv_ctl &= ~TV_ENC_PIPEB_SELECT;
> +	tv_ctl |= TV_ENC_PIPE_SEL(intel_crtc->pipe);
>  
>  	tv_dac &= ~(TVDAC_SENSE_MASK | DAC_A_MASK | DAC_B_MASK | DAC_C_MASK);
>  	tv_dac |= (TVDAC_STATE_CHG_EN |

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