[PATCH 0/7] drm/tidss: Interrupt fixes and cleanups

[PATCH 0/7] drm/tidss: Interrupt fixes and cleanups

[Tomi Valkeinen]

A collection of interrupt related fixes and cleanups. A few patches are
from Devarsh and have been posted to dri-devel, which I've included here
with a permission from Devarsh, so that we have all interrupt patches
together. I have modified both of those patches compared to the posted
versions.

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Devarsh Thakkar (2):
      drm/tidss: Clear the interrupt status for interrupts being disabled
      drm/tidss: Fix race condition while handling interrupt registers

Tomi Valkeinen (5):
      drm/tidss: Fix issue in irq handling causing irq-flood issue
      drm/tidss: Remove unused OCP error flag
      drm/tidss: Remove extra K2G check
      drm/tidss: Add printing of underflows
      drm/tidss: Rename 'wait_lock' to 'irq_lock'

 drivers/gpu/drm/tidss/tidss_dispc.c | 28 ++++++++++++++++------------
 drivers/gpu/drm/tidss/tidss_drv.c   |  2 +-
 drivers/gpu/drm/tidss/tidss_drv.h   |  5 +++--
 drivers/gpu/drm/tidss/tidss_irq.c   | 34 +++++++++++++++++++++++-----------
 drivers/gpu/drm/tidss/tidss_irq.h   |  4 +---
 drivers/gpu/drm/tidss/tidss_plane.c |  8 ++++++++
 drivers/gpu/drm/tidss/tidss_plane.h |  2 ++
 7 files changed, 54 insertions(+), 29 deletions(-)
---
base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
change-id: 20240918-tidss-irq-fix-f687b149a42c

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 1/7] drm/tidss: Fix issue in irq handling causing irq-flood issue

[Tomi Valkeinen]

It has been observed that sometimes DSS will trigger an interrupt and
the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and
VID level interrupt-statuses are zero.

As the top level irqstatus is supposed to tell whether we have VP/VID
interrupts, the thinking of the driver authors was that this particular
case could never happen. Thus the driver only clears the DISPC_IRQSTATUS
bits which has corresponding interrupts in VP/VID status. So when this
issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an
interrupt flood.

It is unclear why the issue happens. It could be a race issue in the
driver, but no such race has been found. It could also be an issue with
the HW. However a similar case can be easily triggered by manually
writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the
DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears
the bit, we get an interrupt flood.

