Re: [PATCH 2/2] drm/i915: Fix SKL cursor watermarks

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

On Wed, Mar 22, 2017 at 11:51:56AM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-03-22 at 10:01 +0100, Maarten Lankhorst wrote:
> > Op 14-03-17 om 16:10 schreef ville.syrjala@linux.intel.com:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Use intel_wm_plane_visible() to determine cursor visibility for SKL+
> > > also. Previously SKL+ would check the actual visibility which now
> > > conflicts with the assumptions in intel_legacy_cursor_update().
> > > 
> > > We also change SKL+ to compute the cursor watermarks based on the
> > > unclipped cursor size, just as we do on all the other platforms.
> > > Using the clipped size could now result in garbage results.
> > > 
> > > Testcase: igt/kms_chv_cursor_fail
> > > Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW")
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100195
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > For patch 1 & 2:
> > 
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > Should be the right way to fix it. :)
> 
> In intel_legacy_cursor_update(), I see the comment below,
> 
>         /*
>          * If any parameters change that may affect watermarks,
>          * take the slowpath. Only changing fb or position should be
>          * in the fastpath.
>          */
> 
> followed by a bunch of checks on the plane size and fb. My understanding is that
> the bug was caused by those assumptions being out of sync with the actual
> watermark code. So IMO, a more proper way to fix this would be to have
> intel_legacy_cursor_update() call into watermark code to ask if it can proceed
> or not, instead of making assumptions of what can cause watermarks to change.

Yeah, we do (at at least used to) have some assumptions about watermarks
in other parts of the driver as well. That's potentially something we
should fix, probably when someone finally starts doing the
s/active/enable/ change for watermarks.

> 
> But since the duplicated assumptions were there before, this fix doesn't make
> the overall situation any worse.
> 
> Acked-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thank you all. Series pushed to dinq.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx