[Intel-gfx] [PATCH] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Paulo Zanoni]

2015-06-18 6:23 GMT-03:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> The current/old frontbuffer might still have gpu frontbuffer rendering
> pending. But once flipped it won't have the corresponding frontbuffer
> bits any more and hence the request retire function won't ever clear
> the corresponding busy bits. The async flip tracking (with the
> flip_prepare and flip_complete functions) already does this, but
> somehow I've forgotten to do this for synchronous flips.
>
> Note that we don't track outstanding rendering of the new framebuffer
> with busy_bits since all our plane update code waits for previous
> rendering to complete before displaying a new buffer. Hence a new
> buffer will never be busy.

Steps to reproduce:
- Boot your machine to the display manager
- Kill it, returning to fbcon
- Try to run any subtest from igt/tests/kms_frontbuffer_tracking: they
all fail because FBC never gets enabled
- Do any blt/render operation to the frontbuffer
- Now the tests will succeed

I also submitted a new test for this, that doesn't require the steps
above, so we can also add:
Testcase: igt/kms_frontbuffer_tracking/fbc-modesetfrombusy

More below:

>
> v2: Drop the spurious inline Ville spotted.
>
> Reported-by: Paulo Zanoni <przanoni at gmail.com>
> Cc: Paulo Zanoni <przanoni at gmail.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
> busy buffer tests.
> ---
>  drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcafefcf048b..b7c69460fb20 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>                                      unsigned frontbuffer_bits);
>  void intel_frontbuffer_flush(struct drm_device *dev,
>                              unsigned frontbuffer_bits);
> -/**
> - * intel_frontbuffer_flip - synchronous frontbuffer flip
> - * @dev: DRM device
> - * @frontbuffer_bits: frontbuffer plane tracking bits
> - *
> - * This function gets called after scheduling a flip on @obj. This is for
> - * synchronous plane updates which will happen on the next vblank and which will
> - * not get delayed by pending gpu rendering.
> - *
> - * Can be called without any locks held.
> - */
> -static inline
> +inline
>  void intel_frontbuffer_flip(struct drm_device *dev,
> -                           unsigned frontbuffer_bits)
> -{
> -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> -}
> +                           unsigned frontbuffer_bits);
>
>  unsigned int intel_fb_align_height(struct drm_device *dev,
>                                    unsigned int height,
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 57095f54c1f2..c3559fde25e5 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>
>         intel_frontbuffer_flush(dev, frontbuffer_bits);
>  }
> +
> +/**
> + * intel_frontbuffer_flip - synchronous frontbuffer flip
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * This function gets called after scheduling a flip on @obj. This is for
> + * synchronous plane updates which will happen on the next vblank and which will
> + * not get delayed by pending gpu rendering.
> + *
> + * Can be called without any locks held.
> + */
> +
> +void intel_frontbuffer_flip(struct drm_device *dev,
> +                           unsigned frontbuffer_bits)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       mutex_lock(&dev_priv->fb_tracking.lock);
> +       dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;

I don't understand the purpose of the line above. Is this really
necessary? It seems that flip_bits is only necessary for the
asynchronous flip case, where someone schedules a flip then draws on
the buffer, so we don't flush the buffer at flip_complete. We do
unconditional flush at intel_frontbuffer_flip(), so it seems like we
shouldn't be touching flip_bits here.

I tested both this version and the version without the flip_bits
change, and both appear to solve the problem I can reproduce, so for
both:
Tested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

And, with the flip_bits problem explained or fixed:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

> +       /* Remove stale busy bits due to the old buffer. */
> +       dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> +       mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +       intel_frontbuffer_flush(dev, frontbuffer_bits);
> +}
> --
> 2.1.0
>



-- 
Paulo Zanoni