On Mon, 15 Apr 2019, José Roberto de Souza <jose.so...@intel.com> wrote:
> PSR registers are a mess, some have the full address while others just
> have the additional offset from psr_mmio_base.
>
> psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET + 0x800 and
> using it makes more difficult for people with an PSR register address
> from BSpec to search the register name in i915 as also the BSpec name
> don't match with the name in i915.
>
> The other option would be use the whole hard-coded address but this is
> not future proof, so here going in the middle ground by making every
> PSR register relative to transcoder(that is EDP transcoder), the only
> exception is PSR_IMR/IIR that is not relative to nothing.
> For the _TRANS2() macros to work it needs the address of the register
> from the TRANSCODER_A, so adding it to every register together with
> the register address from the EDP transcoder so it will make easy for
> people searching with BSpec address also adding those with the BSpec
> name.
>
> For Haswell all the PSR register are relative to 0x64000, so
> mmio_base_adjust was added and used to take care of that.
>
> Also removing BDW_EDP_PSR_BASE from GVT because it is not used as
> the only PSR register that GVT have is this one(SRD/PSR_CTL).
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.so...@intel.com>
> ---
> drivers/gpu/drm/i915/gvt/handlers.c | 1 -
> drivers/gpu/drm/i915/i915_drv.h | 5 +--
> drivers/gpu/drm/i915/i915_reg.h | 52 ++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_psr.c | 11 ++++--
> 4 files changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 86761b1def1e..d09b798e93cb 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -2739,7 +2739,6 @@ static int init_broadwell_mmio_info(struct intel_gvt
> *gvt)
> MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>
> MMIO_D(WM_MISC, D_BDW);
> - MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
>
> MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
> MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 35d0782c077e..a9666290f0b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -494,6 +494,8 @@ struct i915_drrs {
> };
>
> struct i915_psr {
> + /* different than zero only on HSW see _TRANS2_PSR() for more info */
> + u32 mmio_base_adjust;
> struct mutex lock;
>
> #define I915_PSR_DEBUG_MODE_MASK 0x0f
> @@ -508,6 +510,7 @@ struct i915_psr {
> bool enabled;
> struct intel_dp *dp;
> enum pipe pipe;
> + enum transcoder transcoder;
> bool active;
> struct work_struct work;
> unsigned busy_frontbuffer_bits;
> @@ -1534,8 +1537,6 @@ struct drm_i915_private {
> /* MMIO base address for MIPI regs */
> u32 mipi_mmio_base;
>
> - u32 psr_mmio_base;
> -
> u32 pps_mmio_base;
>
> wait_queue_head_t gmbus_wait_queue;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 36420af2cd6f..094bd19abb35 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4213,9 +4213,15 @@ enum {
> #define PIPE_MULT(trans) _MMIO_TRANS2(trans, _PIPE_MULT_A)
>
> /* HSW+ eDP PSR registers */
> -#define HSW_EDP_PSR_BASE 0x64800
> -#define BDW_EDP_PSR_BASE 0x6f800
> -#define EDP_PSR_CTL _MMIO(dev_priv->psr_mmio_base +
> 0)
> +#define HSW_EDP_PSR_BASE 0x64000
> +
> +/* PSR registers on HSW is not relative to eDP transcoder */
> +#define _TRANS2_PSR(reg) (_TRANS2(dev_priv->psr.transcoder, (reg)) -
> dev_priv->psr.mmio_base_adjust)
> +#define _MMIO_TRANS2_PSR(reg) _MMIO(_TRANS2_PSR(reg))
I really think this kind of macros should be at the top next to all the
indexing macros. I understand the desire to put them where they're used,
but having them at the top gives you a better idea what's available and
what to pick for the next use and what could be abstracted.
BR,
Jani.
> +
> +#define _SRD_CTL_A 0x60800
> +#define _SRD_CTL_EDP 0x6F800
> +#define EDP_PSR_CTL _MMIO_TRANS2_PSR(_SRD_CTL_A)
> #define EDP_PSR_ENABLE (1 << 31)
> #define BDW_PSR_SINGLE_FRAME (1 << 30)
> #define EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK (1 << 29) /* SW can't
> modify */
> @@ -4248,16 +4254,22 @@ enum {
> #define EDP_PSR_POST_EXIT (1 << 1)
> #define EDP_PSR_PRE_ENTRY (1 << 0)
>
> -#define EDP_PSR_AUX_CTL
> _MMIO(dev_priv->psr_mmio_base + 0x10)
> +#define _SRD_AUX_CTL_A 0x60810
> +#define _SRD_AUX_CTL_EDP 0x6F810
> +#define EDP_PSR_AUX_CTL
> _MMIO_TRANS2_PSR(_SRD_AUX_CTL_A)
> #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> #define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK (0x1f << 20)
> #define EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK (0xf << 16)
> #define EDP_PSR_AUX_CTL_ERROR_INTERRUPT (1 << 11)
> #define EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK (0x7ff)
>
> -#define EDP_PSR_AUX_DATA(i) _MMIO(dev_priv->psr_mmio_base +
> 0x14 + (i) * 4) /* 5 registers */
> +#define _SRD_AUX_DATA_A 0x60814
> +#define _SRD_AUX_DATA_EDP 0x6F814
> +#define EDP_PSR_AUX_DATA(i)
> _MMIO(_TRANS2_PSR(_SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
>
> -#define EDP_PSR_STATUS
> _MMIO(dev_priv->psr_mmio_base + 0x40)
> +#define _SRD_STATUS_A 0x60840
> +#define _SRD_STATUS_EDP 0x6F840
> +#define EDP_PSR_STATUS
> _MMIO_TRANS2_PSR(_SRD_STATUS_A)
> #define EDP_PSR_STATUS_STATE_MASK (7 << 29)
> #define EDP_PSR_STATUS_STATE_SHIFT 29
> #define EDP_PSR_STATUS_STATE_IDLE (0 << 29)
> @@ -4282,10 +4294,15 @@ enum {
> #define EDP_PSR_STATUS_SENDING_TP1 (1 << 4)
> #define EDP_PSR_STATUS_IDLE_MASK 0xf
>
> -#define EDP_PSR_PERF_CNT _MMIO(dev_priv->psr_mmio_base + 0x44)
> +#define _SRD_PERF_CNT_A 0x60844
> +#define _SRD_PERF_CNT_EDP 0x6F844
> +#define EDP_PSR_PERF_CNT _MMIO_TRANS2_PSR(_SRD_PERF_CNT_A)
> #define EDP_PSR_PERF_CNT_MASK 0xffffff
>
> -#define EDP_PSR_DEBUG
> _MMIO(dev_priv->psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> +/* PSR_MASK on SKL+ */
> +#define _SRD_DEBUG_A 0x60860
> +#define _SRD_DEBUG_EDP 0x6F860
> +#define EDP_PSR_DEBUG
> _MMIO_TRANS2_PSR(_SRD_DEBUG_A)
> #define EDP_PSR_DEBUG_MASK_MAX_SLEEP (1 << 28)
> #define EDP_PSR_DEBUG_MASK_LPSP (1 << 27)
> #define EDP_PSR_DEBUG_MASK_MEMUP (1 << 26)
> @@ -4293,7 +4310,9 @@ enum {
> #define EDP_PSR_DEBUG_MASK_DISP_REG_WRITE (1 << 16) /* Reserved in ICL+
> */
> #define EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>
> -#define EDP_PSR2_CTL _MMIO(0x6f900)
> +#define _PSR2_CTL_A 0x60900
> +#define _PSR2_CTL_EDP 0x6F900
> +#define EDP_PSR2_CTL _MMIO_TRANS2_PSR(_PSR2_CTL_A)
> #define EDP_PSR2_ENABLE (1 << 31)
> #define EDP_SU_TRACK_ENABLE (1 << 30)
> #define EDP_Y_COORDINATE_VALID (1 << 26) /* GLK and CNL+ */
> @@ -4311,7 +4330,9 @@ enum {
> #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> #define EDP_PSR2_IDLE_FRAME_SHIFT 0
>
> -#define PSR_EVENT _MMIO(0x6F848)
> +#define _PSR_EVENT_A 0x60848
> +#define _PSR_EVENT_EDP 0x6F848
> +#define PSR_EVENT _MMIO_TRANS2_PSR(_PSR_EVENT_A)
> #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17)
> #define PSR_EVENT_PSR2_DISABLED (1 << 16)
> #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15)
> @@ -4329,14 +4350,15 @@ enum {
> #define PSR_EVENT_LPSP_MODE_EXIT (1 << 1)
> #define PSR_EVENT_PSR_DISABLE (1 << 0)
>
> -#define EDP_PSR2_STATUS _MMIO(0x6f940)
> +#define _PSR2_STATUS_A 0x60940
> +#define _PSR2_STATUS_EDP 0x6F940
> +#define EDP_PSR2_STATUS _MMIO_TRANS2_PSR(_PSR2_STATUS_A)
> #define EDP_PSR2_STATUS_STATE_MASK (0xf << 28)
> #define EDP_PSR2_STATUS_STATE_SHIFT 28
>
> -#define _PSR2_SU_STATUS_0 0x6F914
> -#define _PSR2_SU_STATUS_1 0x6F918
> -#define _PSR2_SU_STATUS_2 0x6F91C
> -#define _PSR2_SU_STATUS(index) _MMIO(_PICK_EVEN((index),
> _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> +#define _PSR2_SU_STATUS_A 0x60914
> +#define _PSR2_SU_STATUS_EDP 0x6F914
> +#define _PSR2_SU_STATUS(index)
> _MMIO(_TRANS2_PSR(_PSR2_SU_STATUS_A) + (index) * 4)
> #define PSR2_SU_STATUS(frame) (_PSR2_SU_STATUS((frame) / 3))
> #define PSR2_SU_STATUS_SHIFT(frame) (((frame) % 3) * 10)
> #define PSR2_SU_STATUS_MASK(frame) (0x3ff << PSR2_SU_STATUS_SHIFT(frame))
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 581774748c4c..4e3d74d1b227 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -671,6 +671,14 @@ static void intel_psr_enable_locked(struct
> drm_i915_private *dev_priv,
> dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> dev_priv->psr.busy_frontbuffer_bits = 0;
> dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> + dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> +
> + if (IS_HASWELL(dev_priv)) {
> + u32 trans_offset =
> INTEL_INFO(dev_priv)->trans_offsets[dev_priv->psr.transcoder];
> +
> + WARN_ON(trans_offset < HSW_EDP_PSR_BASE);
> + dev_priv->psr.mmio_base_adjust = trans_offset -
> HSW_EDP_PSR_BASE;
> + }
>
> DRM_DEBUG_KMS("Enabling PSR%s\n",
> dev_priv->psr.psr2_enabled ? "2" : "1");
> @@ -1135,9 +1143,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> if (!HAS_PSR(dev_priv))
> return;
>
> - dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> - HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> -
> if (!dev_priv->psr.sink_support)
> return;
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
