Re: [PATCH] [VPG HSW-A] drm/i915: Fix for YouTube full screen video playback flicker issue

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 20, 2013 at 10:31:19AM +0100, Chris Wilson wrote:
> On Fri, Sep 20, 2013 at 11:30:07AM +0530, Shanth Kumar wrote:
> > During full screen video playback on Primary Display[eDP],
> > when the player UI(player controls) is superimposed on video frame,
> > the Sprite plane is disabled and rendering happens on Primary plane.
> > And when the player UI fades away, the Primary plane is disabled,
> > and rendering happens on Sprite plane. This is done for power saving purpose.
> > While disabling Primary plane or Sprite plane we see a momentary flicker.
> > This patch waits on a vblank before disabling the Primary or Sprite plane.
> > Detailed testing was done with this patch to confirm issue is not reproduced.
> > No performance drop or visible user experience degradation seen with this
> > change.
> > 
> > HSD ID for this issue is 5180815
> > 
> > Change Details-
> > drm/i915: intel_wait_for_vblank() is introduced before calling
> > intel_disable_primary() in function intel_update_plane(intel_sprite.c),
> > after checking to_intel_crtc(crtc)->primary_disabled flag being false.
> > drm/i915: intel_wait_for_vblank() is introduced before calling
> > intel_plane->disable_plane() in function intel_update_plane(intel_sprite.c)
> 
> Please don't do this. The patch is far easier to read in unified diff.
> The only time this discussion of the implementation is interesting is
> if there is something strange that either you noticed during writing or
> review. Such oddities deserve addition comments in both the code and
> changelog.

While we bikeshed the commit message: The patch headline should mention
what is fixed on a more technical level - after all you don't fix youtube
specifically, but sprite/primary plane only switching in general.
Mentioned the impact of the patch specifically (or how the issue was
discovered) can then be done in the commit message itself.

The other issue is that all these registers should be vblank synced, and
we're already careful to update them in the right order. So I wonder what
exactly is broken here:
- Either the hw is busted, and then we probably need a specific w/a in
  other places.
- Or userspace renders into these buffers while they're still getting
  scanned out. I suspect it's simply more visible when doing big changes
  like disabling the primary plane. If that's the case (and I suspect it
  is) then we just need to fix userspace.

A minimalistic testcase could be used to make the call here I think. But I
haven't seen the nature of the corruption, so no idea really.

Cheers, Daniel
> 
> > Change-Id: Ief8a250ffc96d961b444c5dd86bcbe219a3a7000
> What does this mean to linux-kernel?
> 
> > Signed-off-by: vkannan <vandana.kannan@intel.com>
> > Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> > Signed-off-by: Shanth Kumar <shanth.kumarx.shivalingappa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 1fa5612..470008a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -824,8 +824,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	else
> >  		intel_plane->disable_plane(plane);
> >  
> > -	if (disable_primary)
> > +	if (disable_primary && !(to_intel_crtc(crtc)->primary_disabled)) {
> > +		intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
> >  		intel_disable_primary(crtc);
> > +	}
> >  
> >  	/* Unpin old obj after new one is active to avoid ugliness */
> >  	if (old_obj) {
> > @@ -857,13 +859,13 @@ intel_disable_plane(struct drm_plane *plane)
> >  
> >  	if (plane->crtc)
> >  		intel_enable_primary(plane->crtc);
> > +
> > +	intel_wait_for_vblank(dev, intel_plane->pipe);
> >  	intel_plane->disable_plane(plane);
> 
> What's the justification for this wait here? Judging by the changelog
> you want to wait for a vblank after touching the primary, in which case
> moving the wait into the if (plane->crtc) { enable_primary(); } branch
> makes sense.
> 
> >  
> >  	if (!intel_plane->obj)
> >  		goto out;
> >  
> > -	intel_wait_for_vblank(dev, intel_plane->pipe);
> > -
> 
> This wait is still required.
> 
> >  	mutex_lock(&dev->struct_mutex);
> >  	intel_unpin_fb_obj(intel_plane->obj);
> >  	intel_plane->obj = NULL;
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch