Re: [Intel-gfx] [PATCH v5 5/9] drm/i915/tgl: Add helper function to prefer dc3co over dc5
On Mon, Aug 19, 2019 at 06:27:48PM +0530, Gupta, Anshuman wrote:
>
>
> On 8/13/2019 9:17 PM, Imre Deak wrote:
> > On Sat, Aug 10, 2019 at 12:02:19AM +0530, Anshuman Gupta wrote:
> > > We need to have a S/W flag based upon which driver can switch to DC3CO.
> > > If it is only edp display connected and it has psr2 capability,
> > > then set a prefer_dc3co flag to true, which will be used to
> > > switch to dc3co as well as to program DC3CO PSR2 transcoder
> > > early exitline event.
> > >
> > > Cc: Jani Nikula
> > > Cc: Imre Deak
> > > Cc: Animesh Manna
> > > Signed-off-by: Anshuman Gupta
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 5 +
> > > .../drm/i915/display/intel_display_power.c| 105 ++
> > > .../drm/i915/display/intel_display_power.h| 5 +
> > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > > drivers/gpu/drm/i915/intel_pm.h | 2 +
> > > 6 files changed, 119 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 647f49ca86ff..1ec204c14a10 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6448,6 +6448,9 @@ static void haswell_crtc_enable(struct
> > > intel_crtc_state *pipe_config,
> > > if (WARN_ON(intel_crtc->active))
> > > return;
> > > + /* Enable PSR2 transcoder exit line */
> > > + if (pipe_config->has_psr2 && dev_priv->csr.prefer_dc3co)
> > > + tgl_enable_psr2_transcoder_exitline(pipe_config);
> >
> > This is part of PSR2 programming, so should be done somewhere below
> > intel_psr_enable() imo.
> >
> > > intel_encoders_pre_pll_enable(intel_crtc, pipe_config, state);
> > > @@ -13685,6 +13688,8 @@ static int intel_atomic_check(struct drm_device
> > > *dev,
> > > "[modeset]" : "[fastset]");
> > > }
> > > + tgl_prefer_dc3co_over_dc5_check(dev_priv, state);
> >
> > I think this belongs to intel_modeset_checks(), where we could also do
> > all necessary pipe locking (since now I can't see how we would prevent
> > allowing DC3CO when an external output is enabled asynchronously). This
> > would be akin to CDCLK rate change, but I defer on this to Ville.
> Hi Ville,
> Could you please provide your inputs on DC3CO design perspective,
> whether we should have pipe locking in order to maintain consistent state
> for dc3co.
>
> I feel there can be two way to maintain consistent state.
>
> 1. Ensure necessary locking on current crtc before calculating prefer_dc3co
> state, this will block the asynchronous commit.
>
> 2. We can make the prefer_dc3co state to false from enable powerwell
> callback which will trigger by getting a reference count for PG2 power
> domains if any external output is enabled.
>
> (DC3CO will be disallowed from power well enable callback if any external
> output is enabled akin to DC5/DC6, here we want to make prefer_dc3co atomic
> state consistent).
Actually the latest idea based on further discussions was: since all DC
states will be disabled if there is an external output enabled, we
wouldn't need to compute prefer_dc3co. We could just enable/disable
DC3CO (with a new enable_dc3co() API) whenever enabling/disabling PSR2
and during a page-flip/from the page-idle work. That together with the
current state of the dc_off power well determines if any of DC5/6 or
DC3co is enabled atm.
>
> Thanks ,
> Anshuman Gupta.
> >
> > > +
> > > return 0;
> > >fail:
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 167839060154..04a02c88ff93 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -18,6 +18,7 @@
> > > #include "intel_hotplug.h"
> > > #include "intel_sideband.h"
> > > #include "intel_tc.h"
> > > +#include "intel_pm.h"
> > > bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > >enum i915_power_well_id
> > > power_well_id);
> > > @@ -791,6 +792,110 @@ static void gen9_set_dc_state(struct
> > > drm_i915_private *dev_priv, u32 state)
> > > dev_priv->csr.dc_state = val & mask;
> > > }
> > > +void tgl_enable_psr2_transcoder_exitline(struct intel_crtc_state
> > > *cstate)
> > > +{
> > > + u32 linetime_us, val, exit_scanlines;
> > > + u32 crtc_vdisplay = cstate->base.adjusted_mode.crtc_vdisplay;
> > > + struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> > > +
> > > + if (WARN_ON(cstate->cpu_transcoder != TRANSCODER_A))
> > > + return;
> >
> > Where's the TRANSCODER-A restriction coming from?
> >
> > > +
> > > + linetime_us =
Re: [Intel-gfx] [PATCH v5 5/9] drm/i915/tgl: Add helper function to prefer dc3co over dc5
On 8/13/2019 9:17 PM, Imre Deak wrote:
On Sat, Aug 10, 2019 at 12:02:19AM +0530, Anshuman Gupta wrote:
We need to have a S/W flag based upon which driver can switch to DC3CO.
If it is only edp display connected and it has psr2 capability,
then set a prefer_dc3co flag to true, which will be used to
switch to dc3co as well as to program DC3CO PSR2 transcoder
early exitline event.
Cc: Jani Nikula
Cc: Imre Deak
Cc: Animesh Manna
Signed-off-by: Anshuman Gupta
---
drivers/gpu/drm/i915/display/intel_display.c | 5 +
.../drm/i915/display/intel_display_power.c| 105 ++
.../drm/i915/display/intel_display_power.h| 5 +
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 2 +-
drivers/gpu/drm/i915/intel_pm.h | 2 +
6 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 647f49ca86ff..1ec204c14a10 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6448,6 +6448,9 @@ static void haswell_crtc_enable(struct intel_crtc_state
*pipe_config,
if (WARN_ON(intel_crtc->active))
return;
+ /* Enable PSR2 transcoder exit line */
+ if (pipe_config->has_psr2 && dev_priv->csr.prefer_dc3co)
+ tgl_enable_psr2_transcoder_exitline(pipe_config);
This is part of PSR2 programming, so should be done somewhere below
intel_psr_enable() imo.
intel_encoders_pre_pll_enable(intel_crtc, pipe_config, state);
@@ -13685,6 +13688,8 @@ static int intel_atomic_check(struct drm_device *dev,
"[modeset]" : "[fastset]");
}
+ tgl_prefer_dc3co_over_dc5_check(dev_priv, state);
I think this belongs to intel_modeset_checks(), where we could also do
all necessary pipe locking (since now I can't see how we would prevent
allowing DC3CO when an external output is enabled asynchronously). This
would be akin to CDCLK rate change, but I defer on this to Ville.
Hi Ville,
Could you please provide your inputs on DC3CO design perspective,
whether we should have pipe locking in order to maintain consistent
state for dc3co.
I feel there can be two way to maintain consistent state.
1. Ensure necessary locking on current crtc before calculating
prefer_dc3co state, this will block the asynchronous commit.
2. We can make the prefer_dc3co state to false from enable powerwell
callback which will trigger by getting a reference count for PG2 power
domains if any external output is enabled.
(DC3CO will be disallowed from power well enable callback if any
external output is enabled akin to DC5/DC6, here we want to make
prefer_dc3co atomic state consistent).
Thanks ,
Anshuman Gupta.
+
return 0;
fail:
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
b/drivers/gpu/drm/i915/display/intel_display_power.c
index 167839060154..04a02c88ff93 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -18,6 +18,7 @@
#include "intel_hotplug.h"
#include "intel_sideband.h"
#include "intel_tc.h"
+#include "intel_pm.h"
bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
enum i915_power_well_id power_well_id);
@@ -791,6 +792,110 @@ static void gen9_set_dc_state(struct drm_i915_private
*dev_priv, u32 state)
dev_priv->csr.dc_state = val & mask;
}
+void tgl_enable_psr2_transcoder_exitline(struct intel_crtc_state *cstate)
+{
+ u32 linetime_us, val, exit_scanlines;
+ u32 crtc_vdisplay = cstate->base.adjusted_mode.crtc_vdisplay;
+ struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+
+ if (WARN_ON(cstate->cpu_transcoder != TRANSCODER_A))
+ return;
Where's the TRANSCODER-A restriction coming from?
+
+ linetime_us = fixed16_to_u32_round_up(intel_get_linetime_us(cstate));
+ if (WARN_ON(!linetime_us))
+ return;
+ /*
+* DC3CO Exit time 200us B.Spec 49196
+* PSR2 transcoder Early Exit scanlines = ROUNDUP(200 / line time) + 1
+* Exit line event need to program above calculated scan lines before
+* next VBLANK.
+*/
+ exit_scanlines = DIV_ROUND_UP(200, linetime_us) + 1;
+ if (WARN_ON(exit_scanlines > crtc_vdisplay))
+ return;
+
+ exit_scanlines = crtc_vdisplay - exit_scanlines;
+ exit_scanlines <<= EXITLINE_SHIFT;
+ val = I915_READ(EXITLINE(cstate->cpu_transcoder));
+ val &= ~(EXITLINE_MASK | EXITLINE_ENABLE);
+ val |= exit_scanlines;
+ val |= EXITLINE_ENABLE;
+ I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
+}
+
+static bool tgl_is_only_edp_connected(struct intel_crtc_state *crtc_state)
+{
+ struct drm_atomic_state
Re: [Intel-gfx] [PATCH v5 5/9] drm/i915/tgl: Add helper function to prefer dc3co over dc5
On Sat, Aug 10, 2019 at 12:02:19AM +0530, Anshuman Gupta wrote:
> We need to have a S/W flag based upon which driver can switch to DC3CO.
> If it is only edp display connected and it has psr2 capability,
> then set a prefer_dc3co flag to true, which will be used to
> switch to dc3co as well as to program DC3CO PSR2 transcoder
> early exitline event.
>
> Cc: Jani Nikula
> Cc: Imre Deak
> Cc: Animesh Manna
> Signed-off-by: Anshuman Gupta
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 5 +
> .../drm/i915/display/intel_display_power.c| 105 ++
> .../drm/i915/display/intel_display_power.h| 5 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.h | 2 +
> 6 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 647f49ca86ff..1ec204c14a10 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6448,6 +6448,9 @@ static void haswell_crtc_enable(struct intel_crtc_state
> *pipe_config,
>
> if (WARN_ON(intel_crtc->active))
> return;
> + /* Enable PSR2 transcoder exit line */
> + if (pipe_config->has_psr2 && dev_priv->csr.prefer_dc3co)
> + tgl_enable_psr2_transcoder_exitline(pipe_config);
>
> intel_encoders_pre_pll_enable(intel_crtc, pipe_config, state);
>
> @@ -13685,6 +13688,8 @@ static int intel_atomic_check(struct drm_device *dev,
> "[modeset]" : "[fastset]");
> }
>
> + tgl_prefer_dc3co_over_dc5_check(dev_priv, state);
> +
> return 0;
>
> fail:
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 167839060154..04a02c88ff93 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -18,6 +18,7 @@
> #include "intel_hotplug.h"
> #include "intel_sideband.h"
> #include "intel_tc.h"
> +#include "intel_pm.h"
>
> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
>enum i915_power_well_id power_well_id);
> @@ -791,6 +792,110 @@ static void gen9_set_dc_state(struct drm_i915_private
> *dev_priv, u32 state)
> dev_priv->csr.dc_state = val & mask;
> }
>
> +void tgl_enable_psr2_transcoder_exitline(struct intel_crtc_state *cstate)
> +{
> + u32 linetime_us, val, exit_scanlines;
> + u32 crtc_vdisplay = cstate->base.adjusted_mode.crtc_vdisplay;
> + struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +
> + if (WARN_ON(cstate->cpu_transcoder != TRANSCODER_A))
> + return;
> +
> + linetime_us = fixed16_to_u32_round_up(intel_get_linetime_us(cstate));
> + if (WARN_ON(!linetime_us))
> + return;
> + /*
> + * DC3CO Exit time 200us B.Spec 49196
> + * PSR2 transcoder Early Exit scanlines = ROUNDUP(200 / line time) + 1
> + * Exit line event need to program above calculated scan lines before
> + * next VBLANK.
> + */
> + exit_scanlines = DIV_ROUND_UP(200, linetime_us) + 1;
> + if (WARN_ON(exit_scanlines > crtc_vdisplay))
> + return;
> +
> + exit_scanlines = crtc_vdisplay - exit_scanlines;
> + exit_scanlines <<= EXITLINE_SHIFT;
> + val = I915_READ(EXITLINE(cstate->cpu_transcoder));
> + val &= ~(EXITLINE_MASK | EXITLINE_ENABLE);
> + val |= exit_scanlines;
> + val |= EXITLINE_ENABLE;
> + I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> +}
> +
> +static bool tgl_is_only_edp_connected(struct intel_crtc_state *crtc_state)
> +{
> + struct drm_atomic_state *state = crtc_state->base.state;
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + struct drm_connector *connector, *edp_connector = NULL;
> + struct drm_connector_state *connector_state;
> + int i;
> +
> + for_each_new_connector_in_state(state, connector, connector_state, i) {
> + if (connector_state->crtc != >base)
> + continue;
> +
> + if (connector->status == connector_status_connected &&
> + connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> + return false;
> + else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> + connector->status == connector_status_connected)
> + edp_connector = connector;
> + }
> +
> + if (edp_connector)
> + return true;
> +
> + return false;
> +}
> +
> +/*
> + * tgl_prefer_dc3co_over_dc5_check check whether it is worth to choose
> + * DC3CO over DC5. Currently it just check crtc psr2 capebilty and only
> + * edp display should be connected.
> + *
Re: [Intel-gfx] [PATCH v5 5/9] drm/i915/tgl: Add helper function to prefer dc3co over dc5
On Sat, Aug 10, 2019 at 12:02:19AM +0530, Anshuman Gupta wrote:
> We need to have a S/W flag based upon which driver can switch to DC3CO.
> If it is only edp display connected and it has psr2 capability,
> then set a prefer_dc3co flag to true, which will be used to
> switch to dc3co as well as to program DC3CO PSR2 transcoder
> early exitline event.
>
> Cc: Jani Nikula
> Cc: Imre Deak
> Cc: Animesh Manna
> Signed-off-by: Anshuman Gupta
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 5 +
> .../drm/i915/display/intel_display_power.c| 105 ++
> .../drm/i915/display/intel_display_power.h| 5 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.h | 2 +
> 6 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 647f49ca86ff..1ec204c14a10 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6448,6 +6448,9 @@ static void haswell_crtc_enable(struct intel_crtc_state
> *pipe_config,
>
> if (WARN_ON(intel_crtc->active))
> return;
> + /* Enable PSR2 transcoder exit line */
> + if (pipe_config->has_psr2 && dev_priv->csr.prefer_dc3co)
> + tgl_enable_psr2_transcoder_exitline(pipe_config);
This is part of PSR2 programming, so should be done somewhere below
intel_psr_enable() imo.
>
> intel_encoders_pre_pll_enable(intel_crtc, pipe_config, state);
>
> @@ -13685,6 +13688,8 @@ static int intel_atomic_check(struct drm_device *dev,
> "[modeset]" : "[fastset]");
> }
>
> + tgl_prefer_dc3co_over_dc5_check(dev_priv, state);
I think this belongs to intel_modeset_checks(), where we could also do
all necessary pipe locking (since now I can't see how we would prevent
allowing DC3CO when an external output is enabled asynchronously). This
would be akin to CDCLK rate change, but I defer on this to Ville.
> +
> return 0;
>
> fail:
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 167839060154..04a02c88ff93 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -18,6 +18,7 @@
> #include "intel_hotplug.h"
> #include "intel_sideband.h"
> #include "intel_tc.h"
> +#include "intel_pm.h"
>
> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
>enum i915_power_well_id power_well_id);
> @@ -791,6 +792,110 @@ static void gen9_set_dc_state(struct drm_i915_private
> *dev_priv, u32 state)
> dev_priv->csr.dc_state = val & mask;
> }
>
> +void tgl_enable_psr2_transcoder_exitline(struct intel_crtc_state *cstate)
> +{
> + u32 linetime_us, val, exit_scanlines;
> + u32 crtc_vdisplay = cstate->base.adjusted_mode.crtc_vdisplay;
> + struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +
> + if (WARN_ON(cstate->cpu_transcoder != TRANSCODER_A))
> + return;
Where's the TRANSCODER-A restriction coming from?
> +
> + linetime_us = fixed16_to_u32_round_up(intel_get_linetime_us(cstate));
> + if (WARN_ON(!linetime_us))
> + return;
> + /*
> + * DC3CO Exit time 200us B.Spec 49196
> + * PSR2 transcoder Early Exit scanlines = ROUNDUP(200 / line time) + 1
> + * Exit line event need to program above calculated scan lines before
> + * next VBLANK.
> + */
> + exit_scanlines = DIV_ROUND_UP(200, linetime_us) + 1;
> + if (WARN_ON(exit_scanlines > crtc_vdisplay))
> + return;
> +
> + exit_scanlines = crtc_vdisplay - exit_scanlines;
> + exit_scanlines <<= EXITLINE_SHIFT;
> + val = I915_READ(EXITLINE(cstate->cpu_transcoder));
> + val &= ~(EXITLINE_MASK | EXITLINE_ENABLE);
> + val |= exit_scanlines;
> + val |= EXITLINE_ENABLE;
> + I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> +}
> +
> +static bool tgl_is_only_edp_connected(struct intel_crtc_state *crtc_state)
> +{
> + struct drm_atomic_state *state = crtc_state->base.state;
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + struct drm_connector *connector, *edp_connector = NULL;
> + struct drm_connector_state *connector_state;
> + int i;
> +
> + for_each_new_connector_in_state(state, connector, connector_state, i) {
> + if (connector_state->crtc != >base)
> + continue;
> +
> + if (connector->status == connector_status_connected &&
> + connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> + return false;
> + else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> +
