Re: [PATCH] drm/i915: Add functions to force psr exit

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

 



On Thu, Jul 18, 2013 at 5:33 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote:
>> PSR tracking engine in HSW doesn't detect automagically some directly copy area
>> operations through scanout so we will have to kick it manually and
>> reschedule it to come back to normal operation as soon as possible.
>>
>> v2: Before PSR Hook. Don't force it when busy yet.
>> v3/v4: Solved small conflict.
>> v5: setup once function was already added on previous commit.
>> v6: Use POSTING_READ to make sure the mutex works as a write barrier as
>>     suggested by Chris Wilson
>>
>> CC: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>  drivers/gpu/drm/i915/intel_dp.c  | 48 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>>  3 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 3bca337..dc10345 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1840,6 +1840,7 @@
>>  #define   EDP_PSR_PERF_CNT_MASK              0xffffff
>>
>>  #define EDP_PSR_DEBUG_CTL            0x64860
>> +#define   EDP_PSR_DEBUG_FORCE_EXIT   (3<<30)
>>  #define   EDP_PSR_DEBUG_MASK_LPSP    (1<<27)
>>  #define   EDP_PSR_DEBUG_MASK_MEMUP   (1<<26)
>>  #define   EDP_PSR_DEBUG_MASK_HPD     (1<<25)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3c9473c..47e1676 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
>>       return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>>  }
>>
>> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
>> +{
>> +     struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
>> +                                              struct intel_dp,
>> +                                              edp_psr_delayed_normal_work);
>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     mutex_lock(&intel_dp->psr_exit_mutex);
>> +     I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
>> +                ~EDP_PSR_DEBUG_FORCE_EXIT);
>> +     POSTING_READ(EDP_PSR_DEBUG_CTL);
>> +     mutex_unlock(&intel_dp->psr_exit_mutex);
>> +}
>> +
>> +void intel_edp_psr_force_exit(struct drm_device *dev)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     struct intel_encoder *encoder;
>> +     struct intel_dp *intel_dp = NULL;
>> +
>> +     if (!intel_edp_is_psr_enabled(dev))
>> +             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);
>> +
>> +     if (!intel_dp)
>> +             return;
>> +
>> +     if (WARN_ON(!intel_dp->psr_setup_done))
>> +             return;
>
> If you have to dig out your data like this it usually means that it's at
> the wrong spot. Imo we should track a psr_possible bit in the pipe_config
> and a psr_enabled (and the locking for the runtime mutex) in the crtc
> itself. Also, the psr_setup_done thing is another hint that we should move
> this into the crtc.

Ok, I'm going to play a bit with this and see what I can come up with.

>
> Second I prefer if functions with tricky tie-in with existing code are
> used in the same patch they're created - if it's split over two patches
> review is a bit a pain.

Agreed. I just splited out because I accepted review suggestions to
let all hooks for the last patch to avoid breaking bisects or
something like that.

> But if I read the follow-up patch correctly we
> call this function from the busy_ioctl unconditionally, which will
> obviously result in tons of falls postives - e.g. libdrm uses this ioctl
> to manage it's buffer cache.

Yes, you are right. Although I noticed that psr was still working
fine, I agree that this is too many unecessary calls :(
But couldn't find a better way to fix xdm/kde issue.
Other ideas was letting psr disabled for so long when it could be
there saving power.
But as I said, low priority on this right now... Maybe when following
your suggestions and the list you made we get it fixed properly ;)

> I think I'll punt on this patch and merge just the parts from the next one
> for normal backbuffer rendering with pageflips model.
> -Daniel

Thanks again,
Rodrigo
>
>> +
>> +     mutex_lock(&intel_dp->psr_exit_mutex);
>> +     I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
>> +                EDP_PSR_DEBUG_FORCE_EXIT);
>> +     POSTING_READ(EDP_PSR_DEBUG_CTL);
>> +     mutex_unlock(&intel_dp->psr_exit_mutex);
>> +
>> +     schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
>> +                           msecs_to_jiffies(100));
>> +}
>> +
>>  static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
>>                                   struct edp_vsc_psr *vsc_psr)
>>  {
>> @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>>       I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
>>                  EDP_PSR_DEBUG_MASK_HPD);
>>
>> +     INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work,
>> +                       intel_edp_psr_delayed_normal_work);
>> +     mutex_init(&intel_dp->psr_exit_mutex);
>> +
>>       intel_dp->psr_setup_done = true;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0f52362..e47f3f3 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -499,6 +499,8 @@ struct intel_dp {
>>       int backlight_off_delay;
>>       struct delayed_work panel_vdd_work;
>>       bool want_panel_vdd;
>> +     struct delayed_work edp_psr_delayed_normal_work;
>> +     struct mutex psr_exit_mutex;
>>       bool psr_setup_done;
>>       struct intel_connector *attached_connector;
>>  };
>> @@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>  extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>  extern void intel_edp_psr_update(struct drm_device *dev);
>>  extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
>> +extern void intel_edp_psr_force_exit(struct drm_device *dev);
>>
>>  #endif /* __INTEL_DRV_H__ */
>> --
>> 1.8.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
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