| Message ID | 20180614203433.5602-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza wrote: > Specification requires that max time should be masked from bdw and > forward but it can be also safely enabled to hsw. > This will make PSR exits more deterministic and only when really > needed. If this was used to fix a issue in some panel than can > only self-refresh for a few seconds, that panel will interrupt > and assert one of the PSR errors handled in: > 'drm/i915/psr: Handle PSR RFB storage error' and > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink' > > Also fixing style here. I don't believe that is a style error. So I believe only the MAX_SLEEP one should be added following same style. > > Spec: 21664 > > v4: > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the > static frame on the sink side' to avoid touch in 2 patches > EDP_PSR_DEBUG. I thought this change here depended on having CRC check enabled so it should come first and only bdw+... Isn't this the case? > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index fd240e45f341..177cd57b1029 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > * on frontbuffer tracking. > */ > I915_WRITE(EDP_PSR_DEBUG, > - EDP_PSR_DEBUG_MASK_MEMUP | > - EDP_PSR_DEBUG_MASK_HPD | > - EDP_PSR_DEBUG_MASK_LPSP | > - EDP_PSR_DEBUG_MASK_DISP_REG_WRITE); > + EDP_PSR_DEBUG_MASK_MEMUP | > + EDP_PSR_DEBUG_MASK_HPD | > + EDP_PSR_DEBUG_MASK_LPSP | > + EDP_PSR_DEBUG_MASK_DISP_REG_WRITE | > + EDP_PSR_DEBUG_MASK_MAX_SLEEP); > } > } > > -- > 2.17.1 >
On Thu, Jun 14, 2018 at 02:50:38PM -0700, Dhinakaran Pandiyan wrote: > On Thu, 2018-06-14 at 14:13 -0700, Rodrigo Vivi wrote: > > On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza > > wrote: > > > > > > Specification requires that max time should be masked from bdw and > > > forward but it can be also safely enabled to hsw. > > > This will make PSR exits more deterministic and only when really > > > needed. If this was used to fix a issue in some panel than can > > > only self-refresh for a few seconds, that panel will interrupt > > > and assert one of the PSR errors handled in: > > > 'drm/i915/psr: Handle PSR RFB storage error' and > > > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink' > > > > > > Also fixing style here. > > I don't believe that is a style error. So I believe only > > the MAX_SLEEP one should be added following same style. > > > > > > > > > > > Spec: 21664 > > > > > > v4: > > > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the > > > static frame on the sink side' to avoid touch in 2 patches > > > EDP_PSR_DEBUG. > > I thought this change here depended on having CRC check enabled > > so it should come first and only bdw+... > > Isn't this the case? > > The dependency is the other way around, sending CRC's requires timeout > to be disabled. Since disabling timeouts results in a predictable > behavior, we can do it for all platforms. ah, cool. It makes sense then.... > > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index fd240e45f341..177cd57b1029 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct > > > intel_dp *intel_dp, > > > * on frontbuffer tracking. > > > */ > > > I915_WRITE(EDP_PSR_DEBUG, > > > - EDP_PSR_DEBUG_MASK_MEMUP | > > > - EDP_PSR_DEBUG_MASK_HPD | > > > - EDP_PSR_DEBUG_MASK_LPSP | > > > - EDP_PSR_DEBUG_MASK_DISP_REG_WRITE); > > > + EDP_PSR_DEBUG_MASK_MEMUP | > > > + EDP_PSR_DEBUG_MASK_HPD | > > > + EDP_PSR_DEBUG_MASK_LPSP | > > > + EDP_PSR_DEBUG_MASK_DISP_REG_WRITE | > > > + EDP_PSR_DEBUG_MASK_MAX_SLEEP); > > > } > > > } > > >
On Thu, 2018-06-14 at 14:13 -0700, Rodrigo Vivi wrote: > On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza > wrote: > > > > Specification requires that max time should be masked from bdw and > > forward but it can be also safely enabled to hsw. > > This will make PSR exits more deterministic and only when really > > needed. If this was used to fix a issue in some panel than can > > only self-refresh for a few seconds, that panel will interrupt > > and assert one of the PSR errors handled in: > > 'drm/i915/psr: Handle PSR RFB storage error' and > > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink' > > > > Also fixing style here. > I don't believe that is a style error. So I believe only > the MAX_SLEEP one should be added following same style. > > > > > > > Spec: 21664 > > > > v4: > > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the > > static frame on the sink side' to avoid touch in 2 patches > > EDP_PSR_DEBUG. > I thought this change here depended on having CRC check enabled > so it should come first and only bdw+... > Isn't this the case? The dependency is the other way around, sending CRC's requires timeout to be disabled. Since disabling timeouts results in a predictable behavior, we can do it for all platforms. > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index fd240e45f341..177cd57b1029 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp, > > * on frontbuffer tracking. > > */ > > I915_WRITE(EDP_PSR_DEBUG, > > - EDP_PSR_DEBUG_MASK_MEMUP | > > - EDP_PSR_DEBUG_MASK_HPD | > > - EDP_PSR_DEBUG_MASK_LPSP | > > - EDP_PSR_DEBUG_MASK_DISP_REG_WRITE); > > + EDP_PSR_DEBUG_MASK_MEMUP | > > + EDP_PSR_DEBUG_MASK_HPD | > > + EDP_PSR_DEBUG_MASK_LPSP | > > + EDP_PSR_DEBUG_MASK_DISP_REG_WRITE | > > + EDP_PSR_DEBUG_MASK_MAX_SLEEP); > > } > > } > >
On Thu, 2018-06-14 at 14:50 -0700, Dhinakaran Pandiyan wrote: > On Thu, 2018-06-14 at 14:13 -0700, Rodrigo Vivi wrote: > > > > On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza > > wrote: > > > > > > > > > Specification requires that max time should be masked from bdw > > > and > > > forward but it can be also safely enabled to hsw. > > > This will make PSR exits more deterministic and only when really > > > needed. If this was used to fix a issue in some panel than can > > > only self-refresh for a few seconds, that panel will interrupt > > > and assert one of the PSR errors handled in: > > > 'drm/i915/psr: Handle PSR RFB storage error' and > > > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink' > > > > > > Also fixing style here. > > I don't believe that is a style error. So I believe only > > the MAX_SLEEP one should be added following same style. > > > > > > > > > > > > > > Spec: 21664 > > > > > > v4: > > > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the > > > static frame on the sink side' to avoid touch in 2 patches > > > EDP_PSR_DEBUG. > > I thought this change here depended on having CRC check enabled > > so it should come first and only bdw+... > > Isn't this the case? > The dependency is the other way around, sending CRC's requires > timeout > to be disabled. Since disabling timeouts results in a predictable > behavior, we can do it for all platforms. > > > > > > > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index fd240e45f341..177cd57b1029 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct > > > intel_dp *intel_dp, > > > * on frontbuffer tracking. > > > */ > > > I915_WRITE(EDP_PSR_DEBUG, > > > - EDP_PSR_DEBUG_MASK_MEMUP | > > > - EDP_PSR_DEBUG_MASK_HPD | > > > - EDP_PSR_DEBUG_MASK_LPSP | > > > - EDP_PSR_DEBUG_MASK_DISP_REG_WRITE); > > > + EDP_PSR_DEBUG_MASK_MEMUP | > > > + EDP_PSR_DEBUG_MASK_HPD | > > > + EDP_PSR_DEBUG_MASK_LPSP | > > > + EDP_PSR_DEBUG_MASK_DISP_REG_WRITE | > > > + EDP_PSR_DEBUG_MASK_MAX_SLEEP); With indentation reverted to how it was earlier, Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > } > > > } > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index fd240e45f341..177cd57b1029 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, * on frontbuffer tracking. */ I915_WRITE(EDP_PSR_DEBUG, - EDP_PSR_DEBUG_MASK_MEMUP | - EDP_PSR_DEBUG_MASK_HPD | - EDP_PSR_DEBUG_MASK_LPSP | - EDP_PSR_DEBUG_MASK_DISP_REG_WRITE); + EDP_PSR_DEBUG_MASK_MEMUP | + EDP_PSR_DEBUG_MASK_HPD | + EDP_PSR_DEBUG_MASK_LPSP | + EDP_PSR_DEBUG_MASK_DISP_REG_WRITE | + EDP_PSR_DEBUG_MASK_MAX_SLEEP); } }
Specification requires that max time should be masked from bdw and forward but it can be also safely enabled to hsw. This will make PSR exits more deterministic and only when really needed. If this was used to fix a issue in some panel than can only self-refresh for a few seconds, that panel will interrupt and assert one of the PSR errors handled in: 'drm/i915/psr: Handle PSR RFB storage error' and 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink' Also fixing style here. Spec: 21664 v4: patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side' to avoid touch in 2 patches EDP_PSR_DEBUG. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)