Hi Laurent, > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Sent: Friday, January 19, 2024 4:30 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@xxxxxxx> > Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>; > aarten.lankhorst@xxxxxxxxxxxxxxx; mripard@xxxxxxxxxx; tzimmermann@xxxxxxx; > airlied@xxxxxxxxx; daniel@xxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>; > dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live > mode > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Anatoliy, > > On Fri, Jan 19, 2024 at 05:53:30AM +0000, Klymenko, Anatoliy wrote: > > On Wed, 17 Jan 2024 16:20:10 +0200, Tomi Valkeinen wrote: > > > 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). > > No disagreement about that. Tomi's point is that it's a good change, but it should > be done in a separate patch, by itself, not bundled with other changes. The usual > rule in the kernel is "one change, one commit". > Sure, I will move this into a separate commit in the next version. Thank you. > > 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? > > -- > Regards, > > Laurent Pinchart Thank you, Anatoliy
