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

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

[Joe Konno]

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.

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

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

[Ausmus, James]

[-- Attachment #1.1: Type: text/plain, Size: 2766 bytes --]

On Thu, Feb 26, 2015 at 2:48 PM, Joe Konno <joe.konno@linux.intel.com>
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.
>
> 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
>

Tested-by: James Ausmus <james.ausmus@intel.com>


> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx




--


James Ausmus
Sr. Software Engineer
SSG-OTC ChromeOS Integration

[-- Attachment #1.2: Type: text/html, Size: 4125 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

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

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

[Daniel Vetter]

On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote:
> 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.

Alternative issue is that wm recompute happens before we update all the
legacy state. Tvrkto just stumbled over that for some of his skl watermark
code, he had to switch a few places in the wm code from looking at
plane->fb to look at plane->state->fb.

Similar changes might be needed for the cursor wm code -
cursor_width/height is kinda redundant since universal planes support.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joe Konno]

On 02/27/2015 06:53 AM, Daniel Vetter wrote:
> On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote:
>> 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.
> 
> Alternative issue is that wm recompute happens before we update all the
> legacy state. Tvrkto just stumbled over that for some of his skl watermark
> code, he had to switch a few places in the wm code from looking at
> plane->fb to look at plane->state->fb.
> 
> Similar changes might be needed for the cursor wm code -
> cursor_width/height is kinda redundant since universal planes support.

Thanks for the insights, Matt and Dan. It's clear to me from Matt's analysis
that my patch is a hack at best and nowhere near a proper fix.

This patch was submitted in the hopes of fixing a substantial graphical
regression. Shall my patch be retooled as a "better" work-around, or will the
regression stand until the watermark code is beat into proper shape?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Fri, Feb 27, 2015 at 08:21:34AM -0800, Joe Konno wrote:
> On 02/27/2015 06:53 AM, Daniel Vetter wrote:
> > On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote:
> >> 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.
> > 
> > Alternative issue is that wm recompute happens before we update all the
> > legacy state. Tvrkto just stumbled over that for some of his skl watermark
> > code, he had to switch a few places in the wm code from looking at
> > plane->fb to look at plane->state->fb.
> > 
> > Similar changes might be needed for the cursor wm code -
> > cursor_width/height is kinda redundant since universal planes support.
> 
> Thanks for the insights, Matt and Dan. It's clear to me from Matt's analysis
> that my patch is a hack at best and nowhere near a proper fix.
> 
> This patch was submitted in the hopes of fixing a substantial graphical
> regression. Shall my patch be retooled as a "better" work-around, or will the
> regression stand until the watermark code is beat into proper shape?

I have a couple patches already written that I think might solve your
problem.  I'll extract those from my in-progress tree and post them
shortly for you to try.


Matt

-- 
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