| Message ID | 1436311737-18270-7-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6749
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -2 287/287 285/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads-display PASS(1) FAIL(1)
*BYT igt@gem_partial_pwrite_pread@reads-uncached PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > This fbdev restore mode was another corner case that was now > calling frontbuffer flip and flush and making we miss > screen updates with PSR enabled. > > So let's also add the invalidate hack here while we don't have > a reliable dirty fbdev op. So when I saw patches 6 and 7 I decided to investigate how fbcon interacts with frontbuffer tracking. One thing that I notice is that, without this patch, if I kill the display manager, I'll have a bunch of flushes without an invalidate when returning to fbcon. And I suppose we don't want PSR/FBC enabled on fbcon, so this patch seems to fix a bug. By the way, I think we can try to simulate this "kill display manager" bug on IGT. We could try to close the DRM device and then check if FBC/PSR stopped. I guess it's probably easier to create a new IGT test for that. More below. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index a76cebc..ae9b809 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -831,11 +831,18 @@ void intel_fbdev_restore_mode(struct drm_device *dev) > { > int ret; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_fbdev *ifbdev = dev_priv->fbdev; > + struct drm_fb_helper *fb_helper = &ifbdev->helper; Can't this ifbdev->helper assignment segfault? Shouldn't we assign this pointer just after the !ifbdev check below? > > - if (!dev_priv->fbdev) > + if (!ifbdev) > return; > > - ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper); > + ret = drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper); You could have used fb_helper here :) > if (ret) > DRM_DEBUG("failed to restore crtc mode\n"); My OCD tells me to request you to add the brackets on the "if" too. Documentation/CodingStyle:168 > + else { > + mutex_lock(&fb_helper->dev->struct_mutex); > + intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT); > + mutex_unlock(&fb_helper->dev->struct_mutex); > + } > } > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 08, 2015 at 03:05:49PM -0300, Paulo Zanoni wrote: > 2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > This fbdev restore mode was another corner case that was now > > calling frontbuffer flip and flush and making we miss > > screen updates with PSR enabled. > > > > So let's also add the invalidate hack here while we don't have > > a reliable dirty fbdev op. > > So when I saw patches 6 and 7 I decided to investigate how fbcon > interacts with frontbuffer tracking. One thing that I notice is that, > without this patch, if I kill the display manager, I'll have a bunch > of flushes without an invalidate when returning to fbcon. And I > suppose we don't want PSR/FBC enabled on fbcon, so this patch seems to > fix a bug. > > By the way, I think we can try to simulate this "kill display manager" > bug on IGT. We could try to close the DRM device and then check if > FBC/PSR stopped. I guess it's probably easier to create a new IGT test > for that. Hm yeah that would be a nice testcase indeed. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index a76cebc..ae9b809 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -831,11 +831,18 @@ void intel_fbdev_restore_mode(struct drm_device *dev) { int ret; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_fbdev *ifbdev = dev_priv->fbdev; + struct drm_fb_helper *fb_helper = &ifbdev->helper; - if (!dev_priv->fbdev) + if (!ifbdev) return; - ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper); + ret = drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper); if (ret) DRM_DEBUG("failed to restore crtc mode\n"); + else { + mutex_lock(&fb_helper->dev->struct_mutex); + intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT); + mutex_unlock(&fb_helper->dev->struct_mutex); + } }
This fbdev restore mode was another corner case that was now calling frontbuffer flip and flush and making we miss screen updates with PSR enabled. So let's also add the invalidate hack here while we don't have a reliable dirty fbdev op. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)