| Message ID | 1465436764-29950-4-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, [auto build test WARNING on v4.7-rc2] [cannot apply to drm-intel/for-linux-next drm/drm-next next-20160608] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Dhinakaran-Pandiyan/CHV-vblank-failures-when-PSR-is-active/20160609-094028 reproduce: make htmldocs All warnings (new ones prefixed by >>): drivers/gpu/drm/drm_fb_cma_helper.c:233: warning: No description found for parameter 'dev' drivers/gpu/drm/drm_fb_cma_helper.c:233: warning: No description found for parameter 'file_priv' drivers/gpu/drm/drm_fb_cma_helper.c:233: warning: No description found for parameter 'mode_cmd' drivers/gpu/drm/drm_fb_cma_helper.c:285: warning: No description found for parameter 'm' drivers/gpu/drm/drm_fb_cma_helper.c:285: warning: No description found for parameter 'arg' include/drm/drm_dp_helper.h:750: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:750: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2383: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2383: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:705: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:715: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:725: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:779: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/intel_runtime_pm.c:2411: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/intel_runtime_pm.c:2411: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1231: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1446: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1473: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1473: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1590: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1590: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1590: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1653: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1653: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1653: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1698: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1698: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1698: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'size' drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'tiling_mode' drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'fenced' drivers/gpu/drm/i915/i915_gem.c:2006: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment' drivers/gpu/drm/i915/i915_gem.c:2960: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_gem.c:3086: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3136: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:3136: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:3136: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:3136: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl' drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'vm' drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'ggtt_view' drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'alignment' drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:3754: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3754: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:3832: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3832: warning: No description found for parameter 'cache_level' drivers/gpu/drm/i915/i915_gem.c:4106: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:4106: warning: No description found for parameter 'write' >> drivers/gpu/drm/i915/intel_psr.c:739: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/intel_psr.c:767: warning: No description found for parameter 'dev' >> drivers/gpu/drm/i915/intel_psr.c:739: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/intel_psr.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser' drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser' drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds' drivers/gpu/drm/i915/intel_lrc.c:316: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:316: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update' drivers/gpu/drm/i915/intel_lrc.c:353: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:353: warning: Excess function parameter 'ring' description in 'intel_execlists_ctx_id' drivers/gpu/drm/i915/intel_lrc.c:544: warning: No description found for parameter 'data' drivers/gpu/drm/i915/intel_lrc.c:544: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler' drivers/gpu/drm/i915/intel_lrc.c:810: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:1195: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:1195: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb' drivers/gpu/drm/i915/intel_lrc.c:1258: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:1258: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb' drivers/gpu/drm/i915/intel_lrc.c:1901: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:1901: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup' drivers/gpu/drm/i915/intel_lrc.c:2486: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:2486: warning: Excess function parameter 'ring' description in 'intel_lr_context_size' drivers/gpu/drm/i915/intel_lrc.c:2525: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:2525: warning: Excess function parameter 'ring' description in 'intel_lr_context_deferred_alloc' drivers/gpu/drm/i915/intel_lrc.c:316: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:316: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update' drivers/gpu/drm/i915/intel_lrc.c:353: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:353: warning: Excess function parameter 'ring' description in 'intel_execlists_ctx_id' drivers/gpu/drm/i915/intel_lrc.c:544: warning: No description found for parameter 'data' drivers/gpu/drm/i915/intel_lrc.c:544: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler' drivers/gpu/drm/i915/intel_lrc.c:810: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:1195: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:1195: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb' drivers/gpu/drm/i915/intel_lrc.c:1258: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:1258: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb' drivers/gpu/drm/i915/intel_lrc.c:1901: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:1901: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup' drivers/gpu/drm/i915/intel_lrc.c:2486: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:2486: warning: Excess function parameter 'ring' description in 'intel_lr_context_size' drivers/gpu/drm/i915/intel_lrc.c:2525: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/intel_lrc.c:2525: warning: Excess function parameter 'ring' description in 'intel_lr_context_deferred_alloc' Warning: didn't use docs for i915_hotplug_interrupt_update Warning: didn't use docs for ilk_update_display_irq Warning: didn't use docs for ilk_update_gt_irq Warning: didn't use docs for snb_update_pm_irq Warning: didn't use docs for bdw_update_port_irq Warning: didn't use docs for bdw_update_pipe_irq Warning: didn't use docs for ibx_display_interrupt_update Warning: didn't use docs for i915_enable_asle_pipestat Warning: didn't use docs for ivybridge_parity_work Warning: didn't use docs for i915_reset_and_wakeup Warning: didn't use docs for i915_handle_error Warning: didn't use docs for intel_irq_install Warning: didn't use docs for intel_irq_uninstall vim +/dev +739 drivers/gpu/drm/i915/intel_psr.c 723 * This bit will be self-clear when it gets to the PSR active state. 724 */ 725 I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE); 726 } 727 mutex_unlock(&dev_priv->psr.lock); 728 } 729 730 /** 731 * vlv_psr_src_timing_get - src timing generation requested 732 * 733 * CHV does not have HW tracking to trigger PSR exit when VBI are enabled nor 734 * does enabling vblank interrupts prevent PSR entry. This function is called 735 * before enabling VBI to exit PSR and prevent PSR re-entry until vblanks are 736 * disabled again. 737 */ 738 void vlv_psr_src_timing_get(struct drm_device *dev) > 739 { 740 struct drm_i915_private *dev_priv = dev->dev_private; 741 742 mutex_lock(&dev_priv->psr.lock); 743 if (!dev_priv->psr.enabled) { 744 mutex_unlock(&dev_priv->psr.lock); 745 return; 746 } 747 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote: > PSR in CHV, unlike HSW, can get activated even if vblanks interrupts > are > enabled. But, the pipe is not expected to generate timings signals > when PSR is active. Specifically, we do not get vblank interrupts in > CHV > if PSR becomes active. This has led to drm_wait_vblank timing out. > > Let's disable PSR using the vblank prepare hook that gets called > before > enabling vblank interrupts and keep it disabled until the interrupts > are > not needed. > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_psr.c | 61 > +++++++++++++++++++++++++++++++++++++++- > 4 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index e4c8e34..03f311e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -994,6 +994,7 @@ struct i915_psr { > bool psr2_support; > bool aux_frame_sync; > bool link_standby; > + bool vlv_src_timing; > }; > > enum intel_pch { > diff --git a/drivers/gpu/drm/i915/i915_irq.c > b/drivers/gpu/drm/i915/i915_irq.c > index caaf1e2..77f3d76 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct > drm_device *dev, unsigned int pipe) > return 0; > } > > +static void valleyview_prepare_vblank(struct drm_device *dev, > unsigned int pipe) > +{ shouldn't we force psr_exit here? What if vblank is enabled after psr is already in active mode? > + vlv_psr_src_timing_get(dev); > +} > + > +static void valleyview_unprepare_vblank(struct drm_device *dev, > unsigned int pipe){ > + > + vlv_psr_src_timing_put(dev); > +} > + > /* Called from drm generic code, passed 'crtc' which > * we use as a pipe index > */ > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private > *dev_priv) > dev->driver->irq_uninstall = > cherryview_irq_uninstall; > dev->driver->enable_vblank = > valleyview_enable_vblank; > dev->driver->disable_vblank = > valleyview_disable_vblank; > + dev->driver->prepare_vblank = > valleyview_prepare_vblank; > + dev->driver->unprepare_vblank = > valleyview_unprepare_vblank; > dev_priv->display.hpd_irq_setup = > i915_hpd_irq_setup; > } else if (IS_VALLEYVIEW(dev_priv)) { > dev->driver->irq_handler = valleyview_irq_handler; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 9b5f663..e2078fd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev, > void intel_psr_init(struct drm_device *dev); > void intel_psr_single_frame_update(struct drm_device *dev, > unsigned frontbuffer_bits); > +void vlv_psr_src_timing_get(struct drm_device *dev); > +void vlv_psr_src_timing_put(struct drm_device *dev); > > /* intel_runtime_pm.c */ > int intel_power_domains_init(struct drm_i915_private *); > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index 29a09bf..c95e680 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp) > > /* Enable PSR on the panel */ > vlv_psr_enable_sink(intel_dp); > + dev_priv->psr.vlv_src_timing = false; > > /* On HSW+ enable_source also means go to PSR > entry/active > * state as soon as idle_frame achieved and here > would be > @@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct > *work) > * The delayed work can race with an invalidate hence we > need to > * recheck. Since psr_flush first clears this and then > reschedules we > * won't ever miss a flush when bailing out here. > + * Also, do not enable PSR if source is required to generate > timing > + * signals like vblanks. > */ > - if (dev_priv->psr.busy_frontbuffer_bits) > + if (dev_priv->psr.busy_frontbuffer_bits || dev_priv > ->psr.vlv_src_timing) > goto unlock; I wonder if in this vlv_src_timing case the work should re-schedule itself... otherwise we have the risk of psr staying disabled forever right? > > intel_psr_activate(intel_dp); > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct > drm_device *dev, > } > > /** > + * vlv_psr_src_timing_get - src timing generation requested > + * > + * CHV does not have HW tracking to trigger PSR exit when VBI are > enabled nor > + * does enabling vblank interrupts prevent PSR entry. This function > is called > + * before enabling VBI to exit PSR and prevent PSR re-entry until > vblanks are > + * disabled again. > + */ > +void vlv_psr_src_timing_get(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&dev_priv->psr.lock); > + if (!dev_priv->psr.enabled) { > + mutex_unlock(&dev_priv->psr.lock); > + return; > + } > + > + //Handle racing with intel_psr_work with this flag Is this comment permanent? if so you should use /**/ > + dev_priv->psr.vlv_src_timing = true; > + > + if(dev_priv->psr.active) > + intel_psr_exit(dev); > + > + mutex_unlock(&dev_priv->psr.lock); > + > +} > + > + > +/** > + * vlv_psr_src_timing_put - src timing generation not required > + * > + * CHV does not have HW tracking to trigger PSR exit when VBI are > enabled nor > + * does enabling vblank interrupts prevent PSR entry. This function > is called > + * when VBI are not required and PSR can be activated. > + */ > +void vlv_psr_src_timing_put(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&dev_priv->psr.lock); > + if (!dev_priv->psr.enabled) { > + mutex_unlock(&dev_priv->psr.lock); > + return; > + } > + > + dev_priv->psr.vlv_src_timing = false; > + > + if (!dev_priv->psr.active) > + if (!work_busy(&dev_priv->psr.work.work)) > + schedule_delayed_work(&dev_priv->psr.work, > + > msecs_to_jiffies(100)); > + > + mutex_unlock(&dev_priv->psr.lock); > +} > + > +/** > * intel_psr_invalidate - Invalidade PSR > * @dev: DRM device > * @frontbuffer_bits: frontbuffer plane tracking bits
On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote: > On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote: > > PSR in CHV, unlike HSW, can get activated even if vblanks interrupts > > are > > enabled. But, the pipe is not expected to generate timings signals > > when PSR is active. Specifically, we do not get vblank interrupts in > > CHV > > if PSR becomes active. This has led to drm_wait_vblank timing out. > > > > Let's disable PSR using the vblank prepare hook that gets called > > before > > enabling vblank interrupts and keep it disabled until the interrupts > > are > > not needed. > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_psr.c | 61 > > +++++++++++++++++++++++++++++++++++++++- > > 4 files changed, 75 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index e4c8e34..03f311e 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -994,6 +994,7 @@ struct i915_psr { > > bool psr2_support; > > bool aux_frame_sync; > > bool link_standby; > > + bool vlv_src_timing; > > }; > > > > enum intel_pch { > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index caaf1e2..77f3d76 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct > > drm_device *dev, unsigned int pipe) > > return 0; > > } > > > > +static void valleyview_prepare_vblank(struct drm_device *dev, > > unsigned int pipe) > > +{ > > shouldn't we force psr_exit here? What if vblank is enabled after psr > is already in active mode? > vlv_psr_src_timing_get calls intel_psr_exit if PSR is already active. > > + vlv_psr_src_timing_get(dev); > > +} > > + > > +static void valleyview_unprepare_vblank(struct drm_device *dev, > > unsigned int pipe){ > > + > > + vlv_psr_src_timing_put(dev); > > +} > > + > > /* Called from drm generic code, passed 'crtc' which > > * we use as a pipe index > > */ > > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private > > *dev_priv) > > dev->driver->irq_uninstall = > > cherryview_irq_uninstall; > > dev->driver->enable_vblank = > > valleyview_enable_vblank; > > dev->driver->disable_vblank = > > valleyview_disable_vblank; > > + dev->driver->prepare_vblank = > > valleyview_prepare_vblank; > > + dev->driver->unprepare_vblank = > > valleyview_unprepare_vblank; > > dev_priv->display.hpd_irq_setup = > > i915_hpd_irq_setup; > > } else if (IS_VALLEYVIEW(dev_priv)) { > > dev->driver->irq_handler = valleyview_irq_handler; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 9b5f663..e2078fd 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev, > > void intel_psr_init(struct drm_device *dev); > > void intel_psr_single_frame_update(struct drm_device *dev, > > unsigned frontbuffer_bits); > > +void vlv_psr_src_timing_get(struct drm_device *dev); > > +void vlv_psr_src_timing_put(struct drm_device *dev); > > > > /* intel_runtime_pm.c */ > > int intel_power_domains_init(struct drm_i915_private *); > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 29a09bf..c95e680 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp) > > > > /* Enable PSR on the panel */ > > vlv_psr_enable_sink(intel_dp); > > + dev_priv->psr.vlv_src_timing = false; > > > > /* On HSW+ enable_source also means go to PSR > > entry/active > > * state as soon as idle_frame achieved and here > > would be > > @@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct > > *work) > > * The delayed work can race with an invalidate hence we > > need to > > * recheck. Since psr_flush first clears this and then > > reschedules we > > * won't ever miss a flush when bailing out here. > > + * Also, do not enable PSR if source is required to generate > > timing > > + * signals like vblanks. > > */ > > - if (dev_priv->psr.busy_frontbuffer_bits) > > + if (dev_priv->psr.busy_frontbuffer_bits || dev_priv > > ->psr.vlv_src_timing) > > goto unlock; > > I wonder if in this vlv_src_timing case the work should re-schedule > itself... otherwise we have the risk of psr staying disabled forever > right? > > intel_psr_work gets rescheduled when vblanks are disabled in vlv_psr_src_timing_get > > > > intel_psr_activate(intel_dp); > > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct > > drm_device *dev, > > } > > > > /** > > + * vlv_psr_src_timing_get - src timing generation requested > > + * > > + * CHV does not have HW tracking to trigger PSR exit when VBI are > > enabled nor > > + * does enabling vblank interrupts prevent PSR entry. This function > > is called > > + * before enabling VBI to exit PSR and prevent PSR re-entry until > > vblanks are > > + * disabled again. > > + */ > > +void vlv_psr_src_timing_get(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + mutex_lock(&dev_priv->psr.lock); > > + if (!dev_priv->psr.enabled) { > > + mutex_unlock(&dev_priv->psr.lock); > > + return; > > + } > > + > > + //Handle racing with intel_psr_work with this flag > > Is this comment permanent? if so you should use /**/ > I will fix this and send a new version. > > + dev_priv->psr.vlv_src_timing = true; > > + > > + if(dev_priv->psr.active) > > + intel_psr_exit(dev); > > + > > + mutex_unlock(&dev_priv->psr.lock); > > + > > +} > > + > > + > > +/** > > + * vlv_psr_src_timing_put - src timing generation not required > > + * > > + * CHV does not have HW tracking to trigger PSR exit when VBI are > > enabled nor > > + * does enabling vblank interrupts prevent PSR entry. This function > > is called > > + * when VBI are not required and PSR can be activated. > > + */ > > +void vlv_psr_src_timing_put(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + mutex_lock(&dev_priv->psr.lock); > > + if (!dev_priv->psr.enabled) { > > + mutex_unlock(&dev_priv->psr.lock); > > + return; > > + } > > + > > + dev_priv->psr.vlv_src_timing = false; > > + > > + if (!dev_priv->psr.active) > > + if (!work_busy(&dev_priv->psr.work.work)) > > + schedule_delayed_work(&dev_priv->psr.work, > > + > > msecs_to_jiffies(100)); > > + > > + mutex_unlock(&dev_priv->psr.lock); > > +} > > + > > +/** > > * intel_psr_invalidate - Invalidade PSR > > * @dev: DRM device > > * @frontbuffer_bits: frontbuffer plane tracking bits
On Wed, 2016-06-15 at 01:02 +0000, Pandiyan, Dhinakaran wrote: > On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote: > > On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote: > > > PSR in CHV, unlike HSW, can get activated even if vblanks > > > interrupts > > > are > > > enabled. But, the pipe is not expected to generate timings > > > signals > > > when PSR is active. Specifically, we do not get vblank interrupts > > > in > > > CHV > > > if PSR becomes active. This has led to drm_wait_vblank timing > > > out. > > > > > > Let's disable PSR using the vblank prepare hook that gets called > > > before > > > enabling vblank interrupts and keep it disabled until the > > > interrupts > > > are > > > not needed. > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > drivers/gpu/drm/i915/intel_psr.c | 61 > > > +++++++++++++++++++++++++++++++++++++++- > > > 4 files changed, 75 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index e4c8e34..03f311e 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -994,6 +994,7 @@ struct i915_psr { > > > bool psr2_support; > > > bool aux_frame_sync; > > > bool link_standby; > > > + bool vlv_src_timing; > > > }; > > > > > > enum intel_pch { > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > b/drivers/gpu/drm/i915/i915_irq.c > > > index caaf1e2..77f3d76 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct > > > drm_device *dev, unsigned int pipe) > > > return 0; > > > } > > > > > > +static void valleyview_prepare_vblank(struct drm_device *dev, > > > unsigned int pipe) > > > +{ > > > > shouldn't we force psr_exit here? What if vblank is enabled after > > psr > > is already in active mode? > > > > vlv_psr_src_timing_get calls intel_psr_exit if PSR is already active. oh it is already there indeed... i'm crazy, sorry... I liked the solution... really simple and straightforward... so simple that confused me hehe > > > > + vlv_psr_src_timing_get(dev); > > > +} > > > + > > > +static void valleyview_unprepare_vblank(struct drm_device *dev, > > > unsigned int pipe){ > > > + > > > + vlv_psr_src_timing_put(dev); > > > +} > > > + > > > /* Called from drm generic code, passed 'crtc' which > > > * we use as a pipe index > > > */ > > > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private > > > *dev_priv) > > > dev->driver->irq_uninstall = > > > cherryview_irq_uninstall; > > > dev->driver->enable_vblank = > > > valleyview_enable_vblank; > > > dev->driver->disable_vblank = > > > valleyview_disable_vblank; > > > + dev->driver->prepare_vblank = > > > valleyview_prepare_vblank; > > > + dev->driver->unprepare_vblank = > > > valleyview_unprepare_vblank; > > > dev_priv->display.hpd_irq_setup = > > > i915_hpd_irq_setup; > > > } else if (IS_VALLEYVIEW(dev_priv)) { > > > dev->driver->irq_handler = > > > valleyview_irq_handler; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 9b5f663..e2078fd 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device > > > *dev, > > > void intel_psr_init(struct drm_device *dev); > > > void intel_psr_single_frame_update(struct drm_device *dev, > > > unsigned frontbuffer_bits); > > > +void vlv_psr_src_timing_get(struct drm_device *dev); > > > +void vlv_psr_src_timing_put(struct drm_device *dev); > > > > > > /* intel_runtime_pm.c */ > > > int intel_power_domains_init(struct drm_i915_private *); > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 29a09bf..c95e680 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp > > > *intel_dp) > > > > > > /* Enable PSR on the panel */ > > > vlv_psr_enable_sink(intel_dp); > > > + dev_priv->psr.vlv_src_timing = false; > > > > > > /* On HSW+ enable_source also means go to PSR > > > entry/active > > > * state as soon as idle_frame achieved and here > > > would be > > > @@ -608,8 +609,10 @@ static void intel_psr_work(struct > > > work_struct > > > *work) > > > * The delayed work can race with an invalidate hence we > > > need to > > > * recheck. Since psr_flush first clears this and then > > > reschedules we > > > * won't ever miss a flush when bailing out here. > > > + * Also, do not enable PSR if source is required to > > > generate > > > timing > > > + * signals like vblanks. > > > */ > > > - if (dev_priv->psr.busy_frontbuffer_bits) > > > + if (dev_priv->psr.busy_frontbuffer_bits || dev_priv > > > ->psr.vlv_src_timing) > > > goto unlock; > > > > I wonder if in this vlv_src_timing case the work should re-schedule > > itself... otherwise we have the risk of psr staying disabled > > forever > > right? > > > > > intel_psr_work gets rescheduled when vblanks are disabled in > vlv_psr_src_timing_get > > > > > > > intel_psr_activate(intel_dp); > > > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct > > > drm_device *dev, > > > } > > > > > > /** > > > + * vlv_psr_src_timing_get - src timing generation requested > > > + * > > > + * CHV does not have HW tracking to trigger PSR exit when VBI > > > are > > > enabled nor > > > + * does enabling vblank interrupts prevent PSR entry. This > > > function > > > is called > > > + * before enabling VBI to exit PSR and prevent PSR re-entry > > > until > > > vblanks are > > > + * disabled again. > > > + */ > > > +void vlv_psr_src_timing_get(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + > > > + mutex_lock(&dev_priv->psr.lock); > > > + if (!dev_priv->psr.enabled) { > > > + mutex_unlock(&dev_priv->psr.lock); > > > > > > + return; > > > + } > > > + > > > + //Handle racing with intel_psr_work with this flag > > > > Is this comment permanent? if so you should use /**/ > > > I will fix this and send a new version. with this fix feel free to use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Also I believe with this series merge we would be good to re-enable psr on VLV/CHV... more random thoughts below... > > > + dev_priv->psr.vlv_src_timing = true; > > > + > > > + if(dev_priv->psr.active) > > > + intel_psr_exit(dev); > > > + > > > + mutex_unlock(&dev_priv->psr.lock); > > > + > > > +} > > > + > > > + > > > +/** > > > + * vlv_psr_src_timing_put - src timing generation not required > > > + * > > > + * CHV does not have HW tracking to trigger PSR exit when VBI > > > are > > > enabled nor > > > + * does enabling vblank interrupts prevent PSR entry. This > > > function > > > is called > > > + * when VBI are not required and PSR can be activated. > > > + */ > > > +void vlv_psr_src_timing_put(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + > > > + mutex_lock(&dev_priv->psr.lock); > > > + if (!dev_priv->psr.enabled) { > > > + mutex_unlock(&dev_priv->psr.lock); > > > + return; > > > + } > > > + > > > + dev_priv->psr.vlv_src_timing = false; > > > + > > > + if (!dev_priv->psr.active) > > > + if (!work_busy(&dev_priv->psr.work.work)) > > > + schedule_delayed_work(&dev_priv > > > ->psr.work, here I wondered it would be better to call the vlv_psr_activate directly, but the work function handle some restrictions so your solution is better... > + > > > msecs_to_jiffies(100)); just asking myself yet again if we should base this timeout in vblank time... one way or another maybe it is good to save it in psr struct to keep it consistent in case we need to change someday... but not a requirement at all, just a random thought... Thanks, Rodrigo. > > > + > > > + mutex_unlock(&dev_priv->psr.lock); > > > +} > > > + > > > +/** > > > * intel_psr_invalidate - Invalidade PSR > > > * @dev: DRM device > > > * @frontbuffer_bits: frontbuffer plane tracking bits >
On Wed, 2016-06-15 at 21:44 +0000, Vivi, Rodrigo wrote: > On Wed, 2016-06-15 at 01:02 +0000, Pandiyan, Dhinakaran wrote: > > On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote: > > > On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote: > > > > PSR in CHV, unlike HSW, can get activated even if vblanks > > > > interrupts > > > > are > > > > enabled. But, the pipe is not expected to generate timings > > > > signals > > > > when PSR is active. Specifically, we do not get vblank interrupts > > > > in > > > > CHV > > > > if PSR becomes active. This has led to drm_wait_vblank timing > > > > out. > > > > > > > > Let's disable PSR using the vblank prepare hook that gets called > > > > before > > > > enabling vblank interrupts and keep it disabled until the > > > > interrupts > > > > are > > > > not needed. > > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++ > > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > > drivers/gpu/drm/i915/intel_psr.c | 61 > > > > +++++++++++++++++++++++++++++++++++++++- > > > > 4 files changed, 75 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index e4c8e34..03f311e 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -994,6 +994,7 @@ struct i915_psr { > > > > bool psr2_support; > > > > bool aux_frame_sync; > > > > bool link_standby; > > > > + bool vlv_src_timing; > > > > }; > > > > > > > > enum intel_pch { > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > > b/drivers/gpu/drm/i915/i915_irq.c > > > > index caaf1e2..77f3d76 100644 > > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct > > > > drm_device *dev, unsigned int pipe) > > > > return 0; > > > > } > > > > > > > > +static void valleyview_prepare_vblank(struct drm_device *dev, > > > > unsigned int pipe) > > > > +{ > > > > > > shouldn't we force psr_exit here? What if vblank is enabled after > > > psr > > > is already in active mode? > > > > > > > vlv_psr_src_timing_get calls intel_psr_exit if PSR is already active. > > oh it is already there indeed... > i'm crazy, sorry... > > I liked the solution... really simple and straightforward... so simple > that confused me hehe > > > > > > > + vlv_psr_src_timing_get(dev); > > > > +} > > > > + > > > > +static void valleyview_unprepare_vblank(struct drm_device *dev, > > > > unsigned int pipe){ > > > > + > > > > + vlv_psr_src_timing_put(dev); > > > > +} > > > > + > > > > /* Called from drm generic code, passed 'crtc' which > > > > * we use as a pipe index > > > > */ > > > > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private > > > > *dev_priv) > > > > dev->driver->irq_uninstall = > > > > cherryview_irq_uninstall; > > > > dev->driver->enable_vblank = > > > > valleyview_enable_vblank; > > > > dev->driver->disable_vblank = > > > > valleyview_disable_vblank; > > > > + dev->driver->prepare_vblank = > > > > valleyview_prepare_vblank; > > > > + dev->driver->unprepare_vblank = > > > > valleyview_unprepare_vblank; > > > > dev_priv->display.hpd_irq_setup = > > > > i915_hpd_irq_setup; > > > > } else if (IS_VALLEYVIEW(dev_priv)) { > > > > dev->driver->irq_handler = > > > > valleyview_irq_handler; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 9b5f663..e2078fd 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device > > > > *dev, > > > > void intel_psr_init(struct drm_device *dev); > > > > void intel_psr_single_frame_update(struct drm_device *dev, > > > > unsigned frontbuffer_bits); > > > > +void vlv_psr_src_timing_get(struct drm_device *dev); > > > > +void vlv_psr_src_timing_put(struct drm_device *dev); > > > > > > > > /* intel_runtime_pm.c */ > > > > int intel_power_domains_init(struct drm_i915_private *); > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index 29a09bf..c95e680 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp > > > > *intel_dp) > > > > > > > > /* Enable PSR on the panel */ > > > > vlv_psr_enable_sink(intel_dp); > > > > + dev_priv->psr.vlv_src_timing = false; > > > > > > > > /* On HSW+ enable_source also means go to PSR > > > > entry/active > > > > * state as soon as idle_frame achieved and here > > > > would be > > > > @@ -608,8 +609,10 @@ static void intel_psr_work(struct > > > > work_struct > > > > *work) > > > > * The delayed work can race with an invalidate hence we > > > > need to > > > > * recheck. Since psr_flush first clears this and then > > > > reschedules we > > > > * won't ever miss a flush when bailing out here. > > > > + * Also, do not enable PSR if source is required to > > > > generate > > > > timing > > > > + * signals like vblanks. > > > > */ > > > > - if (dev_priv->psr.busy_frontbuffer_bits) > > > > + if (dev_priv->psr.busy_frontbuffer_bits || dev_priv > > > > ->psr.vlv_src_timing) > > > > goto unlock; > > > > > > I wonder if in this vlv_src_timing case the work should re-schedule > > > itself... otherwise we have the risk of psr staying disabled > > > forever > > > right? > > > > > > > > intel_psr_work gets rescheduled when vblanks are disabled in > > vlv_psr_src_timing_get > > > > > > > > > > intel_psr_activate(intel_dp); > > > > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct > > > > drm_device *dev, > > > > } > > > > > > > > /** > > > > + * vlv_psr_src_timing_get - src timing generation requested > > > > + * > > > > + * CHV does not have HW tracking to trigger PSR exit when VBI > > > > are > > > > enabled nor > > > > + * does enabling vblank interrupts prevent PSR entry. This > > > > function > > > > is called > > > > + * before enabling VBI to exit PSR and prevent PSR re-entry > > > > until > > > > vblanks are > > > > + * disabled again. > > > > + */ > > > > +void vlv_psr_src_timing_get(struct drm_device *dev) > > > > +{ > > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > > + > > > > + mutex_lock(&dev_priv->psr.lock); > > > > + if (!dev_priv->psr.enabled) { > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > > > > > + return; > > > > + } > > > > + > > > > + //Handle racing with intel_psr_work with this flag > > > > > > Is this comment permanent? if so you should use /**/ > > > > > I will fix this and send a new version. > > with this fix feel free to use > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Also I believe with this series merge we would be good to re-enable psr > on VLV/CHV... > > more random thoughts below... > > > > > + dev_priv->psr.vlv_src_timing = true; > > > > + > > > > + if(dev_priv->psr.active) > > > > + intel_psr_exit(dev); > > > > + > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > + > > > > +} > > > > + > > > > + > > > > +/** > > > > + * vlv_psr_src_timing_put - src timing generation not required > > > > + * > > > > + * CHV does not have HW tracking to trigger PSR exit when VBI > > > > are > > > > enabled nor > > > > + * does enabling vblank interrupts prevent PSR entry. This > > > > function > > > > is called > > > > + * when VBI are not required and PSR can be activated. > > > > + */ > > > > +void vlv_psr_src_timing_put(struct drm_device *dev) > > > > +{ > > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > > + > > > > + mutex_lock(&dev_priv->psr.lock); > > > > + if (!dev_priv->psr.enabled) { > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > + return; > > > > + } > > > > + > > > > + dev_priv->psr.vlv_src_timing = false; > > > > + > > > > + if (!dev_priv->psr.active) > > > > + if (!work_busy(&dev_priv->psr.work.work)) > > > > + schedule_delayed_work(&dev_priv > > > > ->psr.work, > > here I wondered it would be better to call the vlv_psr_activate > directly, but the work function handle some restrictions so your > solution is better... > > > + > > > > msecs_to_jiffies(100)); > > just asking myself yet again if we should base this timeout in vblank > time... > > one way or another maybe it is good to save it in psr struct to keep it > consistent in case we need to change someday... but not a requirement > at all, just a random thought... > > Thanks, > Rodrigo. > > > > > + > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > +} > > > > + > > > > +/** > > > > * intel_psr_invalidate - Invalidade PSR > > > > * @dev: DRM device > > > > * @frontbuffer_bits: frontbuffer plane tracking bits > > Thanks for the review Rodrigo. Unfortunately, I found some IGT failures because of race conditions, I will fix that and send another version.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e4c8e34..03f311e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -994,6 +994,7 @@ struct i915_psr { bool psr2_support; bool aux_frame_sync; bool link_standby; + bool vlv_src_timing; }; enum intel_pch { diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index caaf1e2..77f3d76 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) return 0; } +static void valleyview_prepare_vblank(struct drm_device *dev, unsigned int pipe) +{ + vlv_psr_src_timing_get(dev); +} + +static void valleyview_unprepare_vblank(struct drm_device *dev, unsigned int pipe){ + + vlv_psr_src_timing_put(dev); +} + /* Called from drm generic code, passed 'crtc' which * we use as a pipe index */ @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv) dev->driver->irq_uninstall = cherryview_irq_uninstall; dev->driver->enable_vblank = valleyview_enable_vblank; dev->driver->disable_vblank = valleyview_disable_vblank; + dev->driver->prepare_vblank = valleyview_prepare_vblank; + dev->driver->unprepare_vblank = valleyview_unprepare_vblank; dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; } else if (IS_VALLEYVIEW(dev_priv)) { dev->driver->irq_handler = valleyview_irq_handler; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9b5f663..e2078fd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev, void intel_psr_init(struct drm_device *dev); void intel_psr_single_frame_update(struct drm_device *dev, unsigned frontbuffer_bits); +void vlv_psr_src_timing_get(struct drm_device *dev); +void vlv_psr_src_timing_put(struct drm_device *dev); /* intel_runtime_pm.c */ int intel_power_domains_init(struct drm_i915_private *); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 29a09bf..c95e680 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp) /* Enable PSR on the panel */ vlv_psr_enable_sink(intel_dp); + dev_priv->psr.vlv_src_timing = false; /* On HSW+ enable_source also means go to PSR entry/active * state as soon as idle_frame achieved and here would be @@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct *work) * The delayed work can race with an invalidate hence we need to * recheck. Since psr_flush first clears this and then reschedules we * won't ever miss a flush when bailing out here. + * Also, do not enable PSR if source is required to generate timing + * signals like vblanks. */ - if (dev_priv->psr.busy_frontbuffer_bits) + if (dev_priv->psr.busy_frontbuffer_bits || dev_priv->psr.vlv_src_timing) goto unlock; intel_psr_activate(intel_dp); @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct drm_device *dev, } /** + * vlv_psr_src_timing_get - src timing generation requested + * + * CHV does not have HW tracking to trigger PSR exit when VBI are enabled nor + * does enabling vblank interrupts prevent PSR entry. This function is called + * before enabling VBI to exit PSR and prevent PSR re-entry until vblanks are + * disabled again. + */ +void vlv_psr_src_timing_get(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + mutex_lock(&dev_priv->psr.lock); + if (!dev_priv->psr.enabled) { + mutex_unlock(&dev_priv->psr.lock); + return; + } + + //Handle racing with intel_psr_work with this flag + dev_priv->psr.vlv_src_timing = true; + + if(dev_priv->psr.active) + intel_psr_exit(dev); + + mutex_unlock(&dev_priv->psr.lock); + +} + + +/** + * vlv_psr_src_timing_put - src timing generation not required + * + * CHV does not have HW tracking to trigger PSR exit when VBI are enabled nor + * does enabling vblank interrupts prevent PSR entry. This function is called + * when VBI are not required and PSR can be activated. + */ +void vlv_psr_src_timing_put(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + mutex_lock(&dev_priv->psr.lock); + if (!dev_priv->psr.enabled) { + mutex_unlock(&dev_priv->psr.lock); + return; + } + + dev_priv->psr.vlv_src_timing = false; + + if (!dev_priv->psr.active) + if (!work_busy(&dev_priv->psr.work.work)) + schedule_delayed_work(&dev_priv->psr.work, + msecs_to_jiffies(100)); + + mutex_unlock(&dev_priv->psr.lock); +} + +/** * intel_psr_invalidate - Invalidade PSR * @dev: DRM device * @frontbuffer_bits: frontbuffer plane tracking bits
PSR in CHV, unlike HSW, can get activated even if vblanks interrupts are enabled. But, the pipe is not expected to generate timings signals when PSR is active. Specifically, we do not get vblank interrupts in CHV if PSR becomes active. This has led to drm_wait_vblank timing out. Let's disable PSR using the vblank prepare hook that gets called before enabling vblank interrupts and keep it disabled until the interrupts are not needed. Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_psr.c | 61 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 75 insertions(+), 1 deletion(-)