| Message ID | 1449764904-3229-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-12-10 14:28 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > This bit is also reserved on Skylake. Actually the only > platform that supports this is Haswell, so let's fix > this logic and apply this link entry time only for the > platform that supports it, i.e. Haswell. > > This also changes the style to let more clear platform > differences outside the reg write. We would probably catch > this case sooner if separated, or not... > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 14cc2cf..9ccff30 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) > */ > uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > uint32_t val = 0x0; > - const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > + > + if (IS_HASWELL(dev)) > + val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; It's kinda weird that we used the variable "val" for nothing. At least we're using it now :). <insert bikeshed suggestion about either using it for storing all bits or just kill it> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > I915_WRITE(EDP_PSR_CTL, val | > - (IS_BROADWELL(dev) ? 0 : link_entry_time) | > max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | > idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | > EDP_PSR_ENABLE); > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2015-12-11 at 17:09 -0200, Paulo Zanoni wrote: > 2015-12-10 14:28 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > This bit is also reserved on Skylake. Actually the only > > platform that supports this is Haswell, so let's fix > > this logic and apply this link entry time only for the > > platform that supports it, i.e. Haswell. > > > > This also changes the style to let more clear platform > > differences outside the reg write. We would probably catch > > this case sooner if separated, or not... > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 14cc2cf..9ccff30 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp) > > */ > > uint32_t idle_frames = max(6, dev_priv > > ->vbt.psr.idle_frames); > > uint32_t val = 0x0; > > - const uint32_t link_entry_time = > > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > + > > + if (IS_HASWELL(dev)) > > + val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > It's kinda weird that we used the variable "val" for nothing. At > least > we're using it now :). Yes, indeed... in the past we had some conditional stuff there that we were removing one by one and val end up empty... first version that I was doing here had a patch 1 to remove it and second one with this change here... However I had to add link standby condition back and I decided to keep val. > <insert bikeshed suggestion about either using > it for storing all bits or just kill it> hm.. I like the val to check conditionals and the fixed ones inside the write macro, but I see your point of a standardization... > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> thanks > > > > > I915_WRITE(EDP_PSR_CTL, val | > > - (IS_BROADWELL(dev) ? 0 : link_entry_time) | > > max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | > > idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | > > EDP_PSR_ENABLE); > > -- > > 2.4.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://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 14cc2cf..9ccff30 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) */ uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); uint32_t val = 0x0; - const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; + + if (IS_HASWELL(dev)) + val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; I915_WRITE(EDP_PSR_CTL, val | - (IS_BROADWELL(dev) ? 0 : link_entry_time) | max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | EDP_PSR_ENABLE);
This bit is also reserved on Skylake. Actually the only platform that supports this is Haswell, so let's fix this logic and apply this link entry time only for the platform that supports it, i.e. Haswell. This also changes the style to let more clear platform differences outside the reg write. We would probably catch this case sooner if separated, or not... Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)