Re: [PATCH 09/10] drm/i915/psr: report live PSR2 State

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: vathsala nagaraju <vathsala.nagaraju@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Patil Deepti <deepti.patil@intel.com>,
	Jim Bride <jim.bride@linux.intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 09/10] drm/i915/psr: report live PSR2 State
Date: Thu, 5 Jan 2017 09:47:50 -0800	[thread overview]
Message-ID: <20170105174750.GB13837@intel.com> (raw)
In-Reply-To: <1483356663-32668-10-git-send-email-vathsala.nagaraju@intel.com>

I like this live status!

On Mon, Jan 02, 2017 at 05:01:02PM +0530, vathsala nagaraju wrote:
> Reports  live state of PSR2 form PSR2_STATUS register.
> bit field 31:28 gives the live state of PSR2.
> It can be used to check if system is in deep sleep,
> selective update or selective update standby.
> During video play back, we can use this to check
> if system is entering SU mode or not.
> when system is in idle state, DEEP_SLEEP(8) must be entered.
> When video playback is happening, system must be in
> SLEEP(3 / selective update) or SU_STANDBY( 6 / selective update standby)
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Patil Deepti <deepti.patil@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9d7b5a8..8b0e3f9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2606,6 +2606,30 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  		seq_printf(m, "Performance_Counter: %u\n", psrperf);
>  	}
> +	if (dev_priv->psr.psr2_support) {
> +		static const char * const live_status[] = {
> +							"IDLE",
> +							"CAPTURE",
> +							"CAPTURE_Fs",

Fs? all others are fully capital

> +							"SLEEP",
> +							"BUFON_FW",
> +							"ML_UP",
> +							"SU_STANDBY",
> +							"FAST_SLEEP",
> +							"DEEP_SLEEP",
> +							"BUF_ON",
> +							"TG_ON" };
> +		u8 pos = (I915_READ(EDP_PSR2_STATUS_CTL) &
> +			EDP_PSR2_STATUS_STATE_MASK) >>
> +			EDP_PSR2_STATUS_STATE_SHIFT;
> +
> +		seq_printf(m, "PSR2_STATUS_EDP: %x\n",
> +			I915_READ(EDP_PSR2_STATUS_CTL));
> +
> +		if (pos <= EDP_PSR2_STATUS_TG_ON)

I like this protection...

> +		seq_printf(m, "PSR2 live state %s\n",
> +			live_status[pos]);
> +	}
>  	mutex_unlock(&dev_priv->psr.lock);
>  
>  	intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0cbe564..03a14d9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3624,6 +3624,8 @@ enum {
>  
>  #define EDP_PSR2_STATUS_CTL            _MMIO(0x6f940)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
> +#define EDP_PSR2_STATUS_STATE_SHIFT    28
> +#define EDP_PSR2_STATUS_TG_ON          0xa

... but I don't like how this TG_ON is defined,
because following the general style
EDP_PSR2_STATUS_TG_ON should be (0xa<<28)
so it might confuse people...

maybe just remove this and use the protection with size of live_status?

>  
>  /* VGA port control */
>  #define ADPA			_MMIO(0x61100)
> -- 
> 1.9.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel