| Message ID | 20181005030130.15972-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry" | expand |
On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan wrote: > PSR2 sinks that require Y coordinates for selective update also need the > Y coordinate Valid bit in VSC SDP. > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-12) I couldn't get any meaningful information about Y coordinate valid bit looking at this table... what am I missing? > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 105b7ea2cd98..92672954dfef 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > * good enough. */ > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > - val |= EDP_Y_COORDINATE_ENABLE; > + val |= EDP_Y_COORDINATE_ENABLE | EDP_Y_COORDINATE_VALID; But also, this seems to be doing the opposite what you wrote on the commit message since this bit means: "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713) > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50) > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote: > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan wrote: > > PSR2 sinks that require Y coordinates for selective update also > > need the > > Y coordinate Valid bit in VSC SDP. > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6- > > 12) > > I couldn't get any meaningful information about Y coordinate valid > bit > looking at this table... > > what am I missing? " Y-Coordinate_Valid If the Sink device indicates that Y-coordinate is required, the Source device must program HB2 (VSC Revision Number) to 04h or 05h. Additionally, the Source device must set this bit to 1 to indicate that the Y-coordinate provided in DB12 through DB13 is valid. However, if the Y-coordinate provided in DB12 through DB13 is not valid, this bit must be cleared to 0 (this includes when DB12 through DB13 are not set and remain at a default value of 0000h, because a Y-coordinate value of 0000h represents the first active video line)." We enable PSR2 only when the sink says Y-coordinate is required and the hardware needs to be told to send Y coordinate valid bit in the VSC packets for such sinks. > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 105b7ea2cd98..92672954dfef 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp > > *intel_dp) > > * good enough. */ > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > - val |= EDP_Y_COORDINATE_ENABLE; > > + val |= EDP_Y_COORDINATE_ENABLE | > > EDP_Y_COORDINATE_VALID; > > But also, this seems to be doing the opposite what you wrote on the > commit message since this bit means: > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713) Oops! You are right, the bit name threw me off. > > > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && > > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50) > > -- > > 2.14.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-10-05 at 10:51 -0700, Dhinakaran Pandiyan wrote: > On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote: > > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan > > wrote: > > > PSR2 sinks that require Y coordinates for selective update also > > > need the > > > Y coordinate Valid bit in VSC SDP. > > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6- > > > 12) > > > > I couldn't get any meaningful information about Y coordinate valid > > bit > > looking at this table... > > > > what am I missing? > " > Y-Coordinate_Valid > If the Sink device indicates that Y-coordinate is required, the > Source > device must program HB2 (VSC Revision Number) to 04h or 05h. > Additionally, the Source device must set this bit to 1 to indicate > that > the Y-coordinate provided in DB12 through DB13 is valid. However, if > the Y-coordinate provided in DB12 through DB13 is not valid, this bit > must be cleared to 0 (this includes when DB12 through DB13 are not > set > and remain at a default value of 0000h, because a Y-coordinate value > of > 0000h represents the first active video line)." > > We enable PSR2 only when the sink says Y-coordinate is required and > the > hardware needs to be told to send Y coordinate valid bit in the VSC > packets for such sinks. > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 105b7ea2cd98..92672954dfef 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp > > > *intel_dp) > > > * good enough. */ > > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > - val |= EDP_Y_COORDINATE_ENABLE; > > > + val |= EDP_Y_COORDINATE_ENABLE | > > > EDP_Y_COORDINATE_VALID; > > > > But also, this seems to be doing the opposite what you wrote on the > > commit message since this bit means: > > > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713) > > Oops! You are right, the bit name threw me off. When I added this bit I first named it "EDP_Y_COORDINATE_INVALID" but was decided to go back to BSpec name. > > > > > > > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && > > > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50) > > > -- > > > 2.14.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 05, 2018 at 10:51:28AM -0700, Dhinakaran Pandiyan wrote: > On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote: > > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan wrote: > > > PSR2 sinks that require Y coordinates for selective update also > > > need the > > > Y coordinate Valid bit in VSC SDP. > > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6- > > > 12) > > > > I couldn't get any meaningful information about Y coordinate valid > > bit > > looking at this table... > > > > what am I missing? > " > Y-Coordinate_Valid > If the Sink device indicates that Y-coordinate is required, the Source > device must program HB2 (VSC Revision Number) to 04h or 05h. > Additionally, the Source device must set this bit to 1 to indicate that > the Y-coordinate provided in DB12 through DB13 is valid. However, if > the Y-coordinate provided in DB12 through DB13 is not valid, this bit > must be cleared to 0 (this includes when DB12 through DB13 are not set > and remain at a default value of 0000h, because a Y-coordinate value of > 0000h represents the first active video line)." Interesting... I keep forgetting I need to delete my old spec and download a new one from VESA website... On the one that I have here I couldn't find this text and also HB2 doesn't have these options: 4:0 Revision Number 01h = VSC packet supports only 3D stereo. 02h = 3D stereo + PSR1. 03h = 3D stereo + PSR2. 7:5 RESERVED > > We enable PSR2 only when the sink says Y-coordinate is required and the > hardware needs to be told to send Y coordinate valid bit in the VSC > packets for such sinks. > > > > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 105b7ea2cd98..92672954dfef 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp > > > *intel_dp) > > > * good enough. */ > > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > - val |= EDP_Y_COORDINATE_ENABLE; > > > + val |= EDP_Y_COORDINATE_ENABLE | > > > EDP_Y_COORDINATE_VALID; > > > > But also, this seems to be doing the opposite what you wrote on the > > commit message since this bit means: > > > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713) > > Oops! You are right, the bit name threw me off. yeap... the name is so bad indeed. > > > > > > > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && > > > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50) > > > -- > > > 2.14.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-10-05 at 12:54 -0700, Rodrigo Vivi wrote: > On Fri, Oct 05, 2018 at 10:51:28AM -0700, Dhinakaran Pandiyan wrote: > > On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote: > > > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan > > > wrote: > > > > PSR2 sinks that require Y coordinates for selective update also > > > > need the > > > > Y coordinate Valid bit in VSC SDP. > > > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table > > > > 6- > > > > 12) > > > > > > I couldn't get any meaningful information about Y coordinate > > > valid > > > bit > > > looking at this table... > > > > > > what am I missing? > > > > " > > Y-Coordinate_Valid > > If the Sink device indicates that Y-coordinate is required, the > > Source > > device must program HB2 (VSC Revision Number) to 04h or 05h. > > Additionally, the Source device must set this bit to 1 to indicate > > that > > the Y-coordinate provided in DB12 through DB13 is valid. However, > > if > > the Y-coordinate provided in DB12 through DB13 is not valid, this > > bit > > must be cleared to 0 (this includes when DB12 through DB13 are not > > set > > and remain at a default value of 0000h, because a Y-coordinate > > value of > > 0000h represents the first active video line)." > > Interesting... I keep forgetting I need to delete my old spec and > download > a new one from VESA website... > > On the one that I have here I couldn't find this text and also HB2 > doesn't > have these options: > > 4:0 Revision Number > 01h = VSC packet supports only 3D stereo. > 02h = 3D stereo + PSR1. > 03h = 3D stereo + PSR2. > 7:5 RESERVED This got added in eDP 1.4a. > > > > > We enable PSR2 only when the sink says Y-coordinate is required and > > the > > hardware needs to be told to send Y coordinate valid bit in the VSC > > packets for such sinks. > > > > > > > > > > > > > > > Signed-off-by: Dhinakaran Pandiyan < > > > > dhinakaran.pandiyan@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_psr.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index 105b7ea2cd98..92672954dfef 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct > > > > intel_dp > > > > *intel_dp) > > > > * good enough. */ > > > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > > > if (INTEL_GEN(dev_priv) >= 10 || > > > > IS_GEMINILAKE(dev_priv)) > > > > - val |= EDP_Y_COORDINATE_ENABLE; > > > > + val |= EDP_Y_COORDINATE_ENABLE | > > > > EDP_Y_COORDINATE_VALID; > > > > > > But also, this seems to be doing the opposite what you wrote on > > > the > > > commit message since this bit means: > > > > > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713) > > > > Oops! You are right, the bit name threw me off. > > yeap... the name is so bad indeed. > > > > > > > > > > > > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && > > > > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50) > > > > -- > > > > 2.14.1 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-10-05 at 12:53 -0700, Souza, Jose wrote: > On Fri, 2018-10-05 at 10:51 -0700, Dhinakaran Pandiyan wrote: > > On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote: > > > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan > > > wrote: > > > > PSR2 sinks that require Y coordinates for selective update also > > > > need the > > > > Y coordinate Valid bit in VSC SDP. > > > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table > > > > 6- > > > > 12) > > > > > > I couldn't get any meaningful information about Y coordinate > > > valid > > > bit > > > looking at this table... > > > > > > what am I missing? > > > > " > > Y-Coordinate_Valid > > If the Sink device indicates that Y-coordinate is required, the > > Source > > device must program HB2 (VSC Revision Number) to 04h or 05h. > > Additionally, the Source device must set this bit to 1 to indicate > > that > > the Y-coordinate provided in DB12 through DB13 is valid. However, > > if > > the Y-coordinate provided in DB12 through DB13 is not valid, this > > bit > > must be cleared to 0 (this includes when DB12 through DB13 are not > > set > > and remain at a default value of 0000h, because a Y-coordinate > > value > > of > > 0000h represents the first active video line)." > > > > We enable PSR2 only when the sink says Y-coordinate is required and > > the > > hardware needs to be told to send Y coordinate valid bit in the VSC > > packets for such sinks. > > > > > > > > Signed-off-by: Dhinakaran Pandiyan < > > > > dhinakaran.pandiyan@intel.com > > > > > > > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_psr.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index 105b7ea2cd98..92672954dfef 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct > > > > intel_dp > > > > *intel_dp) > > > > * good enough. */ > > > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > > > if (INTEL_GEN(dev_priv) >= 10 || > > > > IS_GEMINILAKE(dev_priv)) > > > > - val |= EDP_Y_COORDINATE_ENABLE; > > > > + val |= EDP_Y_COORDINATE_ENABLE | > > > > EDP_Y_COORDINATE_VALID; > > > > > > But also, this seems to be doing the opposite what you wrote on > > > the > > > commit message since this bit means: > > > > > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713) > > > > Oops! You are right, the bit name threw me off. > > When I added this bit I first named it "EDP_Y_COORDINATE_INVALID" but > was decided to go back to BSpec name. I remember it now, something like EDP_Y_COORDINATE_VALID_DISABLE would be more appropriate. -DK
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 105b7ea2cd98..92672954dfef 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) * good enough. */ val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) - val |= EDP_Y_COORDINATE_ENABLE; + val |= EDP_Y_COORDINATE_ENABLE | EDP_Y_COORDINATE_VALID; if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
PSR2 sinks that require Y coordinates for selective update also need the Y coordinate Valid bit in VSC SDP. Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-12) Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)