Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge

Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge

[Klymenko, Anatoliy]

Hi Tomi,

Thanks for your response. 

> Date: Wed, 17 Jan 2024 16:06:31 +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.infradead.org,
>         linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge
>         discoverable
> Message-ID: <98a9f4f1-dd55-47c3-bb1b-07e201b299cd@ideasonboard.com>
> Content-Type: text/plain; charset=UTF-8; format=flowed
> 
> On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> > Assign device of node to bridge prior registering it. This will make
> > said bridge discoverable by separate crtc driver.
> 
> I think a few words on why this is needed (and why it wasn't needed
> before) would be nice.

Sure. I will update. 
"  ZynqMP DPSUB supports 2 input modes: DMA based and live video.

    In the first mode, the driver implements CRTC and DP encoder DRM bridge to
    model the complete display pipeline. In this case, DRM bridge is being directly
    instantiated within the driver, not using any bridge discovery mechanisms.

    In the live video input mode video signal is generated by FPGA fabric and passed
    into DPSUB over the connected bus. In this mode driver exposes the DP encoder as
    a DRM bridge, expecting external CRTC to discover it via
    drm_of_find_panel_or_bridge() or a similar call. This discovery relies on
    drm_bridge.of_node being properly set.

    Assign device OF node to the bridge prior to registering it. This will
    make said bridge discoverable by an external CRTC driver.".
How does this sound?

> 
> Other than that:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>   Tomi
> 
> > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> > ---
> >   drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index a0606fab0e22..d60b7431603f 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> >       bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
> >                   | DRM_BRIDGE_OP_HPD;
> >       bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> > +     bridge->of_node = dp->dev->of_node;
> >       dpsub->bridge = bridge;
> >
> >       /*
> 
> 

Regards
Anatoliy

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@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.infradead.org,
>         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

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

[Laurent Pinchart]

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@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).

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

[Klymenko, Anatoliy]

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Friday, January 19, 2024 4:30 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>;
> aarten.lankhorst@linux.intel.com; mripard@kernel.org; tzimmermann@suse.de;
> airlied@gmail.com; daniel@ffwll.ch; Simek, Michal <michal.simek@amd.com>;
> dri-devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@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 +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@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).
> 
> 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

[Tomi Valkeinen]

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 <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.

  Tomi

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

[Anatoliy Klymenko]

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 <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);
-- 
2.25.1