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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
