Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode

[Klymenko, Anatoliy]

Hi Tomi,

Thank you for the review.

> Date: Wed, 17 Jan 2024 16:20:10 +0200
> From: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> To: Anatoliy Klymenko <anatoliy.klymenko@xxxxxxx>,
>         laurent.pinchart@xxxxxxxxxxxxxxxx, maarten.lankhorst@xxxxxxxxxxxxxxx,
>         mripard@xxxxxxxxxx, tzimmermann@xxxxxxx, airlied@xxxxxxxxx,
>         daniel@xxxxxxxx, michal.simek@xxxxxxx,
>         dri-devel@xxxxxxxxxxxxxxxxxxxxx, linux-arm-kernel@lists.infradeadorg,
>         linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate
>         vblank in live mode
> Message-ID: <beb551c7-bb7e-4cd0-b166-e9aad90c4620@xxxxxxxxxxxxxxxx>
> Content-Type: text/plain; charset=UTF-8; format=flowed
> 
> On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> > Filter out status register against interrupts' mask.
> > Some events are being reported via DP status register, even if
> > corresponding interrupts have been disabled. Avoid processing of such
> > events in interrupt handler context.
> 
> The subject talks about vblank and live mode, the the description doesn't Can
> you elaborate in the desc a bit about when this is an issue and why it wasn't
> before?

Yes, I should make patch comment more consistent and elaborate. I will fix it in the next version. Thank you.

> 
> > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@xxxxxxx>
> > ---
> >   drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index d60b7431603f..571c5dbc97e5 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq,
> void *data)
> >       u32 status, mask;
> >
> >       status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS);
> > +     zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
> >       mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK);
> > -     if (!(status & ~mask))
> > +
> > +     /*
> > +      * Status register may report some events, which corresponding interrupts
> > +      * have been disabled. Filter out those events against interrupts' mask.
> > +      */
> > +     status &= ~mask;
> > +
> > +     if (!status)
> >               return IRQ_NONE;
> >
> >       /* dbg for diagnostic, but not much that the driver can do */ @@
> > -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
> *data)
> >       if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK)
> >               dev_dbg_ratelimited(dp->dev, "overflow interrupt\n");
> >
> > -     zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
> >
> >       if (status & ZYNQMP_DP_INT_VBLANK_START)
> >               zynqmp_dpsub_drm_handle_vblank(dp->dpsub);
> 
> Moving the zynqmp_dp_write() is not related to this fix, is it? I think it should be in
> a separate patch.
> 

The sole purpose of zynqmp_dp_write() is to clear status register. The sooner we do it the better (after reading status in the local variable of course). We can also reuse status variable for in-place filtering against the interrupt mask, which makes this change dependent on zynqmp_dp_write() reordering. I will add a comment explaining this. Is it ok?

>   Tomi
> 

Thank you,
Anatoliy