Re: [PATCH] drm/i915: Redo WMs when cursor size changes

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Feb 26, 2015 at 02:48:44PM -0800, Joe Konno wrote:
> From: Joe Konno <joe.konno@intel.com>
> 
> In instances where cursor sizes change, as in Chromium Ozone/Freon,
> watermarks should be recomputed. There should be no hard-coded
> assumptions about cursor widths. This was corrected originally here:
> 
>     commit 64f962e3e38bf6f40bbd2462f8380dee0369e1bf
>     Author: Chris Wilson <chris@chris-wilson.co.uk>
>     Date:   Wed Mar 26 12:38:15 2014 +0000
> 
>         drm/i915: Recompute WM when the cursor size changes
> 
> However, it seems the recompute logic got lost in refactoring.
> Re-introduce the relevant WM re-compute code from the original patch.

I don't believe the recompute logic got lost, it just got moved around a
bit so that it wouldn't happen during our time-sensitive vblank evasion
while interrupts are disabled.  The old_width != intel_crtc->cursor_width
check that you're re-adding below already happens in
intel_check_cursor_plane():

   finish: 
           if (intel_crtc->active) {
                   if (intel_crtc->cursor_width != state->base.crtc_w)
                           intel_crtc->atomic.update_wm = true;

We just set a flag so that we know whether we'll need to update
watermarks later or not.  Then when we get to the commit phase, in
intel_begin_crtc_commit(), we do the following before entering vblank
evasion:

        if (intel_crtc->atomic.update_wm)
                intel_update_watermarks(crtc);

Some of the watermark code we have today sleeps, so we can't call it
under vblank evasion (i.e., during the commit phase while interrupts are
disabled).

Our high-level atomic workflow (which legacy plane ioctls also flow
through internally) looks something like this:

 - Some userspace ioctl (SetCursor, SetPlane, etc.)
      - intel_check_TYPE_plane()
            - check validity of request, bail if invalid
            - set flags in intel_crtc->atomic for various stuff we will
              need to do before/after vblank evasion (e.g., wm's)
      - intel_begin_crtc_commit()
            - Perform sleepable operations noted in intel_crtc->atomic
            - intel_pipe_update_start (interrupts are now disabled!)
      - intel_commit_TYPE_plane()
      - intel_finish_crtc_commit()
            - intel_pipe_update_end (interrupts are now reenabled)
            - Perform sleepable operations noted in intel_crtc->atomic

So your patch below could result in sleeps happening while vblanks are
disabled, which is bad (IIRC, most of those sleeps are in the SKL
codepath right now, but I think there's a workaround-related wait for
IVB as well).

Our watermark code needs a lot of work to beat it into proper shape for
atomic and that's what I'm working on at the moment.


Matt

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Fixes: 32b7eeec4d1e ("drm/i915: Refactor work that can sleep out of commit (v7)")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89346
> Signed-off-by: Joe Konno <joe.konno@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b0f113d9daab..c5cbfea0551e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12234,6 +12234,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>  	uint32_t addr;
> +	unsigned old_width;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
> @@ -12243,6 +12244,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	crtc->cursor_y = state->base.crtc_y;
>  
>  	intel_plane->obj = obj;
> +	old_width = intel_crtc->cursor_width;
>  
>  	if (intel_crtc->cursor_bo == obj)
>  		goto update;
> @@ -12260,8 +12262,11 @@ update:
>  	intel_crtc->cursor_width = state->base.crtc_w;
>  	intel_crtc->cursor_height = state->base.crtc_h;
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->active) {
> +		if (old_width != intel_crtc->cursor_width)
> +			intel_update_watermarks(crtc);
>  		intel_crtc_update_cursor(crtc, state->visible);
> +	}
>  }
>  
>  static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx