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

[Chris Wilson]

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.

> Change-Id: Ief8a250ffc96d961b444c5dd86bcbe219a3a7000
What does this mean to linux-kernel?

> Signed-off-by: vkannan <vandana.kannan at intel.com>
> Signed-off-by: Pradeep Bhat <pradeep.bhat at intel.com>
> Signed-off-by: Shanth Kumar <shanth.kumarx.shivalingappa at 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