| Message ID | 1379656807-14108-1-git-send-email-shanth.kumarx.shivalingappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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;
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
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); if (!intel_plane->obj) goto out; - intel_wait_for_vblank(dev, intel_plane->pipe); - mutex_lock(&dev->struct_mutex); intel_unpin_fb_obj(intel_plane->obj); intel_plane->obj = NULL;