drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

[Shobhit Kumar]

In Haswell while disabling a pipe, we need to disable the DDI control as
well along with the PIPECONF. Otherwise we will hit assertions during crtc
disable

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0161d94..c69671d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1645,6 +1645,13 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val & ~PIPECONF_ENABLE);
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
+
+	/* On HSW DDI Pipe control has to be disabled as well */
+	if (IS_HASWELL(dev_priv->dev)) {
+		val = I915_READ(DDI_FUNC_CTL(pipe));
+		val = val  & (~PIPE_DDI_FUNC_ENABLE);
+		I915_WRITE(DDI_FUNC_CTL(pipe), val);
+	}
 }
 
 /*
-- 
1.7.7.6

Re: drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

[Daniel Vetter]

On Fri, Jun 08, 2012 at 11:44:23AM +0530, Shobhit Kumar wrote:
> In Haswell while disabling a pipe, we need to disable the DDI control as
> well along with the PIPECONF. Otherwise we will hit assertions during crtc
> disable

Hm, can you add such an example assert with backtrace please? All these
asserts encode our current understanding of the hw depency chain, so I'd
like to check whether we're really doing the right thing and don't just
stfu some dmesg noise.

Thanks, Daniel

> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0161d94..c69671d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1645,6 +1645,13 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
>  
>  	I915_WRITE(reg, val & ~PIPECONF_ENABLE);
>  	intel_wait_for_pipe_off(dev_priv->dev, pipe);
> +
> +	/* On HSW DDI Pipe control has to be disabled as well */
> +	if (IS_HASWELL(dev_priv->dev)) {
> +		val = I915_READ(DDI_FUNC_CTL(pipe));
> +		val = val  & (~PIPE_DDI_FUNC_ENABLE);
> +		I915_WRITE(DDI_FUNC_CTL(pipe), val);
> +	}
>  }
>  
>  /*
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

[Eugeni Dodonov]

On 06/08/2012 09:49 AM, Daniel Vetter wrote:
> On Fri, Jun 08, 2012 at 11:44:23AM +0530, Shobhit Kumar wrote:
>> In Haswell while disabling a pipe, we need to disable the DDI control as
>> well along with the PIPECONF. Otherwise we will hit assertions during crtc
>> disable
>
> Hm, can you add such an example assert with backtrace please? All these
> asserts encode our current understanding of the hw depency chain, so I'd
> like to check whether we're really doing the right thing and don't just
> stfu some dmesg noise.
>
> Thanks, Daniel

This is part of the pipe disabling process starting with Haswell. DDI 
pipe function control should be disabled when pipe is being disabled, 
otherwise it stays in enabled state and on next enabling we hit the 
assert within assert_fdi_tx:

...
         if (IS_HASWELL(dev_priv->dev)) {
                 /* On Haswell, DDI is used instead of FDI_TX_CTL */
                 reg = DDI_FUNC_CTL(pipe);
                 val = I915_READ(reg);
                 cur_state = !!(val & PIPE_DDI_FUNC_ENABLE);
...

I missed this in the initial enabling and Shobhit has catched this - thanks!

So you can also add:
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

For this patch.

Eugeni

Re: drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

[Shobhit Kumar]

