| Message ID | 20180216043322.22874-4-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > Preparing a framebuffer should not require a flush. _post_plane_update() > takes care of flushing when a flip is scheduled, this should be > sufficient for PSR and FBC. Makes sense. > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Also Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> to validate the flow through atomic. -Chris > --- > drivers/gpu/drm/i915/intel_display.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 24ca43424c44..c611855bf05a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12717,12 +12717,10 @@ intel_prepare_plane_fb(struct drm_plane *plane, > struct i915_vma *vma; > > vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); > - if (!IS_ERR(vma)) { > + if (!IS_ERR(vma)) > to_intel_plane_state(new_state)->vma = vma; > - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > - } else { > + else > ret = PTR_ERR(vma); > - } > } > > i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY); > -- > 2.14.1 >
On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > Preparing a framebuffer should not require a flush. _post_plane_update() > > takes care of flushing when a flip is scheduled, this should be > > sufficient for PSR and FBC. > > Makes sense. > I also think this might speed up the flips a bit by avoiding flushes. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Also > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > to validate the flow through atomic. > -Chris > > > --- > > drivers/gpu/drm/i915/intel_display.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 24ca43424c44..c611855bf05a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -12717,12 +12717,10 @@ intel_prepare_plane_fb(struct drm_plane *plane, > > struct i915_vma *vma; > > > > vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); > > - if (!IS_ERR(vma)) { > > + if (!IS_ERR(vma)) > > to_intel_plane_state(new_state)->vma = vma; > > - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > > - } else { > > + else > > ret = PTR_ERR(vma); > > - } > > } > > > > i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY); > > -- > > 2.14.1 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) >>> Preparing a framebuffer should not require a flush. _post_plane_update() >>> takes care of flushing when a flip is scheduled, this should be >>> sufficient for PSR and FBC. >> Makes sense. >> > I also think this might speed up the flips a bit by avoiding flushes. > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> Also >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> to validate the flow through atomic. >> -Chris >> Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? Then again, seems like frontbuffer tracking should be done per crtc..
On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > >>> takes care of flushing when a flip is scheduled, this should be > >>> sufficient for PSR and FBC. > >> Makes sense. > >> > > I also think this might speed up the flips a bit by avoiding flushes. > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >> Also > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> to validate the flow through atomic. > >> -Chris > >> > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > I have no context why it was removed, I'll have to understand that change and get back to you. > > Then again, seems like frontbuffer tracking should be done per crtc.. > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > >>> takes care of flushing when a flip is scheduled, this should be > > >>> sufficient for PSR and FBC. > > >> Makes sense. > > >> > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > >> Also > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >> to validate the flow through atomic. > > >> -Chris > > >> > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > I have no context why it was removed, I'll have to understand that > change and get back to you. Since we supposedly have hw nuke for both fbc and psr there doesn't seem to be much need to do anything for flips. I guess DRRS is the only thing that kinda needs it (not really, just avoids flipping with the slow timings). But I think DRRS should really be tied into the vblank stuff somehow so that we switch to the fast timings whenever a vblank interrupts are enabled.
On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > >>> takes care of flushing when a flip is scheduled, this should be > > > >>> sufficient for PSR and FBC. > > > >> Makes sense. > > > >> > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > >> Also > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > >> to validate the flow through atomic. > > > >> -Chris > > > >> > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > I have no context why it was removed, I'll have to understand that > > change and get back to you. > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > to be much need to do anything for flips. I guess DRRS is the only > thing that kinda needs it (not really, just avoids flipping with the > slow timings). But I think DRRS should really be tied into the vblank > stuff somehow so that we switch to the fast timings whenever a vblank > interrupts are enabled. Oh, I guess VLV/CHV PSR is what would need this. To do that properly (ie. main link off) I think we'd basically need to do a full modeset when exiting PSR, so it should probably handled somewhere higher up during modeset, and for other uses the frontbuffer tracking should perhaps just schedule a work to do the full modeset.
On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > >>> sufficient for PSR and FBC. > > > > >> Makes sense. > > > > >> > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > >> Also > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > >> to validate the flow through atomic. > > > > >> -Chris > > > > >> > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > change and get back to you. > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > to be much need to do anything for flips. I guess DRRS is the only > > thing that kinda needs it (not really, just avoids flipping with the > > slow timings). But I think DRRS should really be tied into the vblank > > stuff somehow so that we switch to the fast timings whenever a vblank > > interrupts are enabled. > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > (ie. main link off) I think we'd basically need to do a full modeset > when exiting PSR, so it should probably handled somewhere higher up > during modeset, and for other uses the frontbuffer tracking > should perhaps just schedule a work to do the full modeset. > The mention of "full modeset" got me thinking. I believe you said full modeset because the link needs to be trained on PSR exit if it was off. But, link off isn't supported on VLV/CHV else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) /* On VLV and CHV only standby mode is supported. */ dev_priv->psr.link_standby = true; So we are good here?
On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > >>> sufficient for PSR and FBC. > > > > > >> Makes sense. > > > > > >> > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > >> Also > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > >> to validate the flow through atomic. > > > > > >> -Chris > > > > > >> > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > change and get back to you. > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > to be much need to do anything for flips. I guess DRRS is the only > > > thing that kinda needs it (not really, just avoids flipping with the > > > slow timings). But I think DRRS should really be tied into the vblank > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > interrupts are enabled. > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > (ie. main link off) I think we'd basically need to do a full modeset > > when exiting PSR, so it should probably handled somewhere higher up > > during modeset, and for other uses the frontbuffer tracking > > should perhaps just schedule a work to do the full modeset. > > > The mention of "full modeset" got me thinking. I believe you said full > modeset because the link needs to be trained on PSR exit if it was off. > But, link off isn't supported on VLV/CHV > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > /* On VLV and CHV only standby mode is supported. */ > dev_priv->psr.link_standby = true; I think that's just because we've been lazy and done it. I think even with the link off we'd need to reprogram all planes at least.
On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote: > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > > >>> sufficient for PSR and FBC. > > > > > > >> Makes sense. > > > > > > >> > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > >> Also > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > >> to validate the flow through atomic. > > > > > > >> -Chris > > > > > > >> > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > > change and get back to you. > > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > > to be much need to do anything for flips. I guess DRRS is the only > > > > thing that kinda needs it (not really, just avoids flipping with the > > > > slow timings). But I think DRRS should really be tied into the vblank > > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > > interrupts are enabled. > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > > (ie. main link off) I think we'd basically need to do a full modeset > > > when exiting PSR, so it should probably handled somewhere higher up > > > during modeset, and for other uses the frontbuffer tracking > > > should perhaps just schedule a work to do the full modeset. > > > > > The mention of "full modeset" got me thinking. I believe you said full > > modeset because the link needs to be trained on PSR exit if it was off. > > But, link off isn't supported on VLV/CHV > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > /* On VLV and CHV only standby mode is supported. */ > > dev_priv->psr.link_standby = true; > > I think that's just because we've been lazy and done it. I think even > with the link off we'd need to reprogram all planes at least. Not had coffee yet today, and it shows. Let me rewrite that entire thing: I think that's just because we've been lazy and not done it (link off mode). I think even without the link off we'd need to reprogram all planes at least.
On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote: > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote: > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > > > >>> sufficient for PSR and FBC. > > > > > > > >> Makes sense. > > > > > > > >> > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > > >> Also > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > >> to validate the flow through atomic. > > > > > > > >> -Chris > > > > > > > >> > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > > > change and get back to you. > > > > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > > > to be much need to do anything for flips. I guess DRRS is the only > > > > > thing that kinda needs it (not really, just avoids flipping with the > > > > > slow timings). But I think DRRS should really be tied into the vblank > > > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > > > interrupts are enabled. > > > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > > > (ie. main link off) I think we'd basically need to do a full modeset > > > > when exiting PSR, so it should probably handled somewhere higher up > > > > during modeset, and for other uses the frontbuffer tracking > > > > should perhaps just schedule a work to do the full modeset. > > > > > > > The mention of "full modeset" got me thinking. I believe you said full > > > modeset because the link needs to be trained on PSR exit if it was off. > > > But, link off isn't supported on VLV/CHV > > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > /* On VLV and CHV only standby mode is supported. */ > > > dev_priv->psr.link_standby = true; > > > > I think that's just because we've been lazy and done it. I think even > > with the link off we'd need to reprogram all planes at least. > > Not had coffee yet today, and it shows. Let me rewrite that entire thing: > > I think that's just because we've been lazy and not done it (link off mode). I agree with you here. This was probably exactly what was missing. But, how to do this without flickering (blinking?!) the screen? > I think even without the link off we'd need to reprogram all planes at least. on VLV/CHV or everywhere? and why do you think that? > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote: > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote: > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote: > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > > > > >>> sufficient for PSR and FBC. > > > > > > > > >> Makes sense. > > > > > > > > >> > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > > > >> Also > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > >> to validate the flow through atomic. > > > > > > > > >> -Chris > > > > > > > > >> > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > > > > change and get back to you. > > > > > > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > > > > to be much need to do anything for flips. I guess DRRS is the only > > > > > > thing that kinda needs it (not really, just avoids flipping with the > > > > > > slow timings). But I think DRRS should really be tied into the vblank > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > > > > interrupts are enabled. > > > > > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > > > > (ie. main link off) I think we'd basically need to do a full modeset > > > > > when exiting PSR, so it should probably handled somewhere higher up > > > > > during modeset, and for other uses the frontbuffer tracking > > > > > should perhaps just schedule a work to do the full modeset. > > > > > > > > > The mention of "full modeset" got me thinking. I believe you said full > > > > modeset because the link needs to be trained on PSR exit if it was off. > > > > But, link off isn't supported on VLV/CHV > > > > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > /* On VLV and CHV only standby mode is supported. */ > > > > dev_priv->psr.link_standby = true; > > > > > > I think that's just because we've been lazy and done it. I think even > > > with the link off we'd need to reprogram all planes at least. > > > > Not had coffee yet today, and it shows. Let me rewrite that entire thing: > > > > I think that's just because we've been lazy and not done it (link off mode). > > I agree with you here. This was probably exactly what was missing. > But, how to do this without flickering (blinking?!) the screen? > > > I think even without the link off we'd need to reprogram all planes at least. > > on VLV/CHV or everywhere? and why do you think that? VLV/CHV. I believe the hw implements PSR by simplty turning off the planes (and pipes+dplls for link off), but it doesn't have any automagic stuff for recovering the original state. It's all up to the driver. IIRC Rodrigo even confirmed this at some point, or at least he had to trigger a plane update to get something visible on the screen.
On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote: > On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote: > > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote: > > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote: > > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > > > > > >>> sufficient for PSR and FBC. > > > > > > > > > >> Makes sense. > > > > > > > > > >> > > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > > > > >> Also > > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > > >> to validate the flow through atomic. > > > > > > > > > >> -Chris > > > > > > > > > >> > > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > > > > > change and get back to you. > > > > > > > > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > > > > > to be much need to do anything for flips. I guess DRRS is the only > > > > > > > thing that kinda needs it (not really, just avoids flipping with the > > > > > > > slow timings). But I think DRRS should really be tied into the vblank > > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > > > > > interrupts are enabled. > > > > > > > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > > > > > (ie. main link off) I think we'd basically need to do a full modeset > > > > > > when exiting PSR, so it should probably handled somewhere higher up > > > > > > during modeset, and for other uses the frontbuffer tracking > > > > > > should perhaps just schedule a work to do the full modeset. > > > > > > > > > > > The mention of "full modeset" got me thinking. I believe you said full > > > > > modeset because the link needs to be trained on PSR exit if it was off. > > > > > But, link off isn't supported on VLV/CHV > > > > > > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > /* On VLV and CHV only standby mode is supported. */ > > > > > dev_priv->psr.link_standby = true; > > > > > > > > I think that's just because we've been lazy and done it. I think even > > > > with the link off we'd need to reprogram all planes at least. > > > > > > Not had coffee yet today, and it shows. Let me rewrite that entire thing: > > > > > > I think that's just because we've been lazy and not done it (link off mode). > > > > I agree with you here. This was probably exactly what was missing. > > But, how to do this without flickering (blinking?!) the screen? > > > > > I think even without the link off we'd need to reprogram all planes at least. > > > > on VLV/CHV or everywhere? and why do you think that? > > VLV/CHV. I believe the hw implements PSR by simplty turning off the > planes (and pipes+dplls for link off), but it doesn't have any automagic > stuff for recovering the original state. It's all up to the driver. > > IIRC Rodrigo even confirmed this at some point, or at least he had to > trigger a plane update to get something visible on the screen. My memory is terrible. But this makes so much sense and in sync with some vague memory flashes here :) And probably what blocked me for looking there besides the other bugs on core platforms was my disbelieve that would be possible without seeing any blink of a blank screen when exiting PSR. :/ > > -- > Ville Syrjälä > Intel OTC
On Thu, 2018-03-01 at 11:00 -0800, Rodrigo Vivi wrote: > On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote: > > On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote: > > > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote: > > > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote: > > > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > > > > > > >>> sufficient for PSR and FBC. > > > > > > > > > > >> Makes sense. > > > > > > > > > > >> > > > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > > > > > >> Also > > > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > > > >> to validate the flow through atomic. > > > > > > > > > > >> -Chris > > > > > > > > > > >> > > > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > > > > > > change and get back to you. > > > > > > > > > > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > > > > > > to be much need to do anything for flips. I guess DRRS is the only > > > > > > > > thing that kinda needs it (not really, just avoids flipping with the > > > > > > > > slow timings). But I think DRRS should really be tied into the vblank > > > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > > > > > > interrupts are enabled. > > > > > > > > > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > > > > > > (ie. main link off) I think we'd basically need to do a full modeset > > > > > > > when exiting PSR, so it should probably handled somewhere higher up > > > > > > > during modeset, and for other uses the frontbuffer tracking > > > > > > > should perhaps just schedule a work to do the full modeset. > > > > > > > > > > > > > The mention of "full modeset" got me thinking. I believe you said full > > > > > > modeset because the link needs to be trained on PSR exit if it was off. > > > > > > But, link off isn't supported on VLV/CHV > > > > > > > > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > > /* On VLV and CHV only standby mode is supported. */ > > > > > > dev_priv->psr.link_standby = true; > > > > > > > > > > I think that's just because we've been lazy and done it. I think even > > > > > with the link off we'd need to reprogram all planes at least. > > > > > > > > Not had coffee yet today, and it shows. Let me rewrite that entire thing: > > > > > > > > I think that's just because we've been lazy and not done it (link off mode). > > > > > > I agree with you here. This was probably exactly what was missing. > > > But, how to do this without flickering (blinking?!) the screen? > > > > > > > I think even without the link off we'd need to reprogram all planes at least. > > > > > > on VLV/CHV or everywhere? and why do you think that? > > > > VLV/CHV. I believe the hw implements PSR by simplty turning off the > > planes (and pipes+dplls for link off), but it doesn't have any automagic > > stuff for recovering the original state. It's all up to the driver. > > > > IIRC Rodrigo even confirmed this at some point, or at least he had to > > trigger a plane update to get something visible on the screen. > > My memory is terrible. But this makes so much sense and in sync > with some vague memory flashes here :) > > And probably what blocked me for looking there besides the > other bugs on core platforms was my disbelieve that would > be possible without seeing any blink of a blank screen when > exiting PSR. :/ > We've got a couple of options here - 1) Drop this patch because it breaks VLV/CHV link standby. 2) Keep this patch because VLV/CHV link standby is already broken (no plane programming on exit). Follow this up with reworking PSR on VLV/CHV. 3) Or should this be - if (VLV() || CHV) intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > > > > -- > > Ville Syrjälä > > Intel OTC
On Thu, Mar 01, 2018 at 11:00:40AM -0800, Rodrigo Vivi wrote: > On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote: > > On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote: > > > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote: > > > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote: > > > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote: > > > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote: > > > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote: > > > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran: > > > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote: > > > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21) > > > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update() > > > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be > > > > > > > > > > >>> sufficient for PSR and FBC. > > > > > > > > > > >> Makes sense. > > > > > > > > > > >> > > > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. > > > > > > > > > > > > > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > > > > > >> Also > > > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > > > >> to validate the flow through atomic. > > > > > > > > > > >> -Chris > > > > > > > > > > >> > > > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that? > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have no context why it was removed, I'll have to understand that > > > > > > > > > change and get back to you. > > > > > > > > > > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem > > > > > > > > to be much need to do anything for flips. I guess DRRS is the only > > > > > > > > thing that kinda needs it (not really, just avoids flipping with the > > > > > > > > slow timings). But I think DRRS should really be tied into the vblank > > > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank > > > > > > > > interrupts are enabled. > > > > > > > > > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly > > > > > > > (ie. main link off) I think we'd basically need to do a full modeset > > > > > > > when exiting PSR, so it should probably handled somewhere higher up > > > > > > > during modeset, and for other uses the frontbuffer tracking > > > > > > > should perhaps just schedule a work to do the full modeset. > > > > > > > > > > > > > The mention of "full modeset" got me thinking. I believe you said full > > > > > > modeset because the link needs to be trained on PSR exit if it was off. > > > > > > But, link off isn't supported on VLV/CHV > > > > > > > > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > > /* On VLV and CHV only standby mode is supported. */ > > > > > > dev_priv->psr.link_standby = true; > > > > > > > > > > I think that's just because we've been lazy and done it. I think even > > > > > with the link off we'd need to reprogram all planes at least. > > > > > > > > Not had coffee yet today, and it shows. Let me rewrite that entire thing: > > > > > > > > I think that's just because we've been lazy and not done it (link off mode). > > > > > > I agree with you here. This was probably exactly what was missing. > > > But, how to do this without flickering (blinking?!) the screen? > > > > > > > I think even without the link off we'd need to reprogram all planes at least. > > > > > > on VLV/CHV or everywhere? and why do you think that? > > > > VLV/CHV. I believe the hw implements PSR by simplty turning off the > > planes (and pipes+dplls for link off), but it doesn't have any automagic > > stuff for recovering the original state. It's all up to the driver. > > > > IIRC Rodrigo even confirmed this at some point, or at least he had to > > trigger a plane update to get something visible on the screen. > > My memory is terrible. But this makes so much sense and in sync > with some vague memory flashes here :) > > And probably what blocked me for looking there besides the > other bugs on core platforms was my disbelieve that would > be possible without seeing any blink of a blank screen when > exiting PSR. :/ I suppose we should start the pipe/planes back up, and only then we can tell the sink to stop using the old stored frame.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 24ca43424c44..c611855bf05a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12717,12 +12717,10 @@ intel_prepare_plane_fb(struct drm_plane *plane, struct i915_vma *vma; vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); - if (!IS_ERR(vma)) { + if (!IS_ERR(vma)) to_intel_plane_state(new_state)->vma = vma; - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); - } else { + else ret = PTR_ERR(vma); - } } i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
Preparing a framebuffer should not require a flush. _post_plane_update() takes care of flushing when a flip is scheduled, this should be sufficient for PSR and FBC. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)