Messages in this thread |  | | From | "Klymenko, Anatoliy" <> | | Subject | Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode | | Date | Fri, 19 Jan 2024 05:53:30 +0000 |
| |
Hi Tomi,
Thank you for the review.
> Date: Wed, 17 Jan 2024 16:20:10 +0200 > From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > To: Anatoliy Klymenko <anatoliy.klymenko@amd.com>, > laurent.pinchart@ideasonboard.com, maarten.lankhorst@linux.intel.com, > mripard@kernel.org, tzimmermann@suse.de, airlied@gmail.com, > daniel@ffwll.ch, michal.simek@amd.com, > dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradeadorg, > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate > vblank in live mode > Message-ID: <beb551c7-bb7e-4cd0-b166-e9aad90c4620@ideasonboard.com> > 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@amd.com> > > --- > > 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
|  |