On 06/08/2012 07:20 PM, Eugeni Dodonov wrote:
> On 06/08/2012 09:49 AM, Daniel Vetter wrote:
>> On Fri, Jun 08, 2012 at 11:44:23AM +0530, Shobhit Kumar wrote:
>>> In Haswell while disabling a pipe, we need to disable the DDI control as
>>> well along with the PIPECONF. Otherwise we will hit assertions during
>>> crtc
>>> disable
>>
>> Hm, can you add such an example assert with backtrace please? All these
>> asserts encode our current understanding of the hw depency chain, so I'd
>> like to check whether we're really doing the right thing and don't just
>> stfu some dmesg noise.
>>
>> Thanks, Daniel
>
> This is part of the pipe disabling process starting with Haswell. DDI
> pipe function control should be disabled when pipe is being disabled,
> otherwise it stays in enabled state and on next enabling we hit the
> assert within assert_fdi_tx:
>
> ...
> if (IS_HASWELL(dev_priv->dev)) {
> /* On Haswell, DDI is used instead of FDI_TX_CTL */
> reg = DDI_FUNC_CTL(pipe);
> val = I915_READ(reg);
> cur_state = !!(val & PIPE_DDI_FUNC_ENABLE);
> ...
>
Eugeni already explained how and where the assertion will be raised. 
Please find a sample assertion while loading the driver with HDMI output 
connected -

[   19.172852] WARNING: at drivers/gpu/drm/i915/intel_display.c:976 
assert_fdi_tx+0x87/0x90 [i915]()
[   19.172901] Hardware name: Shark Bay Client platform
[   19.172941] FDI TX state assertion failure (expected off, current on)
[   19.172986] Modules linked in: i915(+) drm_kms_helper
[   19.173085] Pid: 1072, comm: modprobe Not tainted 3.4.0+ #26
[   19.173185] Call Trace:
[   19.173282]  [<ffffffff81035dfa>] warn_slowpath_common+0x7a/0xb0
[   19.173384]  [<ffffffff81035ed1>] warn_slowpath_fmt+0x41/0x50
[   19.173496]  [<ffffffffa002bfe7>] assert_fdi_tx+0x87/0x90 [i915]
[   19.173616]  [<ffffffffa0030ed6>] ironlake_crtc_disable+0x176/0x800 
[i915]
[   19.173726]  [<ffffffffa00315e6>] ironlake_crtc_dpms+0x86/0x90 [i915]
[   19.173835]  [<ffffffffa002cf33>] intel_crtc_dpms+0x43/0x140 [i915]
[   19.173943]  [<ffffffffa0034b10>] intel_crtc_disable+0x30/0xa0 [i915]
[   19.174046]  [<ffffffffa0002595>] 
drm_helper_disable_unused_functions+0x115/0x190 [drm_kms_helper]
[   19.174220]  [<ffffffffa0036b37>] intel_modeset_init+0x677/0xe30 [i915]
[   19.174332]  [<ffffffffa000fa26>] i915_driver_load+0xa36/0xad0 [i915]
[   19.174436]  [<ffffffff813650e3>] ? drm_get_minor+0x263/0x310
[   19.174538]  [<ffffffff813673f9>] drm_get_pci_dev+0x189/0x2b0
[   19.174655]  [<ffffffffa0058443>] i915_pci_probe+0x1b/0x1d [i915]
[   19.174757]  [<ffffffff812d435a>] pci_device_probe+0x12a/0x130
[   19.174859]  [<ffffffff8137e413>] driver_probe_device+0x73/0x210
[   19.174960]  [<ffffffff8137e64b>] __driver_attach+0x9b/0xa0
[   19.175060]  [<ffffffff8137e5b0>] ? driver_probe_device+0x210/0x210
[   19.175162]  [<ffffffff8137c97e>] bus_for_each_dev+0x4e/0x80
[   19.175262]  [<ffffffff8137df89>] driver_attach+0x19/0x20
[   19.175362]  [<ffffffff8137db80>] bus_add_driver+0x1a0/0x270
[   19.175463]  [<ffffffff8137eb51>] driver_register+0x71/0x130
[   19.175568]  [<ffffffff812d4040>] __pci_register_driver+0x50/0xc0
[   19.175671]  [<ffffffff816e966d>] ? notifier_call_chain+0x4d/0x70
[   19.175772]  [<ffffffff81367635>] drm_pci_init+0x115/0x130
[   19.175872]  [<ffffffffa007e000>] ? 0xffffffffa007dfff
[   19.175978]  [<ffffffffa007e08b>] i915_init+0x8b/0x8d [i915]
[   19.176079]  [<ffffffff810001ba>] do_one_initcall+0x3a/0x160
[   19.176181]  [<ffffffff8108a91c>] sys_init_module+0xa1c/0x1ba0
[   19.176283]  [<ffffffff810889e0>] ? 
ftrace_define_fields_module_load+0x70/0x70
[   19.176445]  [<ffffffff816ed5a2>] system_call_fastpath+0x16/0x1b

Regards
Shobhit

Re: drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

[Daniel Vetter]

On Mon, Jun 11, 2012 at 7:25 AM, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> On 06/08/2012 07:20 PM, Eugeni Dodonov wrote:
>>
>> On 06/08/2012 09:49 AM, Daniel Vetter wrote:
>>>
>>> On Fri, Jun 08, 2012 at 11:44:23AM +0530, Shobhit Kumar wrote:
>>>>
>>>> In Haswell while disabling a pipe, we need to disable the DDI control as
>>>> well along with the PIPECONF. Otherwise we will hit assertions during
>>>> crtc
>>>> disable
>>>
>>>
>>> Hm, can you add such an example assert with backtrace please? All these
>>> asserts encode our current understanding of the hw depency chain, so I'd
>>> like to check whether we're really doing the right thing and don't just
>>> stfu some dmesg noise.
>>>
>>> Thanks, Daniel
>>
>>
>> This is part of the pipe disabling process starting with Haswell. DDI
>> pipe function control should be disabled when pipe is being disabled,
>> otherwise it stays in enabled state and on next enabling we hit the
>> assert within assert_fdi_tx:
>>
>> ...
>> if (IS_HASWELL(dev_priv->dev)) {
>> /* On Haswell, DDI is used instead of FDI_TX_CTL */
>> reg = DDI_FUNC_CTL(pipe);
>> val = I915_READ(reg);
>> cur_state = !!(val & PIPE_DDI_FUNC_ENABLE);
>> ...
>>
> Eugeni already explained how and where the assertion will be raised. Please
> find a sample assertion while loading the driver with HDMI output connected

Yeah, we've discussed this quite a bit, thanks anyway for following
up. Imo I'm not too stressed out about this backtrace since (as per my
discussion with Eugeni) our code and asserts around disabling the
ddi/pch fdi rx stuff is a bit ugly/buggy still for hsw/lpt, so I think
this can wait a bit.

Imo the ddi disable shouldn't be in the pipe disable function but
makes more sense in the fdi disable functions. But then it would be
brutally obviously that that still touches the fdi tx regs on hsw, and
from there on it's all down the rabbit hole ...

-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: drm/i915: Disable DDI Pipe Control on HSW while disabling pipe

[Shobhit Kumar]

On 06/11/2012 12:38 PM, Daniel Vetter wrote:
> On Mon, Jun 11, 2012 at 7:25 AM, Shobhit Kumar<shobhit.kumar@intel.com>  wrote:
>> On 06/08/2012 07:20 PM, Eugeni Dodonov wrote:
>>>
>>> On 06/08/2012 09:49 AM, Daniel Vetter wrote:
>>>>
>>>> On Fri, Jun 08, 2012 at 11:44:23AM +0530, Shobhit Kumar wrote:
>>>>>
>>>>> In Haswell while disabling a pipe, we need to disable the DDI control as
>>>>> well along with the PIPECONF. Otherwise we will hit assertions during
>>>>> crtc
>>>>> disable
>>>>
>>>>
>>>> Hm, can you add such an example assert with backtrace please? All these
>>>> asserts encode our current understanding of the hw depency chain, so I'd
>>>> like to check whether we're really doing the right thing and don't just
>>>> stfu some dmesg noise.
>>>>
>>>> Thanks, Daniel
>>>
>>>
>>> This is part of the pipe disabling process starting with Haswell. DDI
>>> pipe function control should be disabled when pipe is being disabled,
>>> otherwise it stays in enabled state and on next enabling we hit the
>>> assert within assert_fdi_tx:
>>>
>>> ...
>>> if (IS_HASWELL(dev_priv->dev)) {
>>> /* On Haswell, DDI is used instead of FDI_TX_CTL */
>>> reg = DDI_FUNC_CTL(pipe);
>>> val = I915_READ(reg);
>>> cur_state = !!(val&  PIPE_DDI_FUNC_ENABLE);
>>> ...
>>>
>> Eugeni already explained how and where the assertion will be raised. Please
>> find a sample assertion while loading the driver with HDMI output connected
>
> Yeah, we've discussed this quite a bit, thanks anyway for following
> up. Imo I'm not too stressed out about this backtrace since (as per my
> discussion with Eugeni) our code and asserts around disabling the
> ddi/pch fdi rx stuff is a bit ugly/buggy still for hsw/lpt, so I think
> this can wait a bit.

Its just that the DP enabling does not work properly if the pipe 
DDI_FUNC_CTL is not off while I am testing my DP code. In case of HDMI 
even if the assertion is there it works when we re-enable it in modeset, 
but for DP, DDI_FUNC_CTL already enabled spells doom. Maybe it has got 
to do with the fact that HDMI is driver by WRPLL while in my code DP is 
over SPLL ?

>
> Imo the ddi disable shouldn't be in the pipe disable function but
> makes more sense in the fdi disable functions. But then it would be
> brutally obviously that that still touches the fdi tx regs on hsw, and
> from there on it's all down the rabbit hole ...

Yeah, I agree

Regards
Shobhit