Re: [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set

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

On Wed, Sep 04, 2013 at 02:20:08PM +0300, Jani Nikula wrote:
> The cursor is supposed to be disabled during crtc mode set (disabled by
> ctrc disable). Assert this is the case.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d88057e..89243cc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
>  	     pipe_name(pipe));
>  }
>  
> +static void assert_cursor(struct drm_i915_private *dev_priv,
> +			  enum pipe pipe, bool state)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	bool cur_state;
> +
> +	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))

This could be (gen >=7 && !VLV), but we have the same check in 
intel_crtc_update_cursor() so that should be changed as well. Can
be left for another patch I suppose.

> +		cur_state = I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE;
> +	else if (IS_845G(dev) || IS_I865G(dev))
> +		cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
> +	else
> +		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;

First I was thingking that can't be right, but looks like it is. Weird
how they stuffed the cursor registers differently on mobile and desktop
during the gen2 days.

> +
> +	WARN(cur_state != state,
> +	     "cursor on pipe %c assertion failure (expected %s, current %s)\n",
> +	     pipe_name(pipe), state_string(state), state_string(cur_state));
> +}
> +#define assert_cursor_enabled(d, p) assert_cursor(d, p, true)
> +#define assert_cursor_disabled(d, p) assert_cursor(d, p, false)

I guess we don't really need assert_cursor_enabled() but no real harm in
having it.

> +
>  void assert_pipe(struct drm_i915_private *dev_priv,
>  		 enum pipe pipe, bool state)
>  {
> @@ -4856,6 +4876,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	const intel_limit_t *limit;
>  	int ret;
>  
> +	assert_cursor_disabled(dev_priv, pipe);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
>  		case INTEL_OUTPUT_LVDS:
> @@ -5754,6 +5776,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	struct intel_shared_dpll *pll;
>  	int ret;
>  
> +	assert_cursor_disabled(dev_priv, pipe);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
>  		case INTEL_OUTPUT_LVDS:
> @@ -6270,6 +6294,8 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	int plane = intel_crtc->plane;
>  	int ret;
>  
> +	assert_cursor_disabled(dev_priv, intel_crtc->pipe);
> +
>  	if (!intel_ddi_pll_mode_set(crtc))
>  		return -EINVAL;

I'd slap the asserts to the same places as the asserts for other planes.
But maybe we'd want to have pipe and plane asserts in mode_set as well,
just in case we manage to mess things up and call mode_set w/o disabling
the pipe first.

Also looks like the plane assert should be killed from
ironlake_fdi_link_train(). We shouldn't need a plain to train the FDI
link as the pipe will anyway pump out pixels.

-- 
Ville Syrjälä
Intel OTC