Re: [PATCH 1/3] drm/i915: PSR also doesn't have link_entry_time on SKL.

From: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
To: "przanoni@gmail.com" <przanoni@gmail.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/3] drm/i915: PSR also doesn't have link_entry_time on SKL.
Date: Fri, 11 Dec 2015 19:14:08 +0000	[thread overview]
Message-ID: <1449861257.1395.62.camel@intel.com> (raw)
In-Reply-To: <CA+gsUGSOEVtuhWycBm8SjFLmjM3uNvJvXfAAeUcx5z+c1Qibeg@mail.gmail.com>

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
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx