Op 09-08-18 om 01:44 schreef Dhinakaran Pandiyan:
> On Wed, 2018-08-08 at 16:19 +0200, Maarten Lankhorst wrote:
>> Currently tests modify i915.enable_psr and then do a modeset cycle
>> to change PSR. We can write a value to i915_edp_psr_debug to force
>> a certain PSR mode without a modeset.
>>
>> To retain compatibility with older userspace, we also still allow
>> the override through the module parameter, and add some tracking
>> to check whether a debugfs mode is specified.
>>
>> Changes since v1:
>> - Rename dev_priv->psr.enabled to .dp, and .hw_configured to
>> .enabled.
>> - Fix i915_psr_debugfs_mode to match the writes to debugfs.
>> - Rename __i915_edp_psr_write to intel_psr_set_debugfs_mode, simplify
>> it and move it to intel_psr.c. This keeps all internals in
>> intel_psr.c
>> - Perform an interruptible wait for hw completion outside of the psr
>> lock, instead of being forced to trywait and return -EBUSY.
>> Changes since v2:
>> - Rebase on top of intel_psr changes.
>> Changes since v3:
>> - Assign psr.dp during init. (dhnkrn)
>> - Add prepared bool, which should be used instead of relying on
>> psr.dp. (dhnkrn)
>> - Fix -EDEADLK handling in debugfs. (dhnkrn)
>> - Clean up waiting for idle in intel_psr_set_debugfs_mode.
>> - Print PSR mode when trying to enable PSR. (dhnkrn)
>> - Move changing psr debug setting to i915_edp_psr_debug_set. (dhnkrn)
>> Changes since v4:
>> - Return error in _set() function.
>> - Change flag values to make them easier to remember. (dhnkrn)
>> - Only assign psr.dp once. (dhnkrn)
>> - Only set crtc_state->has_psr on the crtc with psr.dp.
>> - Fix typo. (dhnkrn)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 23 ++++-
>> drivers/gpu/drm/i915/i915_drv.h | 12 ++-
>> drivers/gpu/drm/i915/i915_irq.c | 2 +-
>> drivers/gpu/drm/i915/intel_drv.h | 5 +-
>> drivers/gpu/drm/i915/intel_psr.c | 138 +++++++++++++++++++++++---
>> --
>> 5 files changed, 149 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index f9ce35da4123..3e81301a94ba 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2708,7 +2708,7 @@ static int i915_edp_psr_status(struct seq_file
>> *m, void *data)
>> intel_runtime_pm_get(dev_priv);
>>
>> mutex_lock(&dev_priv->psr.lock);
>> - seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
>>> psr.enabled));
>> + seq_printf(m, "Enabled: %s\n", yesno(dev_priv-
>>> psr.enabled));
>> seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>> dev_priv->psr.busy_frontbuffer_bits);
>>
>> @@ -2750,17 +2750,32 @@ static int
>> i915_edp_psr_debug_set(void *data, u64 val)
>> {
>> struct drm_i915_private *dev_priv = data;
>> + struct drm_modeset_acquire_ctx ctx;
>> + int ret;
>>
>> if (!CAN_PSR(dev_priv))
>> return -ENODEV;
>>
>> - DRM_DEBUG_KMS("PSR debug %s\n", enableddisabled(val));
>> + DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
>>
>> intel_runtime_pm_get(dev_priv);
>> - intel_psr_irq_control(dev_priv, !!val);
>> +
>> + drm_modeset_acquire_init(&ctx,
>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>> +
>> +retry:
>> + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
>> + if (ret == -EDEADLK) {
>> + ret = drm_modeset_backoff(&ctx);
>> + if (!ret)
>> + goto retry;
>> + }
>> +
>> + drm_modeset_drop_locks(&ctx);
>> + drm_modeset_acquire_fini(&ctx);
>> +
>> intel_runtime_pm_put(dev_priv);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 657f46e0cae9..a3ea48ce1811 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -611,8 +611,17 @@ struct i915_drrs {
>>
>> struct i915_psr {
>> struct mutex lock;
>> +
>> +#define I915_PSR_DEBUG_MODE_MASK 0x0f
>> +#define I915_PSR_DEBUG_DEFAULT 0x00
>> +#define I915_PSR_DEBUG_DISABLE 0x01
>> +#define I915_PSR_DEBUG_ENABLE 0x02
>> +#define I915_PSR_DEBUG_IRQ 0x10
>> +
>> + u32 debug;
> u16?
>
>> bool sink_support;
>> - struct intel_dp *enabled;
>> + bool prepared, enabled;
>> + struct intel_dp *dp;
>> bool active;
>> struct work_struct work;
>> unsigned busy_frontbuffer_bits;
>> @@ -622,7 +631,6 @@ struct i915_psr {
>> bool alpm;
>> bool psr2_enabled;
>> u8 sink_sync_latency;
>> - bool debug;
>> ktime_t last_entry_attempt;
>> ktime_t last_exit;
>> };
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 8084e35b25c5..b2c9838442bc 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4048,7 +4048,7 @@ static int ironlake_irq_postinstall(struct
>> drm_device *dev)
>>
>> if (IS_HASWELL(dev_priv)) {
>> gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
>> - intel_psr_irq_control(dev_priv, dev_priv-
>>> psr.debug);
>> + intel_psr_irq_control(dev_priv, dev_priv->psr.debug
>> & I915_PSR_DEBUG_IRQ);
>> display_mask |= DE_EDP_PSR_INT_HSW;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 1ad7c1124bef..098ea2859a9b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1924,7 +1924,7 @@ int intel_hdcp_enable(struct intel_connector
>> *connector);
>> int intel_hdcp_disable(struct intel_connector *connector);
>> int intel_hdcp_check_link(struct intel_connector *connector);
>> bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port
>> port);
>> -
>> +//
> ^ seems like a ghost comment sneaked in.
>>
>> /* intel_psr.c */
>> #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv-
>>> psr.sink_support)
>> void intel_psr_init_dpcd(struct intel_dp *intel_dp);
>> @@ -1932,6 +1932,9 @@ void intel_psr_enable(struct intel_dp
>> *intel_dp,
>> const struct intel_crtc_state *crtc_state);
>> void intel_psr_disable(struct intel_dp *intel_dp,
>> const struct intel_crtc_state
>> *old_crtc_state);
>> +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + u64 value);
>> void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>> unsigned frontbuffer_bits,
>> enum fb_op_origin origin);
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>> b/drivers/gpu/drm/i915/intel_psr.c
>> index 4bd5768731ee..12fbc59af5a4 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -56,6 +56,18 @@
>> #include "intel_drv.h"
>> #include "i915_drv.h"
>>
>> +static bool psr_global_enabled(u32 debug)
>> +{
>> + switch (debug & I915_PSR_DEBUG_MODE_MASK) {
>> + case I915_PSR_DEBUG_DEFAULT:
>> + return i915_modparams.enable_psr;
>> + case I915_PSR_DEBUG_DISABLE:
>> + return false;
>> + default:
>> + return true;
>> + }
>> +}
>> +
>> void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
>> debug)
>> {
>> u32 debug_mask, mask;
>> @@ -80,7 +92,6 @@ void intel_psr_irq_control(struct drm_i915_private
>> *dev_priv, bool debug)
>> if (debug)
>> mask |= debug_mask;
>>
>> - WRITE_ONCE(dev_priv->psr.debug, debug);
>> I915_WRITE(EDP_PSR_IMR, ~mask);
>> }
>>
>> @@ -213,6 +224,9 @@ void intel_psr_init_dpcd(struct intel_dp
>> *intel_dp)
>> dev_priv->psr.sink_sync_latency =
>> intel_dp_get_sink_sync_latency(intel_dp);
>>
>> + WARN_ON(dev_priv->psr.dp);
>> + dev_priv->psr.dp = intel_dp;
>> +
>> if (INTEL_GEN(dev_priv) >= 9 &&
>> (intel_dp->psr_dpcd[0] ==
>> DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
>> bool y_req = intel_dp->psr_dpcd[1] &
>> @@ -471,8 +485,8 @@ void intel_psr_compute_config(struct intel_dp
>> *intel_dp,
>> if (!CAN_PSR(dev_priv))
>> return;
>>
>> - if (!i915_modparams.enable_psr) {
>> - DRM_DEBUG_KMS("PSR disable by flag\n");
>> + if (intel_dp != dev_priv->psr.dp) {
>> + DRM_DEBUG_KMS("Not enabling PSR: wrong
>> connector\n");
> This debug message will get printed for non eDP ports, return silently?
I guess move it until after the PORT_A check?
>> return;
>> }
>>
>> @@ -517,7 +531,6 @@ void intel_psr_compute_config(struct intel_dp
>> *intel_dp,
>>
>> crtc_state->has_psr = true;
>> crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
>> crtc_state);
>> - DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2"
>> : "");
>> }
>>
>> static void intel_psr_activate(struct intel_dp *intel_dp)
>> @@ -589,6 +602,24 @@ static void intel_psr_enable_source(struct
>> intel_dp *intel_dp,
>> }
>> }
>>
>> +static void intel_psr_enable_locked(struct drm_i915_private
>> *dev_priv,
>> + const struct intel_crtc_state
>> *crtc_state)
>> +{
>> + struct intel_dp *intel_dp = dev_priv->psr.dp;
>> +
>> + if (dev_priv->psr.enabled)
>> + return;
>> +
>> + DRM_DEBUG_KMS("Enabling PSR%s\n",
>> + dev_priv->psr.psr2_enabled ? "2" : "1");
>> + intel_psr_setup_vsc(intel_dp, crtc_state);
>> + intel_psr_enable_sink(intel_dp);
>> + intel_psr_enable_source(intel_dp, crtc_state);
>> + dev_priv->psr.enabled = true;
>> +
>> + intel_psr_activate(intel_dp);
>> +}
>> +
>> /**
>> * intel_psr_enable - Enable PSR
>> * @intel_dp: Intel DP
>> @@ -609,22 +640,20 @@ void intel_psr_enable(struct intel_dp
>> *intel_dp,
>> if (WARN_ON(!CAN_PSR(dev_priv)))
>> return;
>>
>> - WARN_ON(dev_priv->drrs.dp);
> This warning was to catch if DRRS was enabled along with PSR. the
> hardware supports only one at a time.
Oh right. I'll leave it there. Maybe add a DRRS disable call around PSR debugfs?
>> mutex_lock(&dev_priv->psr.lock);
>> - if (dev_priv->psr.enabled) {
>> + if (dev_priv->psr.prepared) {
>> DRM_DEBUG_KMS("PSR already in use\n");
>> goto unlock;
>> }
>>
>> dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
>> dev_priv->psr.busy_frontbuffer_bits = 0;
>> + dev_priv->psr.prepared = true;
>>
>> - intel_psr_setup_vsc(intel_dp, crtc_state);
>> - intel_psr_enable_sink(intel_dp);
>> - intel_psr_enable_source(intel_dp, crtc_state);
>> - dev_priv->psr.enabled = intel_dp;
>> -
>> - intel_psr_activate(intel_dp);
>> + if (psr_global_enabled(dev_priv->psr.debug))
>> + intel_psr_enable_locked(dev_priv, crtc_state);
>> + else
>> + DRM_DEBUG_KMS("PSR disabled by flag\n");
>>
>> unlock:
>> mutex_unlock(&dev_priv->psr.lock);
>> @@ -683,12 +712,14 @@ static void intel_psr_disable_locked(struct
>> intel_dp *intel_dp)
>> if (!dev_priv->psr.enabled)
>> return;
>>
>> + DRM_DEBUG_KMS("Disabling PSR%s\n",
>> + dev_priv->psr.psr2_enabled ? "2" : "1");
>> intel_psr_disable_source(intel_dp);
>>
>> /* Disable PSR on Sink */
>> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
>>
>> - dev_priv->psr.enabled = NULL;
>> + dev_priv->psr.enabled = false;
>> }
>>
>> /**
>> @@ -712,7 +743,14 @@ void intel_psr_disable(struct intel_dp
>> *intel_dp,
>> return;
>>
>> mutex_lock(&dev_priv->psr.lock);
>> + if (!dev_priv->psr.prepared) {
>> + mutex_unlock(&dev_priv->psr.lock);
>> + return;
>> + }
>> +
>> intel_psr_disable_locked(intel_dp);
>> +
>> + dev_priv->psr.prepared = false;
>> mutex_unlock(&dev_priv->psr.lock);
>> cancel_work_sync(&dev_priv->psr.work);
>> }
>> @@ -724,9 +762,6 @@ int intel_psr_wait_for_idle(const struct
>> intel_crtc_state *new_crtc_state)
>> i915_reg_t reg;
>> u32 mask;
>>
>> - if (!new_crtc_state->has_psr)
>> - return 0;
>> -
> We'll end up waiting for PSR to idle when updating CRTCs that are not
> driving a PSR panel. I should have checked this in the previous
> version, my bad.
>
> The patch looks good to me other than for the issues pointed out.
Ok, will reinstate this.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
