| Message ID | 1393631086-3880-11-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: > Baytrail cannot easily detect screen updates and force PSR exit. > So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} > and update to enable it back it later with a delayed workqueue. Why are we not checking if the object being accessed is indeed being used for PSR? In set-domain, you only care about writes. sw-finish and busy are too late for psr_exit, the damage has already been done and presumably the content may already be corrupted? Can you please explain that it is safe to do an psr_exit after the damage is already in the scanount based on the panel refresh cycle. -Chris
On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: >> Baytrail cannot easily detect screen updates and force PSR exit. >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} >> and update to enable it back it later with a delayed workqueue. > > Why are we not checking if the object being accessed is indeed being > used for PSR? In set-domain, you only care about writes. Are y > sw-finish and > busy are too late for psr_exit, the damage has already been done and > presumably the content may already be corrupted? Can you please explain > that it is safe to do an psr_exit after the damage is already in the scanount > based on the panel refresh cycle. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: >> Baytrail cannot easily detect screen updates and force PSR exit. >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} >> and update to enable it back it later with a delayed workqueue. > > Why are we not checking if the object being accessed is indeed being > used for PSR? In set-domain, you only care about writes. Ok, so here you are suggesting that I on trigger psr_exit if it is write domain, right? I agree. > sw-finish and > busy are too late for psr_exit, the damage has already been done and > presumably the content may already be corrupted? I also agree but it is better late than never... or than letting userspace fully control the psr exit. Most of the cases are coverred by psr_exit at set_domain. The remaining cases where userspace set domain once, sleep for while (like 10 seconds) and than write something was coverred by my test that checked crc and it was different from crc base already. > Can you please explain > that it is safe to do an psr_exit after the damage is already in the scanount > based on the panel refresh cycle. The perfect solution is the hardware tracking the changes and doing psr_exit by itself, but unfortunatelly se don't have this scenario so we can live with what we have. If the idea is to allow userspace to let kernel know when to exit psr I'm in favor of a more generic ioctl called pre_damage to or a front_buffer_rend something to warn when some damage will occur and we can disable psr, fbc and do whatever we want before the damage if you prefer I can remove this patch and add the patch with ioclts that let psr_exit full control to userspace. These patches are ready already. I just really don't like this option because I don't like the idea to depend on userspace to get this hardware feature working. Any other suggestion or ideas or cases, that I might be missing or not seeing, are welcome! > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre Thanks
On Sat, Mar 01, 2014 at 03:29:41PM -0300, Rodrigo Vivi wrote: > On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: > >> Baytrail cannot easily detect screen updates and force PSR exit. > >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} > >> and update to enable it back it later with a delayed workqueue. > > > > Why are we not checking if the object being accessed is indeed being > > used for PSR? In set-domain, you only care about writes. > > Ok, so here you are suggesting that I on trigger psr_exit if it is > write domain, right? > I agree. > > > sw-finish and > > busy are too late for psr_exit, the damage has already been done and > > presumably the content may already be corrupted? > > I also agree but it is better late than never... or than letting > userspace fully control the psr exit. > Most of the cases are coverred by psr_exit at set_domain. > The remaining cases where userspace set domain once, sleep for while > (like 10 seconds) and than write something > was coverred by my test that checked crc and it was different from crc > base already. > > > Can you please explain > > that it is safe to do an psr_exit after the damage is already in the scanount > > based on the panel refresh cycle. > > The perfect solution is the hardware tracking the changes and doing > psr_exit by itself, > but unfortunatelly se don't have this scenario so we can live with what we have. > > If the idea is to allow userspace to let kernel know when to exit psr > I'm in favor of a more generic > ioctl called pre_damage to or a front_buffer_rend something to warn > when some damage will occur > and we can disable psr, fbc and do whatever we want before the damage > > if you prefer I can remove this patch and add the patch with ioclts > that let psr_exit full control to userspace. > These patches are ready already. I just really don't like this option > because I don't like the idea to depend > on userspace to get this hardware feature working. This is the level of detail that I want in the changelog and in nearby comments. Keeping track of the weaknesses of the implementation is vital. For example there are problems with this approach if userspace flips between two GTT mmapped buffers, it currently has no reason to call set-domain again (and it never calls sw-finish along this path). So PSR is enabled again, and userspace may continue to directly write into the scanout without the kernel ever detecting and issuing the psr_exit(). I do not believe there is a way to change the coherency semantics of the GTT domain without userspace being aware, or else run afoul of a popular corner case. -Chris
On Sat, Mar 1, 2014 at 6:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Mar 01, 2014 at 03:29:41PM -0300, Rodrigo Vivi wrote: >> On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} >> >> and update to enable it back it later with a delayed workqueue. >> > >> > Why are we not checking if the object being accessed is indeed being >> > used for PSR? In set-domain, you only care about writes. >> >> Ok, so here you are suggesting that I on trigger psr_exit if it is >> write domain, right? >> I agree. >> >> > sw-finish and >> > busy are too late for psr_exit, the damage has already been done and >> > presumably the content may already be corrupted? >> >> I also agree but it is better late than never... or than letting >> userspace fully control the psr exit. >> Most of the cases are coverred by psr_exit at set_domain. >> The remaining cases where userspace set domain once, sleep for while >> (like 10 seconds) and than write something >> was coverred by my test that checked crc and it was different from crc >> base already. >> >> > Can you please explain >> > that it is safe to do an psr_exit after the damage is already in the scanount >> > based on the panel refresh cycle. >> >> The perfect solution is the hardware tracking the changes and doing >> psr_exit by itself, >> but unfortunatelly se don't have this scenario so we can live with what we have. >> >> If the idea is to allow userspace to let kernel know when to exit psr >> I'm in favor of a more generic >> ioctl called pre_damage to or a front_buffer_rend something to warn >> when some damage will occur >> and we can disable psr, fbc and do whatever we want before the damage >> >> if you prefer I can remove this patch and add the patch with ioclts >> that let psr_exit full control to userspace. >> These patches are ready already. I just really don't like this option >> because I don't like the idea to depend >> on userspace to get this hardware feature working. > > This is the level of detail that I want in the changelog and in nearby > comments. Keeping track of the weaknesses of the implementation is > vital. agree > For example there are problems with this approach if userspace flips > between two GTT mmapped buffers, it currently has no reason to call > set-domain again (and it never calls sw-finish along this path). So PSR > is enabled again, and userspace may continue to directly write into the > scanout without the kernel ever detecting and issuing the psr_exit(). I see... > I do not believe there is a way to change the coherency semantics of the > GTT domain without userspace being aware, or else run afoul of a popular > corner case. so, what do you suggest? ioctls for exit? > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Mar 03, 2014 at 01:09:36PM -0300, Rodrigo Vivi wrote: > On Sat, Mar 1, 2014 at 6:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Mar 01, 2014 at 03:29:41PM -0300, Rodrigo Vivi wrote: > >> On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: > >> >> Baytrail cannot easily detect screen updates and force PSR exit. > >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} > >> >> and update to enable it back it later with a delayed workqueue. > >> > > >> > Why are we not checking if the object being accessed is indeed being > >> > used for PSR? In set-domain, you only care about writes. > >> > >> Ok, so here you are suggesting that I on trigger psr_exit if it is > >> write domain, right? > >> I agree. > >> > >> > sw-finish and > >> > busy are too late for psr_exit, the damage has already been done and > >> > presumably the content may already be corrupted? > >> > >> I also agree but it is better late than never... or than letting > >> userspace fully control the psr exit. > >> Most of the cases are coverred by psr_exit at set_domain. > >> The remaining cases where userspace set domain once, sleep for while > >> (like 10 seconds) and than write something > >> was coverred by my test that checked crc and it was different from crc > >> base already. > >> > >> > Can you please explain > >> > that it is safe to do an psr_exit after the damage is already in the scanount > >> > based on the panel refresh cycle. > >> > >> The perfect solution is the hardware tracking the changes and doing > >> psr_exit by itself, > >> but unfortunatelly se don't have this scenario so we can live with what we have. > >> > >> If the idea is to allow userspace to let kernel know when to exit psr > >> I'm in favor of a more generic > >> ioctl called pre_damage to or a front_buffer_rend something to warn > >> when some damage will occur > >> and we can disable psr, fbc and do whatever we want before the damage > >> > >> if you prefer I can remove this patch and add the patch with ioclts > >> that let psr_exit full control to userspace. > >> These patches are ready already. I just really don't like this option > >> because I don't like the idea to depend > >> on userspace to get this hardware feature working. > > > > This is the level of detail that I want in the changelog and in nearby > > comments. Keeping track of the weaknesses of the implementation is > > vital. > > agree > > > For example there are problems with this approach if userspace flips > > between two GTT mmapped buffers, it currently has no reason to call > > set-domain again (and it never calls sw-finish along this path). So PSR > > is enabled again, and userspace may continue to directly write into the > > scanout without the kernel ever detecting and issuing the psr_exit(). > > I see... > > > I do not believe there is a way to change the coherency semantics of the > > GTT domain without userspace being aware, or else run afoul of a popular > > corner case. > > so, what do you suggest? > ioctls for exit? We have it, it's called dirtyfb. But we need to keep the current userspace stuff mostly working first, hence all this trouble. We can fix the gtt trouble with a delayed work (one vblank or so) which a) does a psr_exit b) shots down gtt mappings of the offending frontbuffer Then we can intercept the next write in gem_fault. Of course that will be horrible if we don't track correctly which buffers are actually relevant for psr. -Daniel
On Wed, Mar 5, 2014 at 2:46 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Mar 03, 2014 at 01:09:36PM -0300, Rodrigo Vivi wrote: >> On Sat, Mar 1, 2014 at 6:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Sat, Mar 01, 2014 at 03:29:41PM -0300, Rodrigo Vivi wrote: >> >> On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote: >> >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} >> >> >> and update to enable it back it later with a delayed workqueue. >> >> > >> >> > Why are we not checking if the object being accessed is indeed being >> >> > used for PSR? In set-domain, you only care about writes. >> >> >> >> Ok, so here you are suggesting that I on trigger psr_exit if it is >> >> write domain, right? >> >> I agree. >> >> >> >> > sw-finish and >> >> > busy are too late for psr_exit, the damage has already been done and >> >> > presumably the content may already be corrupted? >> >> >> >> I also agree but it is better late than never... or than letting >> >> userspace fully control the psr exit. >> >> Most of the cases are coverred by psr_exit at set_domain. >> >> The remaining cases where userspace set domain once, sleep for while >> >> (like 10 seconds) and than write something >> >> was coverred by my test that checked crc and it was different from crc >> >> base already. >> >> >> >> > Can you please explain >> >> > that it is safe to do an psr_exit after the damage is already in the scanount >> >> > based on the panel refresh cycle. >> >> >> >> The perfect solution is the hardware tracking the changes and doing >> >> psr_exit by itself, >> >> but unfortunatelly se don't have this scenario so we can live with what we have. >> >> >> >> If the idea is to allow userspace to let kernel know when to exit psr >> >> I'm in favor of a more generic >> >> ioctl called pre_damage to or a front_buffer_rend something to warn >> >> when some damage will occur >> >> and we can disable psr, fbc and do whatever we want before the damage >> >> >> >> if you prefer I can remove this patch and add the patch with ioclts >> >> that let psr_exit full control to userspace. >> >> These patches are ready already. I just really don't like this option >> >> because I don't like the idea to depend >> >> on userspace to get this hardware feature working. >> > >> > This is the level of detail that I want in the changelog and in nearby >> > comments. Keeping track of the weaknesses of the implementation is >> > vital. >> >> agree >> >> > For example there are problems with this approach if userspace flips >> > between two GTT mmapped buffers, it currently has no reason to call >> > set-domain again (and it never calls sw-finish along this path). So PSR >> > is enabled again, and userspace may continue to directly write into the >> > scanout without the kernel ever detecting and issuing the psr_exit(). >> >> I see... >> >> > I do not believe there is a way to change the coherency semantics of the >> > GTT domain without userspace being aware, or else run afoul of a popular >> > corner case. >> >> so, what do you suggest? >> ioctls for exit? > > We have it, it's called dirtyfb. But we need to keep the current userspace > stuff mostly working first, hence all this trouble. We can fix the gtt > trouble with a delayed work (one vblank or so) which > a) does a psr_exit > b) shots down gtt mappings of the offending frontbuffer > > Then we can intercept the next write in gem_fault. > > Of course that will be horrible if we don't track correctly which buffers > are actually relevant for psr. > -Daniel I'm not sure I fully got your idea/plan here. Should I try a psr_exit with delayed back at dirtyfb? and create a new test case? Or this it self isn't enough? Btw, i'm using daily a hsw ult with psr enable and a kde environment using the same psr_exit strategy here for baytrail with psr enabled for more than 2 weeks without any issue so far. > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Mar 12, 2014 at 6:11 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: >> We have it, it's called dirtyfb. But we need to keep the current userspace >> stuff mostly working first, hence all this trouble. We can fix the gtt >> trouble with a delayed work (one vblank or so) which >> a) does a psr_exit >> b) shots down gtt mappings of the offending frontbuffer >> >> Then we can intercept the next write in gem_fault. >> >> Of course that will be horrible if we don't track correctly which buffers >> are actually relevant for psr. >> -Daniel > > I'm not sure I fully got your idea/plan here. > > Should I try a psr_exit with delayed back at dirtyfb? and create a new > test case? > Or this it self isn't enough? > > Btw, i'm using daily a hsw ult with psr enable and a kde environment > using the same psr_exit strategy here for baytrail with psr enabled > for more than 2 weeks without any issue so far. Current userspace doesn't use dirtyfb, so we can't solely rely on that. Iirc you currently have a psr_exit in set_domain. My idea above was that we also need to have it in i915_gem_fault and further we need to shot down ptes in psr_exit for the frontbuffer. For an igt the following should trick this bug: 1) set_domain(GTT, GTT) on the frontbuffer 2) rendering something -> check that the kernel has correctly done a psr_exit with the sink crc stuff This is the testcase we already have. Now comes the interesting part Chris pointed out: 3) have a long delay to make sure we are again in psr mode. 4) Write new stuff to the frontbuffer, _without_ doing a set_domain(GTT, GTT). Userspace is allowed to do this. -> check again sink crcs Note that this issue doesn't exist with cpu memory mapped frontbuffer rendering, since userspace is required to regularly call the sw_finish ioctl, i.e. after both steps 2) and 4). But even for cpu mmaps userspace is allowed to optimize the set_domain(CPU, CPU) call away before step 3). Of course if any other rendering happens in between step 2) and 3) userspace must again do a set_domain call. So this case here is highly contrived ;-) I hope this explains this issue better. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 18f457e..a4e666c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -785,6 +785,8 @@ struct i915_psr { bool sink_support; bool source_ok; bool setup_done; + bool active; + struct delayed_work work; void (*setup)(struct intel_dp *intel_dp); void (*enable_sink)(struct intel_dp *intel_dp); void (*enable_source)(struct intel_dp *intel_dp); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3618bb0..c6bfc81 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1251,6 +1251,14 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, goto unlock; } + /* Here we don't reschedule work to get PSR back because userspace + * can set domain and take as much as time it wants to write it. + * We only reschedule it back on next finish_page_flip. There is the + * risk of PSR be inactive forever depending on how compositor works, + * but this is the safest way to avoid loosing screen updates. + */ + intel_edp_psr_exit(dev, false); + /* Try to flush the object off the GPU without holding the lock. * We will repeat the flush holding the lock in the normal manner * to catch cases where we are gazumped. @@ -1296,6 +1304,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, if (ret) return ret; + intel_edp_psr_exit(dev, true); + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); if (&obj->base == NULL) { ret = -ENOENT; @@ -3982,6 +3992,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, if (ret) return ret; + intel_edp_psr_exit(dev, true); + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); if (&obj->base == NULL) { ret = -ENOENT; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 26196b8..5e4c768 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4433,6 +4433,8 @@ static void intel_connector_check_state(struct intel_connector *connector) * consider. */ void intel_connector_dpms(struct drm_connector *connector, int mode) { + struct drm_device *dev = connector->dev; + struct intel_encoder *intel_encoder; /* All the simple cases only support two dpms states. */ if (mode != DRM_MODE_DPMS_ON) mode = DRM_MODE_DPMS_OFF; @@ -4443,9 +4445,15 @@ void intel_connector_dpms(struct drm_connector *connector, int mode) connector->dpms = mode; /* Only need to change hw state when actually enabled */ - if (connector->encoder) - intel_encoder_dpms(to_intel_encoder(connector->encoder), mode); + if (connector->encoder) { + + intel_encoder = to_intel_encoder(connector->encoder); + intel_encoder_dpms(intel_encoder, mode); + if (intel_encoder->type == INTEL_OUTPUT_EDP && + mode == DRM_MODE_DPMS_OFF) + intel_edp_psr_exit(dev, false); + } intel_modeset_check_state(connector->dev); } @@ -7512,6 +7520,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, if (!visible && !intel_crtc->cursor_visible) return; + intel_edp_psr_exit(dev, true); + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) { I915_WRITE(CURPOS_IVB(pipe), pos); ivb_update_cursor(crtc, base); @@ -8224,12 +8234,15 @@ void intel_mark_idle(struct drm_device *dev) gen6_rps_idle(dev->dev_private); } + void intel_mark_fb_busy(struct drm_i915_gem_object *obj, struct intel_ring_buffer *ring) { struct drm_device *dev = obj->base.dev; struct drm_crtc *crtc; + intel_edp_psr_exit(dev, true); + if (!i915.powersave) return; @@ -8330,6 +8343,10 @@ static void do_intel_finish_page_flip(struct drm_device *dev, queue_work(dev_priv->wq, &work->work); trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj); + + /* Get PSR back after set_domain inactivated it + * without rescheduling it back. */ + intel_edp_psr_update(dev); } void intel_finish_page_flip(struct drm_device *dev, int pipe) @@ -8696,6 +8713,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, if (work == NULL) return -ENOMEM; + /* Exit PSR early in page flip */ + intel_edp_psr_exit(dev, true); + work->event = event; work->crtc = crtc; work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3adc127..f386860 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1883,6 +1883,8 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) /* Enable PSR on the host */ dev_priv->psr.enable_source(intel_dp); + + dev_priv->psr.active = true; } void intel_edp_psr_enable(struct intel_dp *intel_dp) @@ -1919,6 +1921,8 @@ void vlv_edp_psr_disable(struct intel_dp *intel_dp) if (!dev_priv->psr.setup_done) return; + intel_edp_psr_exit(dev, false); + if (wait_for((I915_READ(VLV_PSRSTAT(intel_crtc->pipe)) & VLV_EDP_PSR_IN_TRANS) == 0, 250)) WARN(1, "PSR transition took longer than expected\n"); @@ -1947,18 +1951,11 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp) DRM_ERROR("Timed out waiting for PSR Idle State\n"); } -void intel_edp_psr_update(struct drm_device *dev) +void intel_edp_psr_do_update(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_encoder *encoder; struct intel_dp *intel_dp = NULL; - if (!HAS_PSR(dev)) - return; - - if (!dev_priv->psr.setup_done) - return; - list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) if (encoder->type == INTEL_OUTPUT_EDP) { intel_dp = enc_to_intel_dp(&encoder->base); @@ -1973,6 +1970,90 @@ void intel_edp_psr_update(struct drm_device *dev) } } +void intel_edp_psr_update(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + if (!HAS_PSR(dev)) + return; + + if (!dev_priv->psr.setup_done) + return; + + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_exit(dev, true); + else + intel_edp_psr_do_update(dev); +} + +void intel_edp_psr_work(struct work_struct *work) +{ + struct drm_i915_private *dev_priv = + container_of(work, typeof(*dev_priv), psr.work.work); + struct drm_device *dev = dev_priv->dev; + + intel_edp_psr_do_update(dev); +} + +void intel_edp_psr_inactivate(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_connector *connector; + struct intel_encoder *encoder; + struct intel_crtc *intel_crtc; + struct intel_dp *intel_dp = NULL; + + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) { + + if (connector->base.dpms != DRM_MODE_DPMS_ON) + continue; + + encoder = to_intel_encoder(connector->base.encoder); + if (encoder->type == INTEL_OUTPUT_EDP) { + + intel_dp = enc_to_intel_dp(&encoder->base); + intel_crtc = to_intel_crtc(encoder->base.crtc); + + if (!vlv_edp_is_psr_enabled_on_pipe(dev, + intel_crtc->pipe)) + continue; + + dev_priv->psr.active = false; + + I915_WRITE(VLV_PSRCTL(intel_crtc->pipe), + VLV_EDP_PSR_RESET); + /* WaClearPSRReset:vlv */ + I915_WRITE(VLV_PSRCTL(intel_crtc->pipe), 0); + + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); + } + } +} + +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + if (!HAS_PSR(dev)) + return; + + if (!IS_VALLEYVIEW(dev)) + return; + + if (!dev_priv->psr.setup_done) + return; + + cancel_delayed_work_sync(&dev_priv->psr.work); + + if (dev_priv->psr.active) + intel_edp_psr_inactivate(dev); + + if (schedule_back) + schedule_delayed_work(&dev_priv->psr.work, + msecs_to_jiffies(5000)); +} + void intel_edp_psr_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -1980,6 +2061,8 @@ void intel_edp_psr_init(struct drm_device *dev) if (!HAS_PSR(dev)) return; + INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work); + if (IS_VALLEYVIEW(dev)) { dev_priv->psr.setup = vlv_edp_psr_setup; dev_priv->psr.enable_sink = vlv_edp_psr_enable_sink; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2e4516d..47b8c9f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -757,6 +757,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp); void intel_edp_psr_enable(struct intel_dp *intel_dp); void intel_edp_psr_disable(struct intel_dp *intel_dp); void intel_edp_psr_update(struct drm_device *dev); +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back); void intel_edp_psr_init(struct drm_device *dev);
Baytrail cannot easily detect screen updates and force PSR exit. So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy} and update to enable it back it later with a delayed workqueue. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_gem.c | 12 +++++ drivers/gpu/drm/i915/intel_display.c | 24 ++++++++- drivers/gpu/drm/i915/intel_dp.c | 99 +++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 1 + 5 files changed, 128 insertions(+), 10 deletions(-)