Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Thu, 2019-04-04 at 14:51 -0700, Rodrigo Vivi wrote:
> On Thu, Apr 04, 2019 at 02:41:26PM -0700, Pandiyan, Dhinakaran wrote:
> > On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote:
> > > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote:
> > > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote:
> > > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de
> > > > > Souza
> > > > > wrote:
> > > > > > PSR is only supported in eDP transcoder and there is only
> > > > > > one
> > > > > > instance of it, so lets drop all of this code.
> > > > >
> > > > > Is this sentence true? I mean, in the way it is written it
> > > > > seems like HW doesn't actually support it...
> > > > > Or should we re-phrase for we are not really enabling support
> > > > > for other transcoders than eDP and we do not have plans to do
> > > > > it so soon so let's clean the code...
> > > > > or something like that?
> > > >
> > > > Okay, what about replace it for:
> > > >
> > > > Since BDW all transcoders have PSR registers but only EDP
> > > > transcoder
> > > > can drive a EDP panel and as PSR is only part of the EDP
> > > > specification,
> > > > for real usage PSR is only supported in EDP panel, so lets drop
> > > > all of
> > > > this useless code.
> > >
> > > well, this still looks like HW limitation. If PSR is supported by
> > > HW on
> > > different transcoders is because there's some possibility of
> > > adding
> > > eDP on other transcoders. They wouldn't waste so many register
> > > space
> > > if that wasn't the case.
> > >
> > > Even though we have a dedicated transcoder for eDP I don't
> > > believe we can assume that eDP is not supported on the other
> > > ones.
> > >
> >
> > Why not write something like (or exactly) this
> >
> > "i915 does not support enabling PSR on any transcoder other than
> > eDP. Clean up
> > the misleading non-eDP code that currently exists to allow for the
> > rework of PSR
> > register definitions in the next patch"
>
> +1
Sure, going to change to that in the next version.
>
> >
> > -DK
> > > >
> > > >
> > > > > > Cc: Dhinakaran Pandiyan
> > > > > > Cc: Rodrigo Vivi
> > > > > > Signed-off-by: José Roberto de Souza
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +---
> > > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ---
> > > > > >
> > > > > >
> > > > > > 2 files changed, 42 insertions(+), 122 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index c59cfa83dbaf..18e2991b376d 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -4241,13 +4241,9 @@ enum {
> > > > > > /* Bspec claims those aren't shifted but stay at 0x64800
> > > > > > */
> > > > > > #define EDP_PSR_IMR_MMIO(0
> > > > > > x64834)
> > > > > > #define EDP_PSR_IIR_MMIO(0
> > > > > > x64838)
> > > > > > -#define EDP_PSR_ERROR(shift) (1 <<
> > > > > > ((shift)
> > > > > > + 2))
> > > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift)
> > > > > > + 1))
> > > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > > > > > +#define EDP_PSR_ERROR(1 <<
> > > > > > 2)
> > > > > > +#define EDP_PSR_POST_EXIT(1 <<
> > > > > > 1)
> > > > > > +#define EDP_PSR_PRE_ENTRY(1 <<
> > > > > > 0)
> > > > > >
> > > > > > #define EDP_PSR_AUX_CTL_MMIO(d
> > > > > > ev_priv-
> > > > > > > psr_mmio_base + 0x10)
> > > > > >
> > > > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK(3 <<
> > > > > > 26)
> > > > > > @@ -4312,12 +4308,7 @@ enum {
> > > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> > > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT0
> > > > > >
> > > > > > -#define _PSR_EVENT_TRANS_A 0x60848
> > > > > > -#define _PSR_EVENT_TRANS_B 0x61848
> > > > > > -#define _PSR_EVENT_TRANS_C 0x62848
> > > > > > -#define _PSR_EVENT_TRANS_D 0x63848
> > > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848
> > > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(tr
> > > > > > ans,
> > > > > > _PSR_EVENT_TRANS_A)
> > > > > > +#define PSR_EVENT _MMIO(0x6F848)
> > > > > > #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)
> > > > > > diff --git
Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Thu, 2019-04-04 at 14:41 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote:
> > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote:
> > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote:
> > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza
> > > > wrote:
> > > > > PSR is only supported in eDP transcoder and there is only one
> > > > > instance of it, so lets drop all of this code.
> > > >
> > > > Is this sentence true? I mean, in the way it is written it
> > > > seems like HW doesn't actually support it...
> > > > Or should we re-phrase for we are not really enabling support
> > > > for other transcoders than eDP and we do not have plans to do
> > > > it so soon so let's clean the code...
> > > > or something like that?
> > >
> > > Okay, what about replace it for:
> > >
> > > Since BDW all transcoders have PSR registers but only EDP transcoder
> > > can drive a EDP panel and as PSR is only part of the EDP specification,
> > > for real usage PSR is only supported in EDP panel, so lets drop all of
> > > this useless code.
> >
> > well, this still looks like HW limitation. If PSR is supported by HW on
> > different transcoders is because there's some possibility of adding
> > eDP on other transcoders. They wouldn't waste so many register space
> > if that wasn't the case.
> >
> > Even though we have a dedicated transcoder for eDP I don't
> > believe we can assume that eDP is not supported on the other ones.
> >
>
> Why not write something like (or exactly) this
>
> "i915 does not support enabling PSR on any transcoder other than eDP. Clean up
> the misleading non-eDP code that currently exists to allow for the rework of
> PSR
> register definitions in the next patch"
>
>
> -DK
> > >
> > >
> > >
> > > >
> > > > > Cc: Dhinakaran Pandiyan
> > > > > Cc: Rodrigo Vivi
> > > > > Signed-off-by: José Roberto de Souza
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +---
> > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ---
> > > > >
> > > > > 2 files changed, 42 insertions(+), 122 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index c59cfa83dbaf..18e2991b376d 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -4241,13 +4241,9 @@ enum {
> > > > > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > > > > #define EDP_PSR_IMR _MMIO(0x64834)
> > > > > #define EDP_PSR_IIR _MMIO(0x64838)
> > > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift)
> > > > > + 2))
> > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > > > > +#define EDP_PSR_ERROR (1 << 2)
> > > > > +#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 EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> > > > > @@ -4312,12 +4308,7 @@ enum {
> > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0
> > > > >
> > > > > -#define _PSR_EVENT_TRANS_A 0x60848
> > > > > -#define _PSR_EVENT_TRANS_B 0x61848
> > > > > -#define _PSR_EVENT_TRANS_C 0x62848
> > > > > -#define _PSR_EVENT_TRANS_D 0x63848
> > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848
> > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> > > > > _PSR_EVENT_TRANS_A)
> > > > > +#define PSR_EVENT_MMIO(0x6F848)
> > > > > #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)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index bb97c1657493..b984e005b72e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct
> > > > > drm_i915_private *dev_priv,
> > > > > }
> > > > > }
> > > > >
> > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > > > > -{
> > > > > - switch (cpu_transcoder) {
> > > > > - case TRANSCODER_A:
> > > > > -
Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Thu, Apr 04, 2019 at 02:41:26PM -0700, Pandiyan, Dhinakaran wrote:
> On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote:
> > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote:
> > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote:
> > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza
> > > > wrote:
> > > > > PSR is only supported in eDP transcoder and there is only one
> > > > > instance of it, so lets drop all of this code.
> > > >
> > > > Is this sentence true? I mean, in the way it is written it
> > > > seems like HW doesn't actually support it...
> > > > Or should we re-phrase for we are not really enabling support
> > > > for other transcoders than eDP and we do not have plans to do
> > > > it so soon so let's clean the code...
> > > > or something like that?
> > >
> > > Okay, what about replace it for:
> > >
> > > Since BDW all transcoders have PSR registers but only EDP transcoder
> > > can drive a EDP panel and as PSR is only part of the EDP specification,
> > > for real usage PSR is only supported in EDP panel, so lets drop all of
> > > this useless code.
> >
> > well, this still looks like HW limitation. If PSR is supported by HW on
> > different transcoders is because there's some possibility of adding
> > eDP on other transcoders. They wouldn't waste so many register space
> > if that wasn't the case.
> >
> > Even though we have a dedicated transcoder for eDP I don't
> > believe we can assume that eDP is not supported on the other ones.
> >
>
> Why not write something like (or exactly) this
>
> "i915 does not support enabling PSR on any transcoder other than eDP. Clean up
> the misleading non-eDP code that currently exists to allow for the rework of
> PSR
> register definitions in the next patch"
+1
>
>
> -DK
> > >
> > >
> > >
> > > >
> > > > > Cc: Dhinakaran Pandiyan
> > > > > Cc: Rodrigo Vivi
> > > > > Signed-off-by: José Roberto de Souza
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +---
> > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ---
> > > > >
> > > > > 2 files changed, 42 insertions(+), 122 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index c59cfa83dbaf..18e2991b376d 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -4241,13 +4241,9 @@ enum {
> > > > > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > > > > #define EDP_PSR_IMR _MMIO(0x64834)
> > > > > #define EDP_PSR_IIR _MMIO(0x64838)
> > > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift)
> > > > > + 2))
> > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > > > > +#define EDP_PSR_ERROR (1 << 2)
> > > > > +#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 EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> > > > > @@ -4312,12 +4308,7 @@ enum {
> > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0
> > > > >
> > > > > -#define _PSR_EVENT_TRANS_A 0x60848
> > > > > -#define _PSR_EVENT_TRANS_B 0x61848
> > > > > -#define _PSR_EVENT_TRANS_C 0x62848
> > > > > -#define _PSR_EVENT_TRANS_D 0x63848
> > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848
> > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> > > > > _PSR_EVENT_TRANS_A)
> > > > > +#define PSR_EVENT_MMIO(0x6F848)
> > > > > #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)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index bb97c1657493..b984e005b72e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct
> > > > > drm_i915_private *dev_priv,
> > > > > }
> > > > > }
> > > > >
> > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > > > > -{
> > > > > - switch (cpu_transcoder) {
> > > > > - case TRANSCODER_A:
> > >
Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote:
> On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote:
> > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote:
> > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza
> > > wrote:
> > > > PSR is only supported in eDP transcoder and there is only one
> > > > instance of it, so lets drop all of this code.
> > >
> > > Is this sentence true? I mean, in the way it is written it
> > > seems like HW doesn't actually support it...
> > > Or should we re-phrase for we are not really enabling support
> > > for other transcoders than eDP and we do not have plans to do
> > > it so soon so let's clean the code...
> > > or something like that?
> >
> > Okay, what about replace it for:
> >
> > Since BDW all transcoders have PSR registers but only EDP transcoder
> > can drive a EDP panel and as PSR is only part of the EDP specification,
> > for real usage PSR is only supported in EDP panel, so lets drop all of
> > this useless code.
>
> well, this still looks like HW limitation. If PSR is supported by HW on
> different transcoders is because there's some possibility of adding
> eDP on other transcoders. They wouldn't waste so many register space
> if that wasn't the case.
>
> Even though we have a dedicated transcoder for eDP I don't
> believe we can assume that eDP is not supported on the other ones.
>
Why not write something like (or exactly) this
"i915 does not support enabling PSR on any transcoder other than eDP. Clean up
the misleading non-eDP code that currently exists to allow for the rework of PSR
register definitions in the next patch"
-DK
> >
> >
> >
> > >
> > > > Cc: Dhinakaran Pandiyan
> > > > Cc: Rodrigo Vivi
> > > > Signed-off-by: José Roberto de Souza
> > > > ---
> > > > drivers/gpu/drm/i915/i915_reg.h | 17 +---
> > > > drivers/gpu/drm/i915/intel_psr.c | 147 ---
> > > >
> > > > 2 files changed, 42 insertions(+), 122 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index c59cfa83dbaf..18e2991b376d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -4241,13 +4241,9 @@ enum {
> > > > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > > > #define EDP_PSR_IMR_MMIO(0x64834)
> > > > #define EDP_PSR_IIR_MMIO(0x64838)
> > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift)
> > > > + 2))
> > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > > > +#define EDP_PSR_ERROR(1 << 2)
> > > > +#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 EDP_PSR_AUX_CTL_TIME_OUT_MASK(3 << 26)
> > > > @@ -4312,12 +4308,7 @@ enum {
> > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> > > > #define EDP_PSR2_IDLE_FRAME_SHIFT0
> > > >
> > > > -#define _PSR_EVENT_TRANS_A 0x60848
> > > > -#define _PSR_EVENT_TRANS_B 0x61848
> > > > -#define _PSR_EVENT_TRANS_C 0x62848
> > > > -#define _PSR_EVENT_TRANS_D 0x63848
> > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848
> > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> > > > _PSR_EVENT_TRANS_A)
> > > > +#define PSR_EVENT _MMIO(0x6F848)
> > > > #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)
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index bb97c1657493..b984e005b72e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct
> > > > drm_i915_private *dev_priv,
> > > > }
> > > > }
> > > >
> > > > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > > > -{
> > > > - switch (cpu_transcoder) {
> > > > - case TRANSCODER_A:
> > > > - return EDP_PSR_TRANSCODER_A_SHIFT;
> > > > - case TRANSCODER_B:
> > > > - return EDP_PSR_TRANSCODER_B_SHIFT;
> > > > - case TRANSCODER_C:
> > > > - return EDP_PSR_TRANSCODER_C_SHIFT;
> > > >
Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote:
> On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote:
> > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza
> > wrote:
> > > PSR is only supported in eDP transcoder and there is only one
> > > instance of it, so lets drop all of this code.
> >
> > Is this sentence true? I mean, in the way it is written it
> > seems like HW doesn't actually support it...
> > Or should we re-phrase for we are not really enabling support
> > for other transcoders than eDP and we do not have plans to do
> > it so soon so let's clean the code...
> > or something like that?
>
> Okay, what about replace it for:
>
> Since BDW all transcoders have PSR registers but only EDP transcoder
> can drive a EDP panel and as PSR is only part of the EDP specification,
> for real usage PSR is only supported in EDP panel, so lets drop all of
> this useless code.
well, this still looks like HW limitation. If PSR is supported by HW on
different transcoders is because there's some possibility of adding
eDP on other transcoders. They wouldn't waste so many register space
if that wasn't the case.
Even though we have a dedicated transcoder for eDP I don't
believe we can assume that eDP is not supported on the other ones.
>
>
>
> >
> > > Cc: Dhinakaran Pandiyan
> > > Cc: Rodrigo Vivi
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 17 +---
> > > drivers/gpu/drm/i915/intel_psr.c | 147 ---
> > >
> > > 2 files changed, 42 insertions(+), 122 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index c59cfa83dbaf..18e2991b376d 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4241,13 +4241,9 @@ enum {
> > > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > > #define EDP_PSR_IMR _MMIO(0x64834)
> > > #define EDP_PSR_IIR _MMIO(0x64838)
> > > -#define EDP_PSR_ERROR(shift) (1 << ((shift)
> > > + 2))
> > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > > +#define EDP_PSR_ERROR (1 << 2)
> > > +#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 EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> > > @@ -4312,12 +4308,7 @@ enum {
> > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0
> > >
> > > -#define _PSR_EVENT_TRANS_A 0x60848
> > > -#define _PSR_EVENT_TRANS_B 0x61848
> > > -#define _PSR_EVENT_TRANS_C 0x62848
> > > -#define _PSR_EVENT_TRANS_D 0x63848
> > > -#define _PSR_EVENT_TRANS_EDP 0x6F848
> > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> > > _PSR_EVENT_TRANS_A)
> > > +#define PSR_EVENT_MMIO(0x6F848)
> > > #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)
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index bb97c1657493..b984e005b72e 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct
> > > drm_i915_private *dev_priv,
> > > }
> > > }
> > >
> > > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > > -{
> > > - switch (cpu_transcoder) {
> > > - case TRANSCODER_A:
> > > - return EDP_PSR_TRANSCODER_A_SHIFT;
> > > - case TRANSCODER_B:
> > > - return EDP_PSR_TRANSCODER_B_SHIFT;
> > > - case TRANSCODER_C:
> > > - return EDP_PSR_TRANSCODER_C_SHIFT;
> > > - default:
> > > - MISSING_CASE(cpu_transcoder);
> > > - /* fallthrough */
> > > - case TRANSCODER_EDP:
> > > - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > > - }
> > > -}
> > > -
> > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32
> > > debug)
> > > {
> > > - u32 debug_mask, mask;
> > > - enum transcoder cpu_transcoder;
> > > - u32 transcoders = BIT(TRANSCODER_EDP);
> > > -
> > > - if (INTEL_GEN(dev_priv) >= 8)
> > > - transcoders |= BIT(TRANSCODER_A) |
> > > -BIT(TRANSCODER_B) |
> > >
Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote:
> On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza
> wrote:
> > PSR is only supported in eDP transcoder and there is only one
> > instance of it, so lets drop all of this code.
>
> Is this sentence true? I mean, in the way it is written it
> seems like HW doesn't actually support it...
> Or should we re-phrase for we are not really enabling support
> for other transcoders than eDP and we do not have plans to do
> it so soon so let's clean the code...
> or something like that?
Okay, what about replace it for:
Since BDW all transcoders have PSR registers but only EDP transcoder
can drive a EDP panel and as PSR is only part of the EDP specification,
for real usage PSR is only supported in EDP panel, so lets drop all of
this useless code.
>
> > Cc: Dhinakaran Pandiyan
> > Cc: Rodrigo Vivi
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 17 +---
> > drivers/gpu/drm/i915/intel_psr.c | 147 ---
> >
> > 2 files changed, 42 insertions(+), 122 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index c59cfa83dbaf..18e2991b376d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4241,13 +4241,9 @@ enum {
> > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > #define EDP_PSR_IMR_MMIO(0x64834)
> > #define EDP_PSR_IIR_MMIO(0x64838)
> > -#define EDP_PSR_ERROR(shift) (1 << ((shift)
> > + 2))
> > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > +#define EDP_PSR_ERROR(1 << 2)
> > +#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 EDP_PSR_AUX_CTL_TIME_OUT_MASK(3 << 26)
> > @@ -4312,12 +4308,7 @@ enum {
> > #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> > #define EDP_PSR2_IDLE_FRAME_SHIFT0
> >
> > -#define _PSR_EVENT_TRANS_A 0x60848
> > -#define _PSR_EVENT_TRANS_B 0x61848
> > -#define _PSR_EVENT_TRANS_C 0x62848
> > -#define _PSR_EVENT_TRANS_D 0x63848
> > -#define _PSR_EVENT_TRANS_EDP 0x6F848
> > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> > _PSR_EVENT_TRANS_A)
> > +#define PSR_EVENT _MMIO(0x6F848)
> > #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)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index bb97c1657493..b984e005b72e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct
> > drm_i915_private *dev_priv,
> > }
> > }
> >
> > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > -{
> > - switch (cpu_transcoder) {
> > - case TRANSCODER_A:
> > - return EDP_PSR_TRANSCODER_A_SHIFT;
> > - case TRANSCODER_B:
> > - return EDP_PSR_TRANSCODER_B_SHIFT;
> > - case TRANSCODER_C:
> > - return EDP_PSR_TRANSCODER_C_SHIFT;
> > - default:
> > - MISSING_CASE(cpu_transcoder);
> > - /* fallthrough */
> > - case TRANSCODER_EDP:
> > - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > - }
> > -}
> > -
> > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32
> > debug)
> > {
> > - u32 debug_mask, mask;
> > - enum transcoder cpu_transcoder;
> > - u32 transcoders = BIT(TRANSCODER_EDP);
> > -
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - transcoders |= BIT(TRANSCODER_A) |
> > - BIT(TRANSCODER_B) |
> > - BIT(TRANSCODER_C);
> > -
> > - debug_mask = 0;
> > - mask = 0;
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > - int shift = edp_psr_shift(cpu_transcoder);
> > -
> > - mask |= EDP_PSR_ERROR(shift);
> > - debug_mask |= EDP_PSR_POST_EXIT(shift) |
> > - EDP_PSR_PRE_ENTRY(shift);
> > - }
> > + u32 mask = EDP_PSR_ERROR;
> >
> > if (debug & I915_PSR_DEBUG_IRQ)
> > - mask |= debug_mask;
> > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY;
> >
> >
Re: [Intel-gfx] [PATCH 6/7] drm/i915/psr: Remove partial PSR support on multiple transcoders
On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza wrote:
> PSR is only supported in eDP transcoder and there is only one
> instance of it, so lets drop all of this code.
Is this sentence true? I mean, in the way it is written it
seems like HW doesn't actually support it...
Or should we re-phrase for we are not really enabling support
for other transcoders than eDP and we do not have plans to do
it so soon so let's clean the code...
or something like that?
>
> Cc: Dhinakaran Pandiyan
> Cc: Rodrigo Vivi
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/i915_reg.h | 17 +---
> drivers/gpu/drm/i915/intel_psr.c | 147 ---
> 2 files changed, 42 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c59cfa83dbaf..18e2991b376d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4241,13 +4241,9 @@ enum {
> /* Bspec claims those aren't shifted but stay at 0x64800 */
> #define EDP_PSR_IMR _MMIO(0x64834)
> #define EDP_PSR_IIR _MMIO(0x64838)
> -#define EDP_PSR_ERROR(shift) (1 << ((shift) + 2))
> -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> +#define EDP_PSR_ERROR (1 << 2)
> +#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 EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> @@ -4312,12 +4308,7 @@ enum {
> #define EDP_PSR2_IDLE_FRAME_MASK 0xf
> #define EDP_PSR2_IDLE_FRAME_SHIFT 0
>
> -#define _PSR_EVENT_TRANS_A 0x60848
> -#define _PSR_EVENT_TRANS_B 0x61848
> -#define _PSR_EVENT_TRANS_C 0x62848
> -#define _PSR_EVENT_TRANS_D 0x63848
> -#define _PSR_EVENT_TRANS_EDP 0x6F848
> -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> _PSR_EVENT_TRANS_A)
> +#define PSR_EVENT_MMIO(0x6F848)
> #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)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index bb97c1657493..b984e005b72e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct drm_i915_private
> *dev_priv,
> }
> }
>
> -static int edp_psr_shift(enum transcoder cpu_transcoder)
> -{
> - switch (cpu_transcoder) {
> - case TRANSCODER_A:
> - return EDP_PSR_TRANSCODER_A_SHIFT;
> - case TRANSCODER_B:
> - return EDP_PSR_TRANSCODER_B_SHIFT;
> - case TRANSCODER_C:
> - return EDP_PSR_TRANSCODER_C_SHIFT;
> - default:
> - MISSING_CASE(cpu_transcoder);
> - /* fallthrough */
> - case TRANSCODER_EDP:
> - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> - }
> -}
> -
> void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
> {
> - u32 debug_mask, mask;
> - enum transcoder cpu_transcoder;
> - u32 transcoders = BIT(TRANSCODER_EDP);
> -
> - if (INTEL_GEN(dev_priv) >= 8)
> - transcoders |= BIT(TRANSCODER_A) |
> -BIT(TRANSCODER_B) |
> -BIT(TRANSCODER_C);
> -
> - debug_mask = 0;
> - mask = 0;
> - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> - int shift = edp_psr_shift(cpu_transcoder);
> -
> - mask |= EDP_PSR_ERROR(shift);
> - debug_mask |= EDP_PSR_POST_EXIT(shift) |
> - EDP_PSR_PRE_ENTRY(shift);
> - }
> + u32 mask = EDP_PSR_ERROR;
>
> if (debug & I915_PSR_DEBUG_IRQ)
> - mask |= debug_mask;
> + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY;
>
> I915_WRITE(EDP_PSR_IMR, ~mask);
> }
> @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool psr2_enabled)
>
> void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> {
> - u32 transcoders = BIT(TRANSCODER_EDP);
> - enum transcoder cpu_transcoder;
> - ktime_t time_ns = ktime_get();
> - u32 mask = 0;
> + ktime_t time_ns = ktime_get();
>
> - if (INTEL_GEN(dev_priv) >= 8)
> - transcoders |= BIT(TRANSCODER_A) |
> -
