Re: [PATCH] drm/i915: Idleness detection for DRRS

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Aug-07-2014 7:27 PM, Daniel Vetter wrote:
> Hi Vandana,
> 
> I think we need to start over from a fresh&clean slate with this part
> - too much changed and all the differen revisions of this patch don't
> provide that much information.
> 
> Also as Chris says this introduces yet another idleness detection
> system, and we already have plenty of them.
> 
> High-level comments:
> 
> - We now have the frontbuffer tracking infrastructure, see the various
> new intel_frontbuffer_* and similar functions in intel_display.c. They
> have nice kerneldoc explaining what exactly they do. We need to hook
> into that instead of creating new idlesness detection.
> 
Hi Daniel,

Could you point me to this documentation? I cant find it - might be
missing something..

Apart from this idleness detection patch, there are 2 other patches
which just set the appropriate registers for bdw and vlv (in
set_drrs()). These 2 patches have got R-b from Jani and need rebasing. I
was wondering if I could resend those patches - they won't have an
effect until DRRS trigger is in place..

- Vandana
> - There's already calls to intel_increase/decrease_pllclock. That
> should be renamed to gmch_increase/decrease_pllclock, and we the new
> dp drrs support here should be called at _exactly_ the same spots.
> 
> - This doesn't use the pipe config we've added at all - DRRS shouldn't
> look at the panel, but only at the pipe config to figure out whether
> drrs is supported on a given pipe or not. There is also no need to
> restrict DRRS to single pipe mode at all.
> 
> - Locking is important. In case of doubt follow the example
> established by the new PSR code. Especially important is to not
> require modeset locks.
> 
> - There's no need to disable DRRS from the PSR (or fbc) code at all,
> the frontbuffer tracking code will take care of all this. And just
> using pageflips as business signal isn't good enough.
> 
> This is just a rough guideline, I think it'd be good to first hash out
> the rough design in more detail (maybe as a patch with just
> pseudo-code) before starting with codeing.
> 
> Thanks, Daniel
> 
> 
> 
> On Thu, Aug 7, 2014 at 3:45 PM, Vandana Kannan <vandana.kannan@xxxxxxxxx> wrote:
>> Adding support to detect display idleness by tracking page flip from
>> user space. Switch to low refresh rate is triggered after 2 seconds of
>> idleness. The delay is configurable. If there is a page flip or call to
>> update the plane, then high refresh rate is applied.
>> The feature is not used in dual-display mode.
>>
>> v2: Chris Wilson's review comments incorporated.
>> Modify idleness detection implementation to make it similar to the
>> implementation of intel_update_fbc/intel_disable_fbc
>>
>> v3: Internal review comments incorporated
>> Add NULL pointer check in intel_disable_drrs.
>> Add drrs calls in i9xx_crtc_enable/disable and valleyview_crtc_enable.
>>
>> v4: Jani's review comments incorporated.
>> Change in sequence in intel_update_drrs. Comment modified to remove details
>> of update param. Modified DRRS idleness interval to a module parameter.
>>
>> v5: Chris's review comments incorporated.
>> Initialize connector in idleness detection init. Modifications made to
>> use only intel_connector in i915_drrs and derive intel_dp when required.
>> Added a function drrs_fini to cleanup DRRS work.
>>
>> v6: Internal review comments. Removed check for primary enabled, which is
>> a redundant check, in the case of clone mode. Added a flag to track
>> dual-display configuration. Remove print statement for "cancel DRR work"
>> and print "DRRS not supported" only once.
>>
>> v7: As per internal review comments, removing calls to update/disable drrs
>> from sprite update path. For sprite, all drrs related updates would be
>> taken care of with calls to crtc page flip itself. This will have to be
>> revisited later if flip infrastructure changes for sprite.
>>
>> v8: Incorporated Jani's review comments. Added space after the periods in the
>> module param description. Changes around drrs-fini to remove seamless DRRS
>> check.
>>
>> v9: Added checks for PSR before updating DRRS. Added check for module
>> param drrs_interval before updating DRRS (this is required if the interval
>> is modified by the user during system use). DRRS disabled by default. Changes
>> based on Jani's review comments
>>
>> v10: Disable/enable DRRS when PSR is enable/disabled.
>>
>> v11: Moved DRRS not supported log to patch2.
>>
>> Patch rebased.
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
>> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> Cc: Daniel Vetter <daniel@xxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |   7 ++
>>  drivers/gpu/drm/i915/i915_params.c   |   8 ++
>>  drivers/gpu/drm/i915/intel_display.c |  13 ++++
>>  drivers/gpu/drm/i915/intel_dp.c      |  27 ++++++-
>>  drivers/gpu/drm/i915/intel_drv.h     |   5 +-
>>  drivers/gpu/drm/i915/intel_pm.c      | 142 +++++++++++++++++++++++++++++++++++
>>  6 files changed, 200 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 125a83c..993fdb1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -667,6 +667,12 @@ struct i915_fbc {
>>
>>  struct i915_drrs {
>>         struct intel_connector *connector;
>> +       bool is_clone;
>> +       struct intel_drrs_work {
>> +               struct delayed_work work;
>> +               struct drm_crtc *crtc;
>> +               int interval;
>> +       } *drrs_work;
>>  };
>>
>>  struct intel_dp;
>> @@ -2156,6 +2162,7 @@ struct i915_params {
>>         int enable_ips;
>>         int invert_brightness;
>>         int enable_cmd_parser;
>> +       int drrs_interval;
>>         /* leave bools at the end to not create holes */
>>         bool enable_hangcheck;
>>         bool fastboot;
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 62ee830..912a02f 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -50,6 +50,7 @@ struct i915_params i915 __read_mostly = {
>>         .disable_vtd_wa = 0,
>>         .use_mmio_flip = 0,
>>         .mmio_debug = 0,
>> +       .drrs_interval = 0,
>>  };
>>
>>  module_param_named(modeset, i915.modeset, int, 0400);
>> @@ -167,3 +168,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
>>  MODULE_PARM_DESC(mmio_debug,
>>         "Enable the MMIO debug code (default: false). This may negatively "
>>         "affect performance.");
>> +
>> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
>> +MODULE_PARM_DESC(drrs_interval,
>> +       "DRRS idleness detection interval  (default: 0 ms). "
>> +       "If this field is set to 0, then seamless DRRS feature "
>> +       "based on idleness detection is disabled. "
>> +       "The interval is to be set in milliseconds.");
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 89e0ac5..da24f4e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2743,6 +2743,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>>
>>         mutex_lock(&dev->struct_mutex);
>>         intel_update_fbc(dev);
>> +       intel_update_drrs(dev);
>>         mutex_unlock(&dev->struct_mutex);
>>
>>         return 0;
>> @@ -3897,6 +3898,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>>
>>         mutex_lock(&dev->struct_mutex);
>>         intel_update_fbc(dev);
>> +       intel_update_drrs(dev);
>>         mutex_unlock(&dev->struct_mutex);
>>
>>         /*
>> @@ -4213,6 +4215,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>>
>>         mutex_lock(&dev->struct_mutex);
>>         intel_update_fbc(dev);
>> +       intel_update_drrs(dev);
>>         mutex_unlock(&dev->struct_mutex);
>>  }
>>
>> @@ -4260,6 +4263,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>>
>>         mutex_lock(&dev->struct_mutex);
>>         intel_update_fbc(dev);
>> +       intel_update_drrs(dev);
>>         mutex_unlock(&dev->struct_mutex);
>>
>>         if (intel_crtc_to_shared_dpll(intel_crtc))
>> @@ -4894,6 +4898,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>>
>>         mutex_lock(&dev->struct_mutex);
>>         intel_update_fbc(dev);
>> +       intel_update_drrs(dev);
>>         mutex_unlock(&dev->struct_mutex);
>>  }
>>
>> @@ -9200,6 +9205,10 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>>         drm_gem_object_unreference(&work->old_fb_obj->base);
>>
>>         intel_update_fbc(dev);
>> +       /* disable current DRRS work scheduled and restart
>> +        * to push work by another x seconds
>> +        */
>> +       intel_update_drrs(dev);
>>         mutex_unlock(&dev->struct_mutex);
>>
>>         intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
>> @@ -9851,6 +9860,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>                           INTEL_FRONTBUFFER_PRIMARY(pipe));
>>
>>         intel_disable_fbc(dev);
>> +       intel_disable_drrs(dev);
>>         intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
>>         mutex_unlock(&dev->struct_mutex);
>>
>> @@ -12728,6 +12738,7 @@ void intel_modeset_init(struct drm_device *dev)
>>
>>         /* Just in case the BIOS is doing something questionable. */
>>         intel_disable_fbc(dev);
>> +       intel_disable_drrs(dev);
>>
>>         drm_modeset_lock_all(dev);
>>         intel_modeset_setup_hw_state(dev, false);
>> @@ -13226,6 +13237,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>>
>>         intel_disable_fbc(dev);
>>
>> +       intel_disable_drrs(dev);
>> +
>>         intel_disable_gt_powersave(dev);
>>
>>         ironlake_teardown_rc6(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 34e3c47..6b8de96 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1872,6 +1872,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>>
>>         dev_priv->psr.busy_frontbuffer_bits = 0;
>>
>> +       if (INTEL_INFO(dev)->gen < 8) {
>> +               DRM_DEBUG_KMS("Enabling PSR, disabling DRRS\n");
>> +               intel_disable_drrs(dev);
>> +       }
>> +
>>         /* Setup PSR once */
>>         intel_edp_psr_setup(intel_dp);
>>
>> @@ -1909,6 +1914,9 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>>         mutex_unlock(&dev_priv->psr.lock);
>>
>>         cancel_delayed_work_sync(&dev_priv->psr.work);
>> +
>> +       if (INTEL_INFO(dev)->gen < 8)
>> +               intel_update_drrs(dev);
>>  }
>>
>>  static void intel_edp_psr_work(struct work_struct *work)
>> @@ -3981,17 +3989,34 @@ intel_dp_connector_destroy(struct drm_connector *connector)
>>         kfree(connector);
>>  }
>>
>> +static void
>> +intel_dp_drrs_fini(struct drm_i915_private *dev_priv)
>> +{
>> +       if (cancel_delayed_work_sync(&dev_priv->drrs.drrs_work->work)) {
>> +               kfree(dev_priv->drrs.drrs_work);
>> +               dev_priv->drrs.drrs_work = NULL;
>> +               dev_priv->drrs.connector = NULL;
>> +       }
>> +}
>> +
>>  void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>>  {
>>         struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
>>         struct intel_dp *intel_dp = &intel_dig_port->dp;
>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>>         drm_dp_aux_unregister(&intel_dp->aux);
>>         intel_dp_mst_encoder_cleanup(intel_dig_port);
>>         drm_encoder_cleanup(encoder);
>>         if (is_edp(intel_dp)) {
>>                 cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
>> +
>> +               if (dev_priv->drrs.connector && dev_priv->drrs.drrs_work &&
>> +                       intel_dp == enc_to_intel_dp(
>> +                       &dev_priv->drrs.connector->encoder->base))
>> +                       intel_dp_drrs_fini(dev_priv);
>> +
>>                 drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>                 edp_panel_vdd_off_sync(intel_dp);
>>                 drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> @@ -4445,7 +4470,7 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>                 return NULL;
>>         }
>>
>> -       dev_priv->drrs.connector = intel_connector;
>> +       intel_init_drrs_idleness_detection(dev, intel_connector);
>>
>>         mutex_init(&intel_dp->drrs_state.mutex);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 666ca8a..f34a6d0 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1077,7 +1077,10 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>>  void ilk_wm_get_hw_state(struct drm_device *dev);
>> -
>> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
>> +               struct intel_connector *connector);
>> +void intel_update_drrs(struct drm_device *dev);
>> +void intel_disable_drrs(struct drm_device *dev);
>>
>>  /* intel_sdvo.c */
>>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 41de760..6fbbde5 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -644,6 +644,148 @@ out_disable:
>>         i915_gem_stolen_cleanup_compression(dev);
>>  }
>>
>> +static void intel_drrs_work_fn(struct work_struct *__work)
>> +{
>> +       struct intel_drrs_work *work =
>> +               container_of(to_delayed_work(__work),
>> +                            struct intel_drrs_work, work);
>> +       struct drm_device *dev = work->crtc->dev;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       /* Double check if the dual-display mode is active. */
>> +       if (dev_priv->drrs.is_clone)
>> +               return;
>> +
>> +       intel_dp_set_drrs_state(work->crtc->dev,
>> +               dev_priv->drrs.connector->panel.downclock_mode->vrefresh);
>> +}
>> +
>> +static void intel_cancel_drrs_work(struct drm_i915_private *dev_priv)
>> +{
>> +       if (dev_priv->drrs.drrs_work == NULL)
>> +               return;
>> +
>> +       cancel_delayed_work_sync(&dev_priv->drrs.drrs_work->work);
>> +}
>> +
>> +static void intel_enable_drrs(struct drm_crtc *crtc)
>> +{
>> +       struct drm_device *dev = crtc->dev;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct intel_dp *intel_dp = NULL;
>> +
>> +       intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
>> +
>> +       if (intel_dp == NULL)
>> +               return;
>> +
>> +       intel_cancel_drrs_work(dev_priv);
>> +
>> +       if (intel_dp->drrs_state.refresh_rate_type != DRRS_LOW_RR) {
>> +               dev_priv->drrs.drrs_work->crtc = crtc;
>> +               dev_priv->drrs.drrs_work->interval = i915.drrs_interval;
>> +
>> +               /* Delay the actual enabling to let pageflipping cease and the
>> +                * display to settle before starting DRRS
>> +                */
>> +               schedule_delayed_work(&dev_priv->drrs.drrs_work->work,
>> +                       msecs_to_jiffies(dev_priv->drrs.drrs_work->interval));
>> +       }
>> +}
>> +
>> +void intel_disable_drrs(struct drm_device *dev)
>> +{
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct intel_dp *intel_dp = NULL;
>> +
>> +       if (dev_priv->drrs.connector == NULL)
>> +               return;
>> +
>> +       intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
>> +
>> +       if (intel_dp == NULL)
>> +               return;
>> +
>> +       /* as part of disable DRRS, reset refresh rate to HIGH_RR */
>> +       if (intel_dp->drrs_state.refresh_rate_type == DRRS_LOW_RR) {
>> +               intel_cancel_drrs_work(dev_priv);
>> +               intel_dp_set_drrs_state(dev,
>> +                       dev_priv->drrs.connector->panel.fixed_mode->vrefresh);
>> +       }
>> +}
>> +
>> +/**
>> + * intel_update_drrs - enable/disable DRRS as needed
>> + * @dev: the drm_device
>> +*/
>> +void intel_update_drrs(struct drm_device *dev)
>> +{
>> +       struct drm_crtc *crtc = NULL, *tmp_crtc;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       if (i915.drrs_interval == 0) {
>> +               intel_disable_drrs(dev);
>> +               return;
>> +       }
>> +
>> +       /* if drrs.connector is NULL, then drrs_init did not get called.
>> +        * which means DRRS is not supported.
>> +        */
>> +       if (dev_priv->drrs.connector == NULL)
>> +               return;
>> +
>> +       if (dev_priv->drrs.connector->panel.downclock_mode == NULL)
>> +               return;
>> +
>> +       list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
>> +               if (intel_crtc_active(tmp_crtc)) {
>> +                       if (crtc) {
>> +                               DRM_DEBUG_KMS(
>> +                               "more than one pipe active, disabling DRRS\n");
>> +                               dev_priv->drrs.is_clone = true;
>> +                               intel_disable_drrs(dev);
>> +                               return;
>> +                       }
>> +                       crtc = tmp_crtc;
>> +               }
>> +       }
>> +
>> +       if (crtc == NULL) {
>> +               DRM_DEBUG_KMS("DRRS: crtc not initialized\n");
>> +               return;
>> +       }
>> +
>> +       dev_priv->drrs.is_clone = false;
>> +       intel_disable_drrs(dev);
>> +
>> +       /* re-enable idleness detection */
>> +       intel_enable_drrs(crtc);
>> +}
>> +
>> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
>> +                                       struct intel_connector *connector)
>> +{
>> +       struct intel_drrs_work *work;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       if (i915.drrs_interval == 0)
>> +               DRM_INFO("DRRS disable by flag\n");
>> +
>> +       work = kzalloc(sizeof(struct intel_drrs_work), GFP_KERNEL);
>> +       if (!work) {
>> +               DRM_ERROR("Failed to allocate DRRS work structure\n");
>> +               return;
>> +       }
>> +
>> +       dev_priv->drrs.connector = connector;
>> +       dev_priv->drrs.is_clone = false;
>> +
>> +       work->interval = i915.drrs_interval;
>> +       INIT_DELAYED_WORK(&work->work, intel_drrs_work_fn);
>> +
>> +       dev_priv->drrs.drrs_work = work;
>> +}
>> +
>>  static void i915_pineview_get_mem_freq(struct drm_device *dev)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> --
>> 2.0.1
>>
> 
> 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux