Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
> On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti > 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 > > > >> 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 > > > >>> --- > > > >>> 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 0xE000 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 (0x000C) > > #define DP_DP_HPD_PLUG_INT_MASK (0x0001) > > #define DP_DP_IRQ_HPD_INT_MASK (0x0002) > > #define DP_DP_HPD_REPLUG_INT_MASK (0x0004) > > #define DP_DP_HPD_UNPLUG_INT_MASK (0x0008) > > #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(0x4000) > > #define DP_DP_HPD_STATE_STATUS_PENDING (0x2000) > > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x) > > #define DP_DP_HPD_STATE_STATUS_MASK (0xE000) > > > > 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 0x4001 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 0x4000 (or > 0x2000 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
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti 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 > > >> 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 > > >>> --- > > >>> 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 0xE000 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 (0x000C) > #define DP_DP_HPD_PLUG_INT_MASK (0x0001) > #define DP_DP_IRQ_HPD_INT_MASK (0x0002) > #define DP_DP_HPD_REPLUG_INT_MASK (0x0004) > #define DP_DP_HPD_UNPLUG_INT_MASK (0x0008) > #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(0x4000) > #define DP_DP_HPD_STATE_STATUS_PENDING (0x2000) > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x) > #define DP_DP_HPD_STATE_STATUS_MASK (0xE000) > > 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 0x4001 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 0x4000 (or 0x2000 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
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
Hi Dmitry, > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote: > > Hi Dmitry, > > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti > >> 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 > >>> --- > >>> 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 0xE000 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); > 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 (0x000C) #define DP_DP_HPD_PLUG_INT_MASK (0x0001) #define DP_DP_IRQ_HPD_INT_MASK (0x0002) #define DP_DP_HPD_REPLUG_INT_MASK (0x0004) #define DP_DP_HPD_UNPLUG_INT_MASK (0x0008) #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(0x4000) #define DP_DP_HPD_STATE_STATUS_PENDING (0x2000) #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x) #define DP_DP_HPD_STATE_STATUS_MASK (0xE000) 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 0x4001 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 0x4000 (or 0x2000 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
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote: Hi Dmitry, On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti 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 --- 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 0xE000 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); 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
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
Hi Dmitry, > On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti > 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 > > --- > > 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 0xE000 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. 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
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti 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 > --- > 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; ? > } > > int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) > -- > 2.7.4 > -- With best wishes Dmitry
[Freedreno] [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 --- 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