Re: [PATCH 2/2] drm/i915: improve assert_panel_unlocked

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 21, 2014 at 03:06:26PM +0300, Jani Nikula wrote:
> Fix assert_panel_unlocked for vlv/chv, and improve it a bit for
> non-LVDS. Also don't pretend it works for DDI. There's still work to do
> to get this right for eDP on PCH platforms, but this is a start.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> So I wanted to quickly fix assert_panel_unlocked, but for such a short
> piece of code it's too involved to _quickly_ get right across all
> platforms. I think this is a worthwhile improvement though.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe1d00dc9ef5..d6b48496d7f4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1193,17 +1193,33 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
>  static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
>  				  enum pipe pipe)
>  {
> -	int pp_reg, lvds_reg;
> +	struct drm_device *dev = dev_priv->dev;
> +	int pp_reg;
>  	u32 val;
>  	enum pipe panel_pipe = PIPE_A;
>  	bool locked = true;
>  
> -	if (HAS_PCH_SPLIT(dev_priv->dev)) {
> +	if (HAS_DDI(dev)) {
> +		/* XXX: this neither works nor gets called for DDI */

Not sure why the XXX here. Seems to me there's nothing to fix here for
DDI. Maybe just make that a WARN_ON(HAS_DDI()) or just remove the check
entirely.

> +		return;
> +	} else if (HAS_PCH_SPLIT(dev)) {
> +		u32 port_sel;
> +
>  		pp_reg = PCH_PP_CONTROL;
> -		lvds_reg = PCH_LVDS;
> +		port_sel = I915_READ(PCH_PP_ON_DELAYS) & PANEL_PORT_SELECT_MASK;
> +
> +		if (port_sel == PANEL_PORT_SELECT_LVDS &&
> +		    I915_READ(PCH_LVDS) & LVDS_PIPEB_SELECT)
> +			panel_pipe = PIPE_B;
> +		/* XXX: else fix for eDP */
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		/* presumably write lock depends on pipe, not port select */

Hmm. This is a good question. Needs a bit if testing I suppose. In the
worst case it somehow gets tied in with how the power sequencer gets locked
to the port. For that we'd probably just have to check both power sequencers
here and complain if either has the registers locked. Or maybe we should
just do that anyway because it's such a simple solution? But we could
do that simply by calling assert_panel_unlocked() twice (once for each pipe)
from VLV specific code, so this patch seems to be exactly what we need as
a first step.

Apart from the XXX in the comment:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		pp_reg = VLV_PIPE_PP_CONTROL(pipe);
> +		panel_pipe = pipe;
>  	} else {
>  		pp_reg = PP_CONTROL;
> -		lvds_reg = LVDS;
> +		if (I915_READ(LVDS) & LVDS_PIPEB_SELECT)
> +			panel_pipe = PIPE_B;
>  	}
>  
>  	val = I915_READ(pp_reg);
> @@ -1211,9 +1227,6 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
>  	    ((val & PANEL_UNLOCK_MASK) == PANEL_UNLOCK_REGS))
>  		locked = false;
>  
> -	if (I915_READ(lvds_reg) & LVDS_PIPEB_SELECT)
> -		panel_pipe = PIPE_B;
> -
>  	WARN(panel_pipe == pipe && locked,
>  	     "panel assertion failure, pipe %c regs locked\n",
>  	     pipe_name(pipe));
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC