[PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

Tue May 29 17:43:10 UTC 2018

On 29/05/18 13:17, Heiko Stübner wrote:
> Am Dienstag, 29. Mai 2018, 13:59:42 CEST schrieb Robin Murphy:
>> On 28/05/18 14:20, Heiko Stuebner wrote:
>>> From: Sandy Huang <hjc at rock-chips.com>
>>>
>>> The vop irq is shared between vop and iommu and irq probing in the
>>> iommu driver moved to the probe function recently. This can in some
>>> cases lead to a stall if the irq is triggered while the vop driver
>>> still has it disabled, but the vop irq handler gets called.
>>>
>>> But there is no real need to disable the irq, as the vop can simply
>>> also track its enabled state and ignore irqs in that case.
>>> For this we can simply check the power-domain state of the vop,
>>> similar to how the iommu driver does it.
>>>
>>> So remove the enable/disable handling and add appropriate condition
>>> to the irq handler.
>>>
>>> changes in v2:
>>> - move to just check the power-domain state
>>> - add clock handling
>>>
>>> Signed-off-by: Sandy Huang <hjc at rock-chips.com>
>>> [add commit message, moved to pm_runtime_get_if_in_use]
>>> Signed-off-by: Heiko Stuebner <heiko at sntech.de>
>>> ---
>>>
>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
>>>    1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index
>>> b55156b8ba3b..615a5b44bfe9 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
>>>
>>>    	spin_unlock(&vop->reg_lock);
>>>
>>> -	enable_irq(vop->irq);
>>> -
>>>
>>>    	drm_crtc_vblank_on(crtc);
>>>    	
>>>    	return 0;
>>>
>>> @@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc
>>> *crtc,>
>>>    	vop_dsp_hold_valid_irq_disable(vop);
>>>
>>> -	disable_irq(vop->irq);
>>> -
>>>
>>>    	vop->is_enabled = false;
>>>    	
>>>    	/*
>>>
>>> @@ -1195,6 +1191,16 @@ static irqreturn_t vop_isr(int irq, void *data)
>>>
>>>    	uint32_t active_irqs;
>>>    	int ret = IRQ_NONE;
>>>
>>> +	/*
>>> +	 * The irq is shared with the iommu. If the power-domain is off
>>> +	 * the irq has to be targetted at the iommu.
>>
>> Hmm, aren't the IOMMUs in the same power domain as their respective
>> master, though? I would naively assume so, and it does look that way
>> from the DTs in the BSP kernel.
>>
>> AFAICS the IOMMU usage count should never be greater than the VOP usage
>> count (except before the VOP driver has probed, but I don't think that
>> matters), so although this looks like a sensible change in general I
>> can't help be a little bit puzzled at how and why the flow works.
> 
> Ok, the comment might be misleading. It actually means to use the runtime-pm
> state of the vop-_device_ as a check.
> 
> I.e. in vop_initials(), Marc added the patch clearing and masking all vop
> interrupts. In vop_enable() we have runtime_get_... + enablement of
> vop interrupts, which get disabled in vop_disable again.
> 
> That way, checking the runtime_pm state should be an indicator if the
> irq is for the vop and not the iommu.

Right, but whenever the VOP is nominally-disabled, the IOMMU should also 
be nominally-disabled, in which case if it's even possible for the IRQ 
to be asserted, both drivers would ignore it for the same reason (plus 
the IOMMU driver would also spew a WARN(), which I'm not sure is always 
appropriate...). That's what I couldn't quite make sense of.

However, from serendipitously stumbling across [1] I see that the IOMMU 
is in fact going to get explicitly enabled by the driver core around 
probing the VOP, which does give a window during which the imbalance is 
present. I can imagine that the IOMMU reset via the VOP driver's 
dma_configure_call() might misbehave if e.g. the VOP was left running 
from a bootloader splash screen, but in that case I would expect to see 
various screaming from the IOMMU driver which wasn't apparent in 
Ezequiel's log. Oh well, as I said before the patch looks sane 
regardless of my ability to reason about it ;)

Robin.

[1] https://patchwork.kernel.org/patch/10408825/

>>> +	 */
>>> +	if (!pm_runtime_get_if_in_use(vop->dev))
>>> +		return IRQ_NONE;
>>> +
>>> +	if (WARN_ON(vop_core_clks_enable(vop)))
>>> +		goto out;
>>> +
>>>
>>>    	/*
>>>    	
>>>    	 * interrupt register has interrupt status, enable and clear bits, we
>>>    	 * must hold irq_lock to avoid a race with enable/disable_vblank().
>>>
>>> @@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
>>>
>>>    	spin_unlock(&vop->irq_lock);
>>>    	
>>>    	/* This is expected for vop iommu irqs, since the irq is shared */
>>>
>>> -	if (!active_irqs)
>>> -		return IRQ_NONE;
>>> +	if (!active_irqs) {
>>> +		ret = IRQ_NONE;
>>> +		vop_core_clks_disable(vop);
>>> +		goto out;
>>> +	}
>>>
>>>    	if (active_irqs & DSP_HOLD_VALID_INTR) {
>>>    	
>>>    		complete(&vop->dsp_hold_completion);
>>>
>>> @@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
>>>
>>>    		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
>>>    		
>>>    			      active_irqs);
>>>
>>> +	vop_core_clks_disable(vop);
>>> +
>>> +out:
>>> +	pm_runtime_put(vop->dev);
>>>
>>>    	return ret;
>>>    
>>>    }
>>>
>>> @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct
>>> device *master, void *data)>
>>>    	if (ret)
>>>    	
>>>    		goto err_disable_pm_runtime;
>>>
>>> -	/* IRQ is initially disabled; it gets enabled in power_on */
>>> -	disable_irq(vop->irq);
>>> -
>>>
>>>    	return 0;
>>>    
>>>    err_disable_pm_runtime:
> 
> 
> 
> 