RE: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
Hi Laurent, > -Original Message- > From: Laurent Pinchart > Sent: Friday, January 19, 2024 4:30 AM > To: Klymenko, Anatoliy > Cc: Tomi Valkeinen ; > aarten.lankho...@linux.intel.com; mrip...@kernel.org; tzimmerm...@suse.de; > airl...@gmail.com; dan...@ffwll.ch; Simek, Michal ; > dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org > 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 +, 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 > > > > --- > > > > 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
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
Hi Anatoliy, On Fri, Jan 19, 2024 at 05:53:30AM +, 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 > > > --- > > > 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". > 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
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
Hi Tomi, Thank you for the review. > Date: Wed, 17 Jan 2024 16:20:10 +0200 > From: Tomi Valkeinen > To: Anatoliy Klymenko , > laurent.pinch...@ideasonboard.com, maarten.lankho...@linux.intel.com, > mrip...@kernel.org, tzimmerm...@suse.de, airl...@gmail.com, > dan...@ffwll.ch, michal.si...@amd.com, > dri-devel@lists.freedesktop.org, linux-arm-ker...@lists.infradead.org, > linux-ker...@vger.kernel.org > Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate > vblank in live mode > Message-ID: > 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 > > --- > > 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
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
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? Signed-off-by: Anatoliy Klymenko --- 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. Tomi
[PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
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. Signed-off-by: Anatoliy Klymenko --- 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); -- 2.25.1