To fix the issue, always clear DISPC_IRQSTATUS. The concern with this
solution is that if the top level irqstatus is the one that triggers the
interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts
unhandled if VP/VID interrupt statuses have bits set. However, testing
shows that if any of the irqstatuses is set (i.e. even if
DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an
interrupt.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Co-developed-by: Bin Liu <b-liu@ti.com>
Signed-off-by: Bin Liu <b-liu@ti.com>
Co-developed-by: Devarsh Thakkar <devarsht@ti.com>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 1ad711f8d2a8..f81111067578 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -780,24 +780,20 @@ static
 void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
 {
 	unsigned int i;
-	u32 top_clear = 0;
 
 	for (i = 0; i < dispc->feat->num_vps; ++i) {
-		if (clearmask & DSS_IRQ_VP_MASK(i)) {
+		if (clearmask & DSS_IRQ_VP_MASK(i))
 			dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
-			top_clear |= BIT(i);
-		}
 	}
 	for (i = 0; i < dispc->feat->num_planes; ++i) {
-		if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
+		if (clearmask & DSS_IRQ_PLANE_MASK(i))
 			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
-			top_clear |= BIT(4 + i);
-		}
 	}
 	if (dispc->feat->subrev == DISPC_K2G)
 		return;
 
-	dispc_write(dispc, DISPC_IRQSTATUS, top_clear);
+	/* always clear the top level irqstatus */
+	dispc_write(dispc, DISPC_IRQSTATUS, dispc_read(dispc, DISPC_IRQSTATUS));
 
 	/* Flush posted writes */
 	dispc_read(dispc, DISPC_IRQSTATUS);

-- 
2.43.0

Re: [PATCH 1/7] drm/tidss: Fix issue in irq handling causing irq-flood issue

[Jon Cormier]

On Mon, Oct 21, 2024 at 10:08 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> It has been observed that sometimes DSS will trigger an interrupt and
> the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and
> VID level interrupt-statuses are zero.
>
> As the top level irqstatus is supposed to tell whether we have VP/VID
> interrupts, the thinking of the driver authors was that this particular
> case could never happen. Thus the driver only clears the DISPC_IRQSTATUS
> bits which has corresponding interrupts in VP/VID status. So when this
> issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an
> interrupt flood.
>
> It is unclear why the issue happens. It could be a race issue in the
> driver, but no such race has been found. It could also be an issue with
> the HW. However a similar case can be easily triggered by manually
> writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the
> DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears
> the bit, we get an interrupt flood.
>
> To fix the issue, always clear DISPC_IRQSTATUS. The concern with this
> solution is that if the top level irqstatus is the one that triggers the
> interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts
> unhandled if VP/VID interrupt statuses have bits set. However, testing
> shows that if any of the irqstatuses is set (i.e. even if
> DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an
> interrupt.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Co-developed-by: Bin Liu <b-liu@ti.com>
> Signed-off-by: Bin Liu <b-liu@ti.com>
> Co-developed-by: Devarsh Thakkar <devarsht@ti.com>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

I assume a reviewed by doesn't make sense since I co-developed this
patch but adding my tested by, hopefully, that makes sense.

Tested an equivalent patch for several weeks.
Tested-by: Jonathan Cormier <jcormier@criticallink.com>
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 1ad711f8d2a8..f81111067578 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -780,24 +780,20 @@ static
>  void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
>  {
>         unsigned int i;
> -       u32 top_clear = 0;
>
>         for (i = 0; i < dispc->feat->num_vps; ++i) {
> -               if (clearmask & DSS_IRQ_VP_MASK(i)) {
> +               if (clearmask & DSS_IRQ_VP_MASK(i))
>                         dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
> -                       top_clear |= BIT(i);
> -               }
>         }
>         for (i = 0; i < dispc->feat->num_planes; ++i) {
> -               if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
> +               if (clearmask & DSS_IRQ_PLANE_MASK(i))
>                         dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
> -                       top_clear |= BIT(4 + i);
> -               }
>         }
>         if (dispc->feat->subrev == DISPC_K2G)
>                 return;
>
> -       dispc_write(dispc, DISPC_IRQSTATUS, top_clear);
> +       /* always clear the top level irqstatus */
> +       dispc_write(dispc, DISPC_IRQSTATUS, dispc_read(dispc, DISPC_IRQSTATUS));
>
>         /* Flush posted writes */
>         dispc_read(dispc, DISPC_IRQSTATUS);
>
> --
> 2.43.0
>

Re: [PATCH 1/7] drm/tidss: Fix issue in irq handling causing irq-flood issue

[Aradhya Bhatia]

Hi Tomi, Devarsh,

On 10/21/24 19:37, Tomi Valkeinen wrote:
> It has been observed that sometimes DSS will trigger an interrupt and
> the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and
> VID level interrupt-statuses are zero.

Does this mean that there was a legitimate interrupt that potentially
went unrecognized? Or that there was a, for the lack of a better word,
fake interrupt trigger that doesn't need handling but just clearing?

> 
> As the top level irqstatus is supposed to tell whether we have VP/VID
> interrupts, the thinking of the driver authors was that this particular
> case could never happen. Thus the driver only clears the DISPC_IRQSTATUS
> bits which has corresponding interrupts in VP/VID status. So when this
> issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an
> interrupt flood.
> 
> It is unclear why the issue happens. It could be a race issue in the
> driver, but no such race has been found. It could also be an issue with
> the HW. However a similar case can be easily triggered by manually
> writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the
> DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears
> the bit, we get an interrupt flood.
> 
> To fix the issue, always clear DISPC_IRQSTATUS. The concern with this
> solution is that if the top level irqstatus is the one that triggers the
> interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts
> unhandled if VP/VID interrupt statuses have bits set. However, testing
> shows that if any of the irqstatuses is set (i.e. even if
> DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an
> interrupt.

Does this mean if VID/VP irqstatus has been set right around the time
the equivalent DISPC_IRQSTATUS bit is being cleared, the equivalent
DISPC_IRQSTATUS bit is going to get set again, and make the driver
handle the event as we expect it to?

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Co-developed-by: Bin Liu <b-liu@ti.com>
> Signed-off-by: Bin Liu <b-liu@ti.com>
> Co-developed-by: Devarsh Thakkar <devarsht@ti.com>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 1ad711f8d2a8..f81111067578 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -780,24 +780,20 @@ static
>  void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
>  {
>  	unsigned int i;
> -	u32 top_clear = 0;
>  
>  	for (i = 0; i < dispc->feat->num_vps; ++i) {
> -		if (clearmask & DSS_IRQ_VP_MASK(i)) {
> +		if (clearmask & DSS_IRQ_VP_MASK(i))
>  			dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
> -			top_clear |= BIT(i);
> -		}
>  	}
>  	for (i = 0; i < dispc->feat->num_planes; ++i) {
> -		if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
> +		if (clearmask & DSS_IRQ_PLANE_MASK(i))
>  			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
> -			top_clear |= BIT(4 + i);
> -		}
>  	}

nit: Maybe these for-loop braces could be dropped as well.

Otherwise, LGTM,

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya


[..]

Re: [PATCH 1/7] drm/tidss: Fix issue in irq handling causing irq-flood issue

[Tomi Valkeinen]

Hi,

On 24/11/2024 19:18, Aradhya Bhatia wrote:
> Hi Tomi, Devarsh,
> 
> On 10/21/24 19:37, Tomi Valkeinen wrote:
>> It has been observed that sometimes DSS will trigger an interrupt and
>> the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and
>> VID level interrupt-statuses are zero.
> 
> Does this mean that there was a legitimate interrupt that potentially
> went unrecognized? Or that there was a, for the lack of a better word,
> fake interrupt trigger that doesn't need handling but just clearing?

I don't have an answer to that. I haven't been able to trigger this 
issue, and I guess it's difficult to say for certain in any case.

My guess is that it's some kind of race issue either in the HW or the 
combination of HW+SW.

>> As the top level irqstatus is supposed to tell whether we have VP/VID
>> interrupts, the thinking of the driver authors was that this particular
>> case could never happen. Thus the driver only clears the DISPC_IRQSTATUS
>> bits which has corresponding interrupts in VP/VID status. So when this
>> issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an
>> interrupt flood.
>>
>> It is unclear why the issue happens. It could be a race issue in the
>> driver, but no such race has been found. It could also be an issue with
>> the HW. However a similar case can be easily triggered by manually
>> writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the
>> DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears
>> the bit, we get an interrupt flood.
>>
>> To fix the issue, always clear DISPC_IRQSTATUS. The concern with this
>> solution is that if the top level irqstatus is the one that triggers the
>> interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts
>> unhandled if VP/VID interrupt statuses have bits set. However, testing
>> shows that if any of the irqstatuses is set (i.e. even if
>> DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an
>> interrupt.
> 
> Does this mean if VID/VP irqstatus has been set right around the time
> the equivalent DISPC_IRQSTATUS bit is being cleared, the equivalent
> DISPC_IRQSTATUS bit is going to get set again, and make the driver
> handle the event as we expect it to?

(If I recall right) no, DISPC_IRQSTATUS won't be set. But it doesn't 
matter, the interrupt will be triggered anyway, and the driver will 
handle the interrupt.

>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Co-developed-by: Bin Liu <b-liu@ti.com>
>> Signed-off-by: Bin Liu <b-liu@ti.com>
>> Co-developed-by: Devarsh Thakkar <devarsht@ti.com>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
>> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 1ad711f8d2a8..f81111067578 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -780,24 +780,20 @@ static
>>   void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
>>   {
>>   	unsigned int i;
>> -	u32 top_clear = 0;
>>   
>>   	for (i = 0; i < dispc->feat->num_vps; ++i) {
>> -		if (clearmask & DSS_IRQ_VP_MASK(i)) {
>> +		if (clearmask & DSS_IRQ_VP_MASK(i))
>>   			dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
>> -			top_clear |= BIT(i);
>> -		}
>>   	}
>>   	for (i = 0; i < dispc->feat->num_planes; ++i) {
>> -		if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
>> +		if (clearmask & DSS_IRQ_PLANE_MASK(i))
>>   			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
>> -			top_clear |= BIT(4 + i);
>> -		}
>>   	}
> 
> nit: Maybe these for-loop braces could be dropped as well.

I like to have braces if there are multiple lines under it.

> Otherwise, LGTM,
> 
> Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Thanks!

  Tomi

[PATCH 2/7] drm/tidss: Remove unused OCP error flag

[Tomi Valkeinen]

We never use the DSS_IRQ_DEVICE_OCP_ERR flag, and the HW doesn't even
have such a bit... So remove it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_irq.c | 5 +----
 drivers/gpu/drm/tidss/tidss_irq.h | 4 +---
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
index 604334ef526a..91498ff664a2 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.c
+++ b/drivers/gpu/drm/tidss/tidss_irq.c
@@ -78,9 +78,6 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
 			tidss_crtc_error_irq(crtc, irqstatus);
 	}
 
-	if (irqstatus & DSS_IRQ_DEVICE_OCP_ERR)
-		dev_err_ratelimited(tidss->dev, "OCP error\n");
-
 	return IRQ_HANDLED;
 }
 
@@ -105,7 +102,7 @@ int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
 	if (ret)
 		return ret;
 
-	tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
+	tidss->irq_mask = 0;
 
 	for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
 		struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h
index b512614d5863..dd61f645f662 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.h
+++ b/drivers/gpu/drm/tidss/tidss_irq.h
@@ -19,15 +19,13 @@
  * bit use   |D  |fou|FEOL|FEOL|FEOL|FEOL|  UUUU  |          |
  * bit number|0  |1-3|4-7 |8-11|  12-19  | 20-23  |  24-31   |
  *
- * device bits:	D = OCP error
+ * device bits:	D = Unused
  * WB bits:	f = frame done wb, o = wb buffer overflow,
  *		u = wb buffer uncomplete
  * vp bits:	F = frame done, E = vsync even, O = vsync odd, L = sync lost
  * plane bits:	U = fifo underflow
  */
 
-#define DSS_IRQ_DEVICE_OCP_ERR			BIT(0)
-
 #define DSS_IRQ_DEVICE_FRAMEDONEWB		BIT(1)
 #define DSS_IRQ_DEVICE_WBBUFFEROVERFLOW		BIT(2)
 #define DSS_IRQ_DEVICE_WBUNCOMPLETEERROR	BIT(3)

-- 
2.43.0

Re: [PATCH 2/7] drm/tidss: Remove unused OCP error flag

[Devarsh Thakkar]

On 21/10/24 19:37, Tomi Valkeinen wrote:
> We never use the DSS_IRQ_DEVICE_OCP_ERR flag, and the HW doesn't even
> have such a bit... So remove it.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh
> ---
>  drivers/gpu/drm/tidss/tidss_irq.c | 5 +----
>  drivers/gpu/drm/tidss/tidss_irq.h | 4 +---
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 604334ef526a..91498ff664a2 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -78,9 +78,6 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
>  			tidss_crtc_error_irq(crtc, irqstatus);
>  	}
>  
> -	if (irqstatus & DSS_IRQ_DEVICE_OCP_ERR)
> -		dev_err_ratelimited(tidss->dev, "OCP error\n");
> -
>  	return IRQ_HANDLED;
>  }
>  
> @@ -105,7 +102,7 @@ int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
>  	if (ret)
>  		return ret;
>  
> -	tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
> +	tidss->irq_mask = 0;
>  
>  	for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
>  		struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h
> index b512614d5863..dd61f645f662 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.h
> +++ b/drivers/gpu/drm/tidss/tidss_irq.h
> @@ -19,15 +19,13 @@
>   * bit use   |D  |fou|FEOL|FEOL|FEOL|FEOL|  UUUU  |          |
>   * bit number|0  |1-3|4-7 |8-11|  12-19  | 20-23  |  24-31   |
>   *
> - * device bits:	D = OCP error
> + * device bits:	D = Unused
>   * WB bits:	f = frame done wb, o = wb buffer overflow,
>   *		u = wb buffer uncomplete
>   * vp bits:	F = frame done, E = vsync even, O = vsync odd, L = sync lost
>   * plane bits:	U = fifo underflow
>   */
>  
> -#define DSS_IRQ_DEVICE_OCP_ERR			BIT(0)
> -
>  #define DSS_IRQ_DEVICE_FRAMEDONEWB		BIT(1)
>  #define DSS_IRQ_DEVICE_WBBUFFEROVERFLOW		BIT(2)
>  #define DSS_IRQ_DEVICE_WBUNCOMPLETEERROR	BIT(3)
> 

Re: [PATCH 2/7] drm/tidss: Remove unused OCP error flag

[Aradhya Bhatia]

On 10/21/24 19:37, Tomi Valkeinen wrote:
> We never use the DSS_IRQ_DEVICE_OCP_ERR flag, and the HW doesn't even
> have such a bit... So remove it.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya

[...]

[PATCH 3/7] drm/tidss: Remove extra K2G check

[Tomi Valkeinen]

We check if the platform is K2G in dispc_k3_clear_irqstatus(), and
return early if so. This cannot happen, as the _k3_ functions are never
called on K2G in the first place. So remove the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index f81111067578..99a1138f3e69 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -789,8 +789,6 @@ void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
 		if (clearmask & DSS_IRQ_PLANE_MASK(i))
 			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
 	}
-	if (dispc->feat->subrev == DISPC_K2G)
-		return;
 
 	/* always clear the top level irqstatus */
 	dispc_write(dispc, DISPC_IRQSTATUS, dispc_read(dispc, DISPC_IRQSTATUS));

-- 
2.43.0

Re: [PATCH 3/7] drm/tidss: Remove extra K2G check

[Devarsh Thakkar]

On 21/10/24 19:37, Tomi Valkeinen wrote:
> We check if the platform is K2G in dispc_k3_clear_irqstatus(), and
> return early if so. This cannot happen, as the _k3_ functions are never
> called on K2G in the first place. So remove the check.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index f81111067578..99a1138f3e69 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -789,8 +789,6 @@ void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
>  		if (clearmask & DSS_IRQ_PLANE_MASK(i))
>  			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
>  	}
> -	if (dispc->feat->subrev == DISPC_K2G)
> -		return;
>  
>  	/* always clear the top level irqstatus */
>  	dispc_write(dispc, DISPC_IRQSTATUS, dispc_read(dispc, DISPC_IRQSTATUS));
> 

Re: [PATCH 3/7] drm/tidss: Remove extra K2G check

[Aradhya Bhatia]

On 10/21/24 19:37, Tomi Valkeinen wrote:
> We check if the platform is K2G in dispc_k3_clear_irqstatus(), and
> return early if so. This cannot happen, as the _k3_ functions are never
> called on K2G in the first place. So remove the check.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>


Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya

[...]

[PATCH 4/7] drm/tidss: Add printing of underflows

[Tomi Valkeinen]

Add printing of underflows the same way as we handle sync losts.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_irq.c   | 14 ++++++++++++++
 drivers/gpu/drm/tidss/tidss_plane.c |  8 ++++++++
 drivers/gpu/drm/tidss/tidss_plane.h |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
index 91498ff664a2..3cc4024ec7ff 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.c
+++ b/drivers/gpu/drm/tidss/tidss_irq.c
@@ -78,6 +78,14 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
 			tidss_crtc_error_irq(crtc, irqstatus);
 	}
 
+	for (unsigned int i = 0; i < tidss->num_planes; ++i) {
+		struct drm_plane *plane = tidss->planes[i];
+		struct tidss_plane *tplane = to_tidss_plane(plane);
+
+		if (irqstatus & DSS_IRQ_PLANE_FIFO_UNDERFLOW(tplane->hw_plane_id))
+			tidss_plane_error_irq(plane, irqstatus);
+	}
+
 	return IRQ_HANDLED;
 }
 
@@ -112,6 +120,12 @@ int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
 		tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
 	}
 
+	for (unsigned int i = 0; i < tidss->num_planes; ++i) {
+		struct tidss_plane *tplane = to_tidss_plane(tidss->planes[i]);
+
+		tidss->irq_mask |= DSS_IRQ_PLANE_FIFO_UNDERFLOW(tplane->hw_plane_id);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
index a5d86822c9e3..116de124bddb 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -18,6 +18,14 @@
 #include "tidss_drv.h"
 #include "tidss_plane.h"
 
+void tidss_plane_error_irq(struct drm_plane *plane, u64 irqstatus)
+{
+	struct tidss_plane *tplane = to_tidss_plane(plane);
+
+	dev_err_ratelimited(plane->dev->dev, "Plane%u underflow (irq %llx)\n",
+			    tplane->hw_plane_id, irqstatus);
+}
+
 /* drm_plane_helper_funcs */
 
 static int tidss_plane_atomic_check(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/tidss/tidss_plane.h b/drivers/gpu/drm/tidss/tidss_plane.h
index e933e158b617..aecaf2728406 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.h
+++ b/drivers/gpu/drm/tidss/tidss_plane.h
@@ -22,4 +22,6 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
 				       u32 crtc_mask, const u32 *formats,
 				       u32 num_formats);
 
+void tidss_plane_error_irq(struct drm_plane *plane, u64 irqstatus);
+
 #endif

-- 
2.43.0

Re: [PATCH 4/7] drm/tidss: Add printing of underflows

[Devarsh Thakkar]

On 21/10/24 19:37, Tomi Valkeinen wrote:
> Add printing of underflows the same way as we handle sync losts.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

> ---
>  drivers/gpu/drm/tidss/tidss_irq.c   | 14 ++++++++++++++
>  drivers/gpu/drm/tidss/tidss_plane.c |  8 ++++++++
>  drivers/gpu/drm/tidss/tidss_plane.h |  2 ++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 91498ff664a2..3cc4024ec7ff 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -78,6 +78,14 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
>  			tidss_crtc_error_irq(crtc, irqstatus);
>  	}
>  
> +	for (unsigned int i = 0; i < tidss->num_planes; ++i) {
> +		struct drm_plane *plane = tidss->planes[i];
> +		struct tidss_plane *tplane = to_tidss_plane(plane);
> +
> +		if (irqstatus & DSS_IRQ_PLANE_FIFO_UNDERFLOW(tplane->hw_plane_id))
> +			tidss_plane_error_irq(plane, irqstatus);
> +	}
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -112,6 +120,12 @@ int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
>  		tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
>  	}
>  
> +	for (unsigned int i = 0; i < tidss->num_planes; ++i) {
> +		struct tidss_plane *tplane = to_tidss_plane(tidss->planes[i]);
> +
> +		tidss->irq_mask |= DSS_IRQ_PLANE_FIFO_UNDERFLOW(tplane->hw_plane_id);
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index a5d86822c9e3..116de124bddb 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> @@ -18,6 +18,14 @@
>  #include "tidss_drv.h"
>  #include "tidss_plane.h"
>  
> +void tidss_plane_error_irq(struct drm_plane *plane, u64 irqstatus)
> +{
> +	struct tidss_plane *tplane = to_tidss_plane(plane);
> +
> +	dev_err_ratelimited(plane->dev->dev, "Plane%u underflow (irq %llx)\n",
> +			    tplane->hw_plane_id, irqstatus);
> +}
> +
>  /* drm_plane_helper_funcs */
>  
>  static int tidss_plane_atomic_check(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.h b/drivers/gpu/drm/tidss/tidss_plane.h
> index e933e158b617..aecaf2728406 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.h
> +++ b/drivers/gpu/drm/tidss/tidss_plane.h
> @@ -22,4 +22,6 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
>  				       u32 crtc_mask, const u32 *formats,
>  				       u32 num_formats);
>  
> +void tidss_plane_error_irq(struct drm_plane *plane, u64 irqstatus);
> +
>  #endif
> 

Re: [PATCH 4/7] drm/tidss: Add printing of underflows

[Aradhya Bhatia]

On 10/21/24 19:37, Tomi Valkeinen wrote:
> Add printing of underflows the same way as we handle sync losts.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>


Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya

[...]

[PATCH 5/7] drm/tidss: Clear the interrupt status for interrupts being disabled

[Tomi Valkeinen]

From: Devarsh Thakkar <devarsht@ti.com>

The driver does not touch the irqstatus register when it is disabling
interrupts.  This might cause an interrupt to trigger for an interrupt
that was just disabled.

To fix the issue, clear the irqstatus registers right after disabling
the interrupts.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Cc: stable@vger.kernel.org
Reported-by: Jonathan Cormier <jcormier@criticallink.com>
Closes: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1394222/am625-issue-about-tidss-rcu_preempt-self-detected-stall-on-cpu/5424479#5424479
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
[Tomi: mostly rewrote the patch]
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 99a1138f3e69..515f82e8a0a5 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -700,7 +700,7 @@ void dispc_k2g_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 {
 	dispc_irq_t old_mask = dispc_k2g_read_irqenable(dispc);
 
-	/* clear the irqstatus for newly enabled irqs */
+	/* clear the irqstatus for irqs that will be enabled */
 	dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & mask);
 
 	dispc_k2g_vp_set_irqenable(dispc, 0, mask);
@@ -708,6 +708,9 @@ void dispc_k2g_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 
 	dispc_write(dispc, DISPC_IRQENABLE_SET, (1 << 0) | (1 << 7));
 
+	/* clear the irqstatus for irqs that were disabled */
+	dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & old_mask);
+
 	/* flush posted write */
 	dispc_k2g_read_irqenable(dispc);
 }
@@ -837,7 +840,7 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
 
 	old_mask = dispc_k3_read_irqenable(dispc);
 
-	/* clear the irqstatus for newly enabled irqs */
+	/* clear the irqstatus for irqs that will be enabled */
 	dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & mask);
 
 	for (i = 0; i < dispc->feat->num_vps; ++i) {
@@ -862,6 +865,9 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
 	if (main_disable)
 		dispc_write(dispc, DISPC_IRQENABLE_CLR, main_disable);
 
+	/* clear the irqstatus for irqs that were disabled */
+	dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & old_mask);
+
 	/* Flush posted writes */
 	dispc_read(dispc, DISPC_IRQENABLE_SET);
 }

-- 
2.43.0

Re: [PATCH 5/7] drm/tidss: Clear the interrupt status for interrupts being disabled

[Jon Cormier]

On Mon, Oct 21, 2024 at 10:08 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Devarsh Thakkar <devarsht@ti.com>
>
> The driver does not touch the irqstatus register when it is disabling
> interrupts.  This might cause an interrupt to trigger for an interrupt
> that was just disabled.
>
> To fix the issue, clear the irqstatus registers right after disabling
> the interrupts.
>
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Cc: stable@vger.kernel.org
> Reported-by: Jonathan Cormier <jcormier@criticallink.com>
> Closes: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1394222/am625-issue-about-tidss-rcu_preempt-self-detected-stall-on-cpu/5424479#5424479
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> [Tomi: mostly rewrote the patch]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks for the updates. They look pretty similar to the changes I
proposed and thus look good to me.
Reviewed-by: Jonathan Cormier <jcormier@criticallink.com>
Tested an equivalent patch for several weeks.
Tested-by: Jonathan Cormier <jcormier@criticallink.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 99a1138f3e69..515f82e8a0a5 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c



-- 
Jonathan Cormier
Software Engineer

Voice:  315.425.4045 x222



http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211

Re: [PATCH 5/7] drm/tidss: Clear the interrupt status for interrupts being disabled

[Aradhya Bhatia]

On 10/21/24 19:37, Tomi Valkeinen wrote:
> From: Devarsh Thakkar <devarsht@ti.com>
> 
> The driver does not touch the irqstatus register when it is disabling
> interrupts.  This might cause an interrupt to trigger for an interrupt
> that was just disabled.
> 
> To fix the issue, clear the irqstatus registers right after disabling
> the interrupts.
> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Cc: stable@vger.kernel.org
> Reported-by: Jonathan Cormier <jcormier@criticallink.com>
> Closes: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1394222/am625-issue-about-tidss-rcu_preempt-self-detected-stall-on-cpu/5424479#5424479
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> [Tomi: mostly rewrote the patch]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya

[...]

[PATCH 6/7] drm/tidss: Fix race condition while handling interrupt registers

[Tomi Valkeinen]

From: Devarsh Thakkar <devarsht@ti.com>

The driver has a spinlock for protecting the irq_masks field and irq
enable registers. However, the driver misses protecting the irq status
registers which can lead to races.

Take the spinlock when accessing irqstatus too.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Cc: stable@vger.kernel.org
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
[Tomi: updated the desc]
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 4 ++++
 drivers/gpu/drm/tidss/tidss_irq.c   | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 515f82e8a0a5..07f5c26cfa26 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2767,8 +2767,12 @@ static void dispc_init_errata(struct dispc_device *dispc)
  */
 static void dispc_softreset_k2g(struct dispc_device *dispc)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&dispc->tidss->wait_lock, flags);
 	dispc_set_irqenable(dispc, 0);
 	dispc_read_and_clear_irqstatus(dispc);
+	spin_unlock_irqrestore(&dispc->tidss->wait_lock, flags);
 
 	for (unsigned int vp_idx = 0; vp_idx < dispc->feat->num_vps; ++vp_idx)
 		VP_REG_FLD_MOD(dispc, vp_idx, DISPC_VP_CONTROL, 0, 0, 0);
diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
index 3cc4024ec7ff..8af4682ba56b 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.c
+++ b/drivers/gpu/drm/tidss/tidss_irq.c
@@ -60,7 +60,9 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
 	unsigned int id;
 	dispc_irq_t irqstatus;
 
+	spin_lock(&tidss->wait_lock);
 	irqstatus = dispc_read_and_clear_irqstatus(tidss->dispc);
+	spin_unlock(&tidss->wait_lock);
 
 	for (id = 0; id < tidss->num_crtcs; id++) {
 		struct drm_crtc *crtc = tidss->crtcs[id];

-- 
2.43.0

Re: [PATCH 6/7] drm/tidss: Fix race condition while handling interrupt registers

[Jon Cormier]

On Mon, Oct 21, 2024 at 10:08 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Devarsh Thakkar <devarsht@ti.com>
>
> The driver has a spinlock for protecting the irq_masks field and irq
> enable registers. However, the driver misses protecting the irq status
> registers which can lead to races.
>
> Take the spinlock when accessing irqstatus too.
>
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Cc: stable@vger.kernel.org
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> [Tomi: updated the desc]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 4 ++++
>  drivers/gpu/drm/tidss/tidss_irq.c   | 2 ++
>  2 files changed, 6 insertions(+)

Reviewed-by: Jonathan Cormier <jcormier@criticallink.com>
Tested an equivalent patch for several weeks.
Tested-by: Jonathan Cormier <jcormier@criticallink.com>

>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 515f82e8a0a5..07f5c26cfa26 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2767,8 +2767,12 @@ static void dispc_init_errata(struct dispc_device *dispc)
>   */
>  static void dispc_softreset_k2g(struct dispc_device *dispc)
>  {
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&dispc->tidss->wait_lock, flags);
>         dispc_set_irqenable(dispc, 0);
>         dispc_read_and_clear_irqstatus(dispc);
> +       spin_unlock_irqrestore(&dispc->tidss->wait_lock, flags);
>
>         for (unsigned int vp_idx = 0; vp_idx < dispc->feat->num_vps; ++vp_idx)
>                 VP_REG_FLD_MOD(dispc, vp_idx, DISPC_VP_CONTROL, 0, 0, 0);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 3cc4024ec7ff..8af4682ba56b 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -60,7 +60,9 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
>         unsigned int id;
>         dispc_irq_t irqstatus;
>
> +       spin_lock(&tidss->wait_lock);
>         irqstatus = dispc_read_and_clear_irqstatus(tidss->dispc);
> +       spin_unlock(&tidss->wait_lock);
>
>         for (id = 0; id < tidss->num_crtcs; id++) {
>                 struct drm_crtc *crtc = tidss->crtcs[id];
>
> --
> 2.43.0
>

Re: [PATCH 6/7] drm/tidss: Fix race condition while handling interrupt registers

[Aradhya Bhatia]

On 10/21/24 19:37, Tomi Valkeinen wrote:
> From: Devarsh Thakkar <devarsht@ti.com>
> 
> The driver has a spinlock for protecting the irq_masks field and irq
> enable registers. However, the driver misses protecting the irq status
> registers which can lead to races.
> 
> Take the spinlock when accessing irqstatus too.
> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Cc: stable@vger.kernel.org
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> [Tomi: updated the desc]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya

[...]

[PATCH 7/7] drm/tidss: Rename 'wait_lock' to 'irq_lock'

[Tomi Valkeinen]

The 'wait_lock' name seems to be a copy-paste from omapdrm, and makes no
sense here. Rename it to 'irq_lock'. Also clarify the related comment to
make it clear what it protects, and drop any comments related to
'wait_list' which doesn't exist in tidss.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c |  4 ++--
 drivers/gpu/drm/tidss/tidss_drv.c   |  2 +-
 drivers/gpu/drm/tidss/tidss_drv.h   |  5 +++--
 drivers/gpu/drm/tidss/tidss_irq.c   | 19 +++++++++----------
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 07f5c26cfa26..cacb5f3d8085 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2769,10 +2769,10 @@ static void dispc_softreset_k2g(struct dispc_device *dispc)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&dispc->tidss->wait_lock, flags);
+	spin_lock_irqsave(&dispc->tidss->irq_lock, flags);
 	dispc_set_irqenable(dispc, 0);
 	dispc_read_and_clear_irqstatus(dispc);
-	spin_unlock_irqrestore(&dispc->tidss->wait_lock, flags);
+	spin_unlock_irqrestore(&dispc->tidss->irq_lock, flags);
 
 	for (unsigned int vp_idx = 0; vp_idx < dispc->feat->num_vps; ++vp_idx)
 		VP_REG_FLD_MOD(dispc, vp_idx, DISPC_VP_CONTROL, 0, 0, 0);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index d15f836dca95..c7de8c9fa12e 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -138,7 +138,7 @@ static int tidss_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tidss);
 
-	spin_lock_init(&tidss->wait_lock);
+	spin_lock_init(&tidss->irq_lock);
 
 	ret = dispc_init(tidss);
 	if (ret) {
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..7f4f4282bc04 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -29,8 +29,9 @@ struct tidss_device {
 
 	unsigned int irq;
 
-	spinlock_t wait_lock;	/* protects the irq masks */
-	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
+	/* protects the irq masks field and irqenable/irqstatus registers */
+	spinlock_t irq_lock;
+	dispc_irq_t irq_mask;	/* enabled irqs */
 };
 
 #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)
diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
index 8af4682ba56b..5abc788781f4 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.c
+++ b/drivers/gpu/drm/tidss/tidss_irq.c
@@ -15,10 +15,9 @@
 #include "tidss_irq.h"
 #include "tidss_plane.h"
 
-/* call with wait_lock and dispc runtime held */
 static void tidss_irq_update(struct tidss_device *tidss)
 {
-	assert_spin_locked(&tidss->wait_lock);
+	assert_spin_locked(&tidss->irq_lock);
 
 	dispc_set_irqenable(tidss->dispc, tidss->irq_mask);
 }
@@ -31,11 +30,11 @@ void tidss_irq_enable_vblank(struct drm_crtc *crtc)
 	u32 hw_videoport = tcrtc->hw_videoport;
 	unsigned long flags;
 
-	spin_lock_irqsave(&tidss->wait_lock, flags);
+	spin_lock_irqsave(&tidss->irq_lock, flags);
 	tidss->irq_mask |= DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |
 			   DSS_IRQ_VP_VSYNC_ODD(hw_videoport);
 	tidss_irq_update(tidss);
-	spin_unlock_irqrestore(&tidss->wait_lock, flags);
+	spin_unlock_irqrestore(&tidss->irq_lock, flags);
 }
 
 void tidss_irq_disable_vblank(struct drm_crtc *crtc)
@@ -46,11 +45,11 @@ void tidss_irq_disable_vblank(struct drm_crtc *crtc)
 	u32 hw_videoport = tcrtc->hw_videoport;
 	unsigned long flags;
 
-	spin_lock_irqsave(&tidss->wait_lock, flags);
+	spin_lock_irqsave(&tidss->irq_lock, flags);
 	tidss->irq_mask &= ~(DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |
 			     DSS_IRQ_VP_VSYNC_ODD(hw_videoport));
 	tidss_irq_update(tidss);
-	spin_unlock_irqrestore(&tidss->wait_lock, flags);
+	spin_unlock_irqrestore(&tidss->irq_lock, flags);
 }
 
 static irqreturn_t tidss_irq_handler(int irq, void *arg)
@@ -60,9 +59,9 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
 	unsigned int id;
 	dispc_irq_t irqstatus;
 
-	spin_lock(&tidss->wait_lock);
+	spin_lock(&tidss->irq_lock);
 	irqstatus = dispc_read_and_clear_irqstatus(tidss->dispc);
-	spin_unlock(&tidss->wait_lock);
+	spin_unlock(&tidss->irq_lock);
 
 	for (id = 0; id < tidss->num_crtcs; id++) {
 		struct drm_crtc *crtc = tidss->crtcs[id];
@@ -95,9 +94,9 @@ void tidss_irq_resume(struct tidss_device *tidss)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&tidss->wait_lock, flags);
+	spin_lock_irqsave(&tidss->irq_lock, flags);
 	tidss_irq_update(tidss);
-	spin_unlock_irqrestore(&tidss->wait_lock, flags);
+	spin_unlock_irqrestore(&tidss->irq_lock, flags);
 }
 
 int tidss_irq_install(struct drm_device *ddev, unsigned int irq)

-- 
2.43.0

Re: [PATCH 7/7] drm/tidss: Rename 'wait_lock' to 'irq_lock'

[Devarsh Thakkar]

On 21/10/24 19:37, Tomi Valkeinen wrote:
> The 'wait_lock' name seems to be a copy-paste from omapdrm, and makes no
> sense here. Rename it to 'irq_lock'. Also clarify the related comment to
> make it clear what it protects, and drop any comments related to
> 'wait_list' which doesn't exist in tidss.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c |  4 ++--
>  drivers/gpu/drm/tidss/tidss_drv.c   |  2 +-
>  drivers/gpu/drm/tidss/tidss_drv.h   |  5 +++--
>  drivers/gpu/drm/tidss/tidss_irq.c   | 19 +++++++++----------
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 07f5c26cfa26..cacb5f3d8085 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2769,10 +2769,10 @@ static void dispc_softreset_k2g(struct dispc_device *dispc)
>  {
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&dispc->tidss->wait_lock, flags);
> +	spin_lock_irqsave(&dispc->tidss->irq_lock, flags);
>  	dispc_set_irqenable(dispc, 0);
>  	dispc_read_and_clear_irqstatus(dispc);
> -	spin_unlock_irqrestore(&dispc->tidss->wait_lock, flags);
> +	spin_unlock_irqrestore(&dispc->tidss->irq_lock, flags);
>  
>  	for (unsigned int vp_idx = 0; vp_idx < dispc->feat->num_vps; ++vp_idx)
>  		VP_REG_FLD_MOD(dispc, vp_idx, DISPC_VP_CONTROL, 0, 0, 0);
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index d15f836dca95..c7de8c9fa12e 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -138,7 +138,7 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, tidss);
>  
> -	spin_lock_init(&tidss->wait_lock);
> +	spin_lock_init(&tidss->irq_lock);
>  
>  	ret = dispc_init(tidss);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index d7f27b0b0315..7f4f4282bc04 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -29,8 +29,9 @@ struct tidss_device {
>  
>  	unsigned int irq;
>  
> -	spinlock_t wait_lock;	/* protects the irq masks */
> -	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
> +	/* protects the irq masks field and irqenable/irqstatus registers */
> +	spinlock_t irq_lock;
> +	dispc_irq_t irq_mask;	/* enabled irqs */
>  };
>  
>  #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 8af4682ba56b..5abc788781f4 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -15,10 +15,9 @@
>  #include "tidss_irq.h"
>  #include "tidss_plane.h"
>  
> -/* call with wait_lock and dispc runtime held */
>  static void tidss_irq_update(struct tidss_device *tidss)
>  {
> -	assert_spin_locked(&tidss->wait_lock);
> +	assert_spin_locked(&tidss->irq_lock);
>  
>  	dispc_set_irqenable(tidss->dispc, tidss->irq_mask);
>  }
> @@ -31,11 +30,11 @@ void tidss_irq_enable_vblank(struct drm_crtc *crtc)
>  	u32 hw_videoport = tcrtc->hw_videoport;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&tidss->wait_lock, flags);
> +	spin_lock_irqsave(&tidss->irq_lock, flags);
>  	tidss->irq_mask |= DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |
>  			   DSS_IRQ_VP_VSYNC_ODD(hw_videoport);
>  	tidss_irq_update(tidss);
> -	spin_unlock_irqrestore(&tidss->wait_lock, flags);
> +	spin_unlock_irqrestore(&tidss->irq_lock, flags);
>  }
>  
>  void tidss_irq_disable_vblank(struct drm_crtc *crtc)
> @@ -46,11 +45,11 @@ void tidss_irq_disable_vblank(struct drm_crtc *crtc)
>  	u32 hw_videoport = tcrtc->hw_videoport;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&tidss->wait_lock, flags);
> +	spin_lock_irqsave(&tidss->irq_lock, flags);
>  	tidss->irq_mask &= ~(DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |
>  			     DSS_IRQ_VP_VSYNC_ODD(hw_videoport));
>  	tidss_irq_update(tidss);
> -	spin_unlock_irqrestore(&tidss->wait_lock, flags);
> +	spin_unlock_irqrestore(&tidss->irq_lock, flags);
>  }
>  
>  static irqreturn_t tidss_irq_handler(int irq, void *arg)
> @@ -60,9 +59,9 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg)
>  	unsigned int id;
>  	dispc_irq_t irqstatus;
>  
> -	spin_lock(&tidss->wait_lock);
> +	spin_lock(&tidss->irq_lock);
>  	irqstatus = dispc_read_and_clear_irqstatus(tidss->dispc);
> -	spin_unlock(&tidss->wait_lock);
> +	spin_unlock(&tidss->irq_lock);
>  
>  	for (id = 0; id < tidss->num_crtcs; id++) {
>  		struct drm_crtc *crtc = tidss->crtcs[id];
> @@ -95,9 +94,9 @@ void tidss_irq_resume(struct tidss_device *tidss)
>  {
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&tidss->wait_lock, flags);
> +	spin_lock_irqsave(&tidss->irq_lock, flags);
>  	tidss_irq_update(tidss);
> -	spin_unlock_irqrestore(&tidss->wait_lock, flags);
> +	spin_unlock_irqrestore(&tidss->irq_lock, flags);
>  }
>  
>  int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> 

Re: [PATCH 7/7] drm/tidss: Rename 'wait_lock' to 'irq_lock'

[Aradhya Bhatia]

On 10/21/24 19:37, Tomi Valkeinen wrote:
> The 'wait_lock' name seems to be a copy-paste from omapdrm, and makes no
> sense here. Rename it to 'irq_lock'. Also clarify the related comment to
> make it clear what it protects, and drop any comments related to
> 'wait_list' which doesn't exist in tidss.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Regards
Aradhya

[...]