Subject: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

The interrupt register will still reflect the connect and disconnect
interrupt status without generating an actual HW interrupt.
The controller driver should not handle those masked interrupts.

Signed-off-by: Sankeerth Billakanti <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 3c16f95..1809ce2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- int isr = 0;
+ int isr, mask;

isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
(isr & DP_DP_HPD_INT_MASK));
+ mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);

- return isr;
+ return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
}

int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
--
2.7.4


2022-03-31 15:13:22

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti
<[email protected]> wrote:
>
> Hi Dmitry,
>
> > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > > Hi Dmitry,
> > >
> > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> > >> <[email protected]> wrote:
> > >>>
> > >>> The interrupt register will still reflect the connect and disconnect
> > >>> interrupt status without generating an actual HW interrupt.
> > >>> The controller driver should not handle those masked interrupts.
> > >>>
> > >>> Signed-off-by: Sankeerth Billakanti <[email protected]>
> > >>> ---
> > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> index 3c16f95..1809ce2 100644
> > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
> > >>> dp_catalog *dp_catalog) {
> > >>> struct dp_catalog_private *catalog = container_of(dp_catalog,
> > >>> struct dp_catalog_private, dp_catalog);
> > >>> - int isr = 0;
> > >>> + int isr, mask;
> > >>>
> > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> > >>> (isr & DP_DP_HPD_INT_MASK));
> > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> > >>>
> > >>> - return isr;
> > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> > >>
> > >> I suspect that the logic is inverted here. Shouldn't it be:
> > >>
> > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> > >>
> > >> ?
> > >>
> > >
> > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the
> > value
> > > of the read interrupt mask variable could be is 0xF.
> > >
> > > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK,
> > bits 3:0.
> > > The HPD status is indicated via a different read-only register
> > REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> >
> > I see. Maybe the following expression would be better?
> >
> > return isr & (mask & ~DP_DP_HPD_INT_MASK);

Ugh, excuse me please. I meant:

return isr & (mask | ~DP_DP_HPD_INT_MASK);

> >
>
> I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under the same register definition as REG_DP_DP_HPD_INT_MASK
> I will rearrange the definitions below.
>
> #define REG_DP_DP_HPD_INT_MASK (0x0000000C)
> #define DP_DP_HPD_PLUG_INT_MASK (0x00000001)
> #define DP_DP_IRQ_HPD_INT_MASK (0x00000002)
> #define DP_DP_HPD_REPLUG_INT_MASK (0x00000004)
> #define DP_DP_HPD_UNPLUG_INT_MASK (0x00000008)
> #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \
> DP_DP_IRQ_HPD_INT_MASK | \
> DP_DP_HPD_REPLUG_INT_MASK | \
> DP_DP_HPD_UNPLUG_INT_MASK)
>
> Below are status bits from register REG_DP_DP_HPD_INT_STATUS
>
> #define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000)
> #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000)
> #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000)
> #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000)
>
> DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.
>
> For DP, we want to enable all interrupts.
> So the programmed mask value is 0xF. We want to return 0x40000001 for connect and 8 for disconnect
>
> For eDP, we want to disable the connect and disconnect interrupts. So, the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK)
> We want to return 0x40000000 (or 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect respectively.
>
> > >
> > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > >
> > >>> }
> > >>>
> > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> > >>> --
> > >>> 2.7.4
> > >>>
> > >>
> > >>
> > >> --
> > >> With best wishes
> > >> Dmitry
> > >
> > > Thank you,
> > > Sankeerth
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> Thank you,
> Sankeerth



--
With best wishes
Dmitry

Subject: RE: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

> On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti
> <[email protected]> wrote:
> >
> > Hi Dmitry,
> >
> > > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > > > Hi Dmitry,
> > > >
> > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> > > >> <[email protected]> wrote:
> > > >>>
> > > >>> The interrupt register will still reflect the connect and
> > > >>> disconnect interrupt status without generating an actual HW
> interrupt.
> > > >>> The controller driver should not handle those masked interrupts.
> > > >>>
> > > >>> Signed-off-by: Sankeerth Billakanti <[email protected]>
> > > >>> ---
> > > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > > >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> index 3c16f95..1809ce2 100644
> > > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
> > > >>> dp_catalog *dp_catalog) {
> > > >>> struct dp_catalog_private *catalog = container_of(dp_catalog,
> > > >>> struct dp_catalog_private, dp_catalog);
> > > >>> - int isr = 0;
> > > >>> + int isr, mask;
> > > >>>
> > > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> > > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> > > >>> (isr & DP_DP_HPD_INT_MASK));
> > > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> > > >>>
> > > >>> - return isr;
> > > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> > > >>
> > > >> I suspect that the logic is inverted here. Shouldn't it be:
> > > >>
> > > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> > > >>
> > > >> ?
> > > >>
> > > >
> > > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the
> > > value
> > > > of the read interrupt mask variable could be is 0xF.
> > > >
> > > > The mask value is indicated via the register,
> > > > REG_DP_DP_HPD_INT_MASK,
> > > bits 3:0.
> > > > The HPD status is indicated via a different read-only register
> > > REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> > >
> > > I see. Maybe the following expression would be better?
> > >
> > > return isr & (mask & ~DP_DP_HPD_INT_MASK);
>
> Ugh, excuse me please. I meant:
>
> return isr & (mask | ~DP_DP_HPD_INT_MASK);
>

Okay. I will change it.

> > >
> >
> > I believe the confusion occurred because the
> > DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under
> the same register definition as REG_DP_DP_HPD_INT_MASK I will rearrange
> the definitions below.
> >
> > #define REG_DP_DP_HPD_INT_MASK (0x0000000C)
> > #define DP_DP_HPD_PLUG_INT_MASK (0x00000001)
> > #define DP_DP_IRQ_HPD_INT_MASK (0x00000002)
> > #define DP_DP_HPD_REPLUG_INT_MASK (0x00000004)
> > #define DP_DP_HPD_UNPLUG_INT_MASK (0x00000008)
> > #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \
> > DP_DP_IRQ_HPD_INT_MASK | \
> > DP_DP_HPD_REPLUG_INT_MASK | \
> >
> > DP_DP_HPD_UNPLUG_INT_MASK)
> >
> > Below are status bits from register REG_DP_DP_HPD_INT_STATUS
> >
> > #define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000)
> > #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000)
> > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000)
> > #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000)
> >
> > DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits
> 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.
> >
> > For DP, we want to enable all interrupts.
> > So the programmed mask value is 0xF. We want to return 0x40000001 for
> > connect and 8 for disconnect
> >
> > For eDP, we want to disable the connect and disconnect interrupts. So,
> > the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK |
> DP_DP_HPD_REPLUG_INT_MASK) We want to return 0x40000000 (or
> 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect
> respectively.
> >
> > > >
> > > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > > >
> > > >>> }
> > > >>>
> > > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog
> > > >>> *dp_catalog)
> > > >>> --
> > > >>> 2.7.4
> > > >>>
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > > >
> > > > Thank you,
> > > > Sankeerth
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
> > Thank you,
> > Sankeerth
>
>
>
> --
> With best wishes
> Dmitry