Re: [PATCH v5 3/9] drm/i915/tgl: Add power well to enable DC3CO state

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

On 8/27/2019 6:50 PM, Imre Deak wrote:
> On Tue, Aug 27, 2019 at 06:31:31PM +0530, Gupta, Anshuman wrote:
>>
>>
>> On 8/13/2019 8:16 PM, Imre Deak wrote:
>>> On Sat, Aug 10, 2019 at 12:02:17AM +0530, Anshuman Gupta wrote:
>>>> "DC3CO Off" power well inherits its power domains from
>>>> "DC Off" power well, these power domains will disallow
>>>> DC3CO when any external displays are connected and at
>>>> time of modeset and aux programming.
>>>> Renaming "DC Off" power well to "DC5 Off" power well.
>>>>
>>>> v2: commit log improvement.
>>>> v3: Used intel_wait_for_register to wait for DC3CO exit. [Imre]
>>>>       Used gen9_set_dc_state() to allow/disallow DC3CO. [Imre]
>>>>       Moved transcoder psr2 exit line enablement from tgl_allow_dc3co()
>>>>       to a appropriate place haswell_crtc_enable(). [Imre]
>>>>       Changed the DC3CO power well enabled call back logic as
>>>>       recommended in review comments. [Imre]
>>>> v4: Used wait_for_us() instead of intel_wait_for_reg(). [Imre (IRC)]
>>>> v5: using udelay() instead of waiting for DC3CO exit status.
>>>>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Animesh Manna <animesh.manna@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>> ---
>>>>    .../drm/i915/display/intel_display_power.c    | 69 ++++++++++++++++++-
>>>>    1 file changed, 67 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>>>> index e2ef202aeeef..c9e92d48cdab 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>>>> @@ -791,7 +791,26 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
>>>>    	dev_priv->csr.dc_state = val & mask;
>>>>    }
>>>> -static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>>>> +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
>>>> +}
>>>> +
>>>> +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	val = I915_READ(DC_STATE_EN);
>>>> +	val &= ~DC_STATE_DC3CO_STATUS;
>>>> +	I915_WRITE(DC_STATE_EN, val);
>>>> +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>>>> +	/*
>>>> +	 * Delay of 200us DC3CO Exit time B.Spec 49196
>>>> +	 */
>>>> +	udelay(200);
>>>> +}
>>>> +
>>>> +void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>>>>    {
>>>>    	assert_can_enable_dc9(dev_priv);
>>>> @@ -1007,6 +1026,33 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>>>>    		gen9_enable_dc5(dev_priv);
>>>>    }
>>>> +static void tgl_dc3co_power_well_enable(struct drm_i915_private *dev_priv,
>>>> +					struct i915_power_well *power_well)
>>>
>>> Should be called dc3co_off power well.
>>>
>>>> +{
>>>> +	tgl_disallow_dc3co(dev_priv);
>>>> +}
>>>> +
>>>> +static void tgl_dc3co_power_well_disable(struct drm_i915_private *dev_priv,
>>>> +					 struct i915_power_well *power_well)
>>>> +{
>>>> +	if (!dev_priv->psr.sink_psr2_support)
>>>> +		return;
>>>
>>> We could end up enabling DC3CO while PSR2 is disabled after disabling
>>> a PSR2 capable output, which is against the spec.
>>>
>>> I'm thinking now that we should have a single dc_off power well and a
>>> new interface setting the max allowed DC state (DC3CO, DC5/6).
>> Hi Imre,
>>
>> Could you please comment if below part of new API code set_max_dc_state() is
>> ok? with respect to our DC3CO design sync.
>>
>> static void set_target_dc_state(struct drm_i915_private *dev_priv)
>> {
>>          gen9_dc_off_power_well_disable(dev_priv, NULL);
>> }
>>
>> void set_max_dc_state(struct drm_i915_private *dev_priv, u32 state)
>> {
>>          bool dc_off_enabled;
>>          struct i915_power_domains *power_domains = &dev_priv->power_domains;
>> /* need to define an id for DC off power well "TGL_DISP_DC_OFF"*/
>>          dc_off_enabled = intel_display_power_well_is_enabled(dev_priv,
>> TGL_DISP_DC_OFF);
> 
> dc_off_enabled can get stale this way until you acquire the lock.
> Probably easier to lookup_power_well() and then check its state with the
> lock held.
> 
>>
>>          mutex_lock(&power_domains->lock);
>>          if (dc_off_enabled) {
>>                  dev_priv->csr.max_dc_state = state;
>>          } else {
>>                  dev_priv->csr.max_dc_state = state;
>>                  set_target_dc_state(dev_priv);
>>          }
>>          mutex_unlock(&power_domains->lock);
>> }
> 
>> gen9_dc_off_power_well_disable will enable max_dc_state to DC_STATE_EN
>> register accordingly.
>>
>> There can be only issue according to me here when "DC off" power well
>> is disable (this will happen when we want to set max_dc_state to dc5
>> in dc5_idle_thread). In that case we need to manually call
>> set_target_dc_state()->gen9_dc_off_power_well_disable().
> 
> In that case you could just call power_well->enable(), ->disable(), and
> in the ->disable() hook you'd enable the DC state accorindg to
> csr.max_dc_state?
Thanks for suggestion, will incorporate suggesting approach.
Thanks,
Anshuman
> 
>>
>> Thanks,
>> Anshuman Gupta.
>>>
>>> (Right now I think there is also a missing re-enabling of DC3CO when
>>>    disabling DC5/6).
>>>
>>>> +
>>>> +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
>>>> +		tgl_allow_dc3co(dev_priv);
>>>> +}
>>>> +
>>>> +static bool tgl_dc3co_power_well_enabled(struct drm_i915_private *dev_priv,
>>>> +					 struct i915_power_well *power_well)
>>>> +{
>>>> +	/*
>>>> +	 * Checking alone DC_STATE_EN is not enough as DC5 power well also
>>>> +	 * allow/disallow DC3CO to make sure both are not enabled at same time
>>>> +	 */
>>>> +	return ((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC3CO) == 0 &&
>>>> +		(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0);
>>>> +}
>>>> +
>>>>    static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
>>>>    					 struct i915_power_well *power_well)
>>>>    {
>>>> @@ -2611,6 +2657,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>>    	BIT_ULL(POWER_DOMAIN_TRANSCODER_VDSC_PW2) |	\
>>>>    	BIT_ULL(POWER_DOMAIN_INIT))
>>>> +#define TGL_DISPLAY_DC3CO_OFF_POWER_DOMAINS (		\
>>>> +	TGL_PW_2_POWER_DOMAINS |			\
>>>> +	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>>>> +	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>>>> +	BIT_ULL(POWER_DOMAIN_INIT))
>>>> +
>>>>    #define TGL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
>>>>    	TGL_PW_2_POWER_DOMAINS |			\
>>>>    	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>>>> @@ -2715,6 +2767,13 @@ static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
>>>>    	.is_enabled = gen9_dc_off_power_well_enabled,
>>>>    };
>>>> +static const struct i915_power_well_ops tgl_dc3co_power_well_ops = {
>>>> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>>>> +	.enable = tgl_dc3co_power_well_enable,
>>>> +	.disable = tgl_dc3co_power_well_disable,
>>>> +	.is_enabled = tgl_dc3co_power_well_enabled,
>>>> +};
>>>> +
>>>>    static const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops = {
>>>>    	.sync_hw = i9xx_power_well_sync_hw_noop,
>>>>    	.enable = bxt_dpio_cmn_power_well_enable,
>>>> @@ -3626,11 +3685,17 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
>>>>    		},
>>>>    	},
>>>>    	{
>>>> -		.name = "DC off",
>>>> +		.name = "DC5 off",
>>>>    		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
>>>>    		.ops = &gen9_dc_off_power_well_ops,
>>>>    		.id = DISP_PW_ID_NONE,
>>>>    	},
>>>> +	{
>>>> +		.name = "DC3CO off",
>>>> +		.domains = TGL_DISPLAY_DC3CO_OFF_POWER_DOMAINS,
>>>> +		.ops = &tgl_dc3co_power_well_ops,
>>>> +		.id = DISP_PW_ID_NONE,
>>>> +	},
>>>>    	{
>>>>    		.name = "power well 2",
>>>>    		.domains = TGL_PW_2_POWER_DOMAINS,
>>>> -- 
>>>> 2.21.0
>>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx