* RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
[not found] ` <TYCPR01MB11269BCE6CAEE3C5063C4D1728693A@TYCPR01MB11269.jpnprd01.prod.outlook.com>
@ 2024-01-04 14:17 ` Biju Das
2024-01-10 19:38 ` Laurent Pinchart
0 siblings, 1 reply; 3+ messages in thread
From: Biju Das @ 2024-01-04 14:17 UTC (permalink / raw)
To: Maxime Ripard, Laurent Pinchart
Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie,
Daniel Vetter, Philipp Zabel, Laurent Pinchart, Kieran Bingham,
Jacopo Mondi, Geert Uytterhoeven, Magnus Damm, dri-devel,
linux-renesas-soc, Fabrizio Castro, Prabhakar Mahadev Lad,
biju.das.au, linux-media
Hi Maxime Ripard,
> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Friday, December 15, 2023 2:56 PM
> Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> > -----Original Message-----
> > From: Maxime Ripard <mripard@kernel.org>
> > Sent: Friday, December 15, 2023 2:18 PM
> > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > On Fri, Dec 15, 2023 at 01:52:28PM +0000, Biju Das wrote:
> > > > > > > > > +static int rzg2l_du_crtc_enable_vblank(struct drm_crtc
> > *crtc) {
> > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > +
> > > > > > > > > + rcrtc->vblank_enable = true;
> > > > > > > > > +
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void rzg2l_du_crtc_disable_vblank(struct
> > > > > > > > > +drm_crtc
> > > > > > > > > +*crtc)
> > > > {
> > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > +
> > > > > > > > > + rcrtc->vblank_enable = false; }
> > > > > > > >
> > > > > > > > You should enable / disable your interrupts here
> > > > > > >
> > > > > > > We don't have dedicated vblank IRQ for enabling/disabling
> > vblank.
> > > > > > >
> > > > > > > vblank is handled by vspd.
> > > > > > >
> > > > > > > vspd is directly rendering images to display,
> > > > > > > rcar_du_crtc_finish_page_flip() and drm_crtc_handle_vblank()
> > > > > > > called in vspd's pageflip context.
> > > > > > >
> > > > > > > See rzg2l_du_vsp_complete()in rzg2l_du_vsp.c
> > > > > >
> > > > > > Sorry, I couldn't really get how the interrupt flow / vblank
> > > > > > reporting is going to work. Could you explain it a bit more?
> > > > >
> > > > > We just need to handle vertical blanking in the VSP frame end
> > handler.
> > > > > See the code below.
> > > > >
> > > > > static void rzg2l_du_vsp_complete(void *private, unsigned int
> > > > > status,
> > > > > u32 crc) {
> > > > > struct rzg2l_du_crtc *crtc = private;
> > > > >
> > > > > if (crtc->vblank_enable)
> > > > > drm_crtc_handle_vblank(&crtc->crtc);
> > > > >
> > > > > if (status & VSP1_DU_STATUS_COMPLETE)
> > > > > rzg2l_du_crtc_finish_page_flip(crtc);
> > > > >
> > > > > drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc); }
> > > >
> > > > Then we're back to the same question :)
> > > >
> > > > Why can't you mask the frame end interrupt?
> > >
> > > We are masking interrupts.
> > >
> > > [ 70.639139] #######rzg2l_du_crtc_disable_vblank#######
> > > [ 70.650243] #########rzg2l_du_vsp_disable ############
> > > [ 70.652003] ########## vsp1_wpf_stop###
> > >
> > > Unmask is,
> > >
> > > [ 176.354520] #######rzg2l_du_crtc_enable_vblank#######
> > > [ 176.354922] #########rzg2l_du_vsp_atomic_flush ############ [
> > > 176.355198] ########## wpf_configure_stream###
> >
> > Sorry, my question was why aren't you unmasking and masking them in
> > the enable/disable_vblank hooks of the CRTC.
>
> I have n't tried that. Will try and provide feedback.
>
> Currently the IRQ source belongs to VSPD in media subsystem.
> So I need to export an API though vsp1_drm and test it.
>
+ linux-media
Laurent, are you ok with the below RZ/G2L specific patch[1] for enabling/disabling frame end interrupt in VSP driver?
Note:
I need to add a quirk for handling this only for RZ/G2L family as other SoCs have Vblank specific interrupt available in DU.
[1]
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
index 9b087bd8df7d..39347c16bb27 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
@@ -936,6 +936,14 @@ void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt)
}
EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
+void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask)
+{
+ struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+
+ vsp1_write(vsp1, VI6_WPF_IRQ_ENB(0), mask ? 0 : VI6_WPF_IRQ_ENB_DFEE);
+}
+EXPORT_SYMBOL_GPL(vsp1_du_mask_frame_end_interrupt);
+
/* -----------------------------------------------------------------------------
* Initialization
*/
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index 48f4a5023d81..ccac48a6bdd2 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -117,4 +117,6 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt);
void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);
+void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask);
+
#endif /* __MEDIA_VSP1_H__ */
Cheers,
Biju
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
2024-01-04 14:17 ` [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support Biju Das
@ 2024-01-10 19:38 ` Laurent Pinchart
2024-01-11 9:31 ` Biju Das
0 siblings, 1 reply; 3+ messages in thread
From: Laurent Pinchart @ 2024-01-10 19:38 UTC (permalink / raw)
To: Biju Das
Cc: Maxime Ripard, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Philipp Zabel, Kieran Bingham,
Jacopo Mondi, Geert Uytterhoeven, Magnus Damm, dri-devel,
linux-renesas-soc, Fabrizio Castro, Prabhakar Mahadev Lad,
biju.das.au, linux-media
Hi Biju,
On Thu, Jan 04, 2024 at 02:17:39PM +0000, Biju Das wrote:
> On Friday, December 15, 2023 2:56 PM, Biju Das wrote:
> > On Friday, December 15, 2023 2:18 PM, Maxime Ripard wrote:
> > > On Fri, Dec 15, 2023 at 01:52:28PM +0000, Biju Das wrote:
> > > > > > > > > > +static int rzg2l_du_crtc_enable_vblank(struct drm_crtc *crtc) {
> > > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > > +
> > > > > > > > > > + rcrtc->vblank_enable = true;
> > > > > > > > > > +
> > > > > > > > > > + return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void rzg2l_du_crtc_disable_vblank(struct drm_crtc *crtc)
> > > > > > > > > > +{
> > > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > > +
> > > > > > > > > > + rcrtc->vblank_enable = false; }
> > > > > > > > >
> > > > > > > > > You should enable / disable your interrupts here
> > > > > > > >
> > > > > > > > We don't have dedicated vblank IRQ for enabling/disabling vblank.
> > > > > > > >
> > > > > > > > vblank is handled by vspd.
> > > > > > > >
> > > > > > > > vspd is directly rendering images to display,
> > > > > > > > rcar_du_crtc_finish_page_flip() and drm_crtc_handle_vblank()
> > > > > > > > called in vspd's pageflip context.
> > > > > > > >
> > > > > > > > See rzg2l_du_vsp_complete()in rzg2l_du_vsp.c
> > > > > > >
> > > > > > > Sorry, I couldn't really get how the interrupt flow / vblank
> > > > > > > reporting is going to work. Could you explain it a bit more?
> > > > > >
> > > > > > We just need to handle vertical blanking in the VSP frame end handler.
> > > > > > See the code below.
> > > > > >
> > > > > > static void rzg2l_du_vsp_complete(void *private, unsigned int status,
> > > > > > u32 crc) {
> > > > > > struct rzg2l_du_crtc *crtc = private;
> > > > > >
> > > > > > if (crtc->vblank_enable)
> > > > > > drm_crtc_handle_vblank(&crtc->crtc);
> > > > > >
> > > > > > if (status & VSP1_DU_STATUS_COMPLETE)
> > > > > > rzg2l_du_crtc_finish_page_flip(crtc);
> > > > > >
> > > > > > drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc); }
> > > > >
> > > > > Then we're back to the same question :)
> > > > >
> > > > > Why can't you mask the frame end interrupt?
> > > >
> > > > We are masking interrupts.
> > > >
> > > > [ 70.639139] #######rzg2l_du_crtc_disable_vblank#######
> > > > [ 70.650243] #########rzg2l_du_vsp_disable ############
> > > > [ 70.652003] ########## vsp1_wpf_stop###
> > > >
> > > > Unmask is,
> > > >
> > > > [ 176.354520] #######rzg2l_du_crtc_enable_vblank#######
> > > > [ 176.354922] #########rzg2l_du_vsp_atomic_flush ############
> > > > [ 176.355198] ########## wpf_configure_stream###
> > >
> > > Sorry, my question was why aren't you unmasking and masking them in
> > > the enable/disable_vblank hooks of the CRTC.
> >
> > I have n't tried that. Will try and provide feedback.
> >
> > Currently the IRQ source belongs to VSPD in media subsystem.
> > So I need to export an API though vsp1_drm and test it.
>
> + linux-media
>
> Laurent, are you ok with the below RZ/G2L specific patch[1] for
> enabling/disabling frame end interrupt in VSP driver?
> Note:
> I need to add a quirk for handling this only for RZ/G2L family as
> other SoCs have Vblank specific interrupt available in DU.
The DU driver on Gen3 handles vblank exactly as in your patch. What's
the problem with that ?
> [1]
>
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> index 9b087bd8df7d..39347c16bb27 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> @@ -936,6 +936,14 @@ void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt)
> }
> EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
>
> +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask)
> +{
> + struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> + vsp1_write(vsp1, VI6_WPF_IRQ_ENB(0), mask ? 0 : VI6_WPF_IRQ_ENB_DFEE);
That will break everything. As soon as you turn of vblank reporting, the
VSP will stop processing frames and the display will freeze.
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_mask_frame_end_interrupt);
> +
> /* -----------------------------------------------------------------------------
> * Initialization
> */
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 48f4a5023d81..ccac48a6bdd2 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -117,4 +117,6 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt);
> void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);
>
> +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask);
> +
> #endif /* __MEDIA_VSP1_H__ */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
2024-01-10 19:38 ` Laurent Pinchart
@ 2024-01-11 9:31 ` Biju Das
0 siblings, 0 replies; 3+ messages in thread
From: Biju Das @ 2024-01-11 9:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Maxime Ripard, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Philipp Zabel, Kieran Bingham,
Jacopo Mondi, Geert Uytterhoeven, Magnus Damm, dri-devel,
linux-renesas-soc, Fabrizio Castro, Prabhakar Mahadev Lad,
biju.das.au, linux-media
Hi Laurent,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, January 10, 2024 7:39 PM
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> Hi Biju,
>
> On Thu, Jan 04, 2024 at 02:17:39PM +0000, Biju Das wrote:
> > On Friday, December 15, 2023 2:56 PM, Biju Das wrote:
> > > On Friday, December 15, 2023 2:18 PM, Maxime Ripard wrote:
> > > > On Fri, Dec 15, 2023 at 01:52:28PM +0000, Biju Das wrote:
> > > > > > > > > > > +static int rzg2l_du_crtc_enable_vblank(struct
> drm_crtc *crtc) {
> > > > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > > > +
> > > > > > > > > > > + rcrtc->vblank_enable = true;
> > > > > > > > > > > +
> > > > > > > > > > > + return 0;
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static void rzg2l_du_crtc_disable_vblank(struct
> > > > > > > > > > > +drm_crtc *crtc) {
> > > > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > > > +
> > > > > > > > > > > + rcrtc->vblank_enable = false; }
> > > > > > > > > >
> > > > > > > > > > You should enable / disable your interrupts here
> > > > > > > > >
> > > > > > > > > We don't have dedicated vblank IRQ for enabling/disabling
> vblank.
> > > > > > > > >
> > > > > > > > > vblank is handled by vspd.
> > > > > > > > >
> > > > > > > > > vspd is directly rendering images to display,
> > > > > > > > > rcar_du_crtc_finish_page_flip() and
> > > > > > > > > drm_crtc_handle_vblank() called in vspd's pageflip
> context.
> > > > > > > > >
> > > > > > > > > See rzg2l_du_vsp_complete()in rzg2l_du_vsp.c
> > > > > > > >
> > > > > > > > Sorry, I couldn't really get how the interrupt flow /
> > > > > > > > vblank reporting is going to work. Could you explain it a
> bit more?
> > > > > > >
> > > > > > > We just need to handle vertical blanking in the VSP frame end
> handler.
> > > > > > > See the code below.
> > > > > > >
> > > > > > > static void rzg2l_du_vsp_complete(void *private, unsigned
> > > > > > > int status,
> > > > > > > u32 crc) {
> > > > > > > struct rzg2l_du_crtc *crtc = private;
> > > > > > >
> > > > > > > if (crtc->vblank_enable)
> > > > > > > drm_crtc_handle_vblank(&crtc->crtc);
> > > > > > >
> > > > > > > if (status & VSP1_DU_STATUS_COMPLETE)
> > > > > > > rzg2l_du_crtc_finish_page_flip(crtc);
> > > > > > >
> > > > > > > drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc); }
> > > > > >
> > > > > > Then we're back to the same question :)
> > > > > >
> > > > > > Why can't you mask the frame end interrupt?
> > > > >
> > > > > We are masking interrupts.
> > > > >
> > > > > [ 70.639139] #######rzg2l_du_crtc_disable_vblank#######
> > > > > [ 70.650243] #########rzg2l_du_vsp_disable ############
> > > > > [ 70.652003] ########## vsp1_wpf_stop###
> > > > >
> > > > > Unmask is,
> > > > >
> > > > > [ 176.354520] #######rzg2l_du_crtc_enable_vblank#######
> > > > > [ 176.354922] #########rzg2l_du_vsp_atomic_flush ############ [
> > > > > 176.355198] ########## wpf_configure_stream###
> > > >
> > > > Sorry, my question was why aren't you unmasking and masking them
> > > > in the enable/disable_vblank hooks of the CRTC.
> > >
> > > I have n't tried that. Will try and provide feedback.
> > >
> > > Currently the IRQ source belongs to VSPD in media subsystem.
> > > So I need to export an API though vsp1_drm and test it.
> >
> > + linux-media
> >
> > Laurent, are you ok with the below RZ/G2L specific patch[1] for
> > enabling/disabling frame end interrupt in VSP driver?
> > Note:
> > I need to add a quirk for handling this only for RZ/G2L family as
> > other SoCs have Vblank specific interrupt available in DU.
>
> The DU driver on Gen3 handles vblank exactly as in your patch. What's the
> problem with that ?
There is no issue. Maxime Ripard is checking whether is it possible to mask/unmask
Interrupt associated with vblank reporting during vblank{enable, disable}?
That is the reason I produced the below patch[1] for suggestions.
>
> > [1]
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > index 9b087bd8df7d..39347c16bb27 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > @@ -936,6 +936,14 @@ void vsp1_du_unmap_sg(struct device *dev, struct
> > sg_table *sgt) } EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
> >
> > +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask)
> > +{
> > + struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > +
> > + vsp1_write(vsp1, VI6_WPF_IRQ_ENB(0), mask ? 0 :
> > + VI6_WPF_IRQ_ENB_DFEE);
>
> That will break everything. As soon as you turn of vblank reporting, the
> VSP will stop processing frames and the display will freeze.
OK. I am not able to reproduce this issue on RZ/G2L with limited testing compared to
R-Car. As per your suggestion, I will drop this change. I hope it is ok for
everyone.
Cheers,
Biju
>
> > +}
> > +EXPORT_SYMBOL_GPL(vsp1_du_mask_frame_end_interrupt);
> > +
> > /* --------------------------------------------------------------------
> ---------
> > * Initialization
> > */
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h index
> > 48f4a5023d81..ccac48a6bdd2 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
> > @@ -117,4 +117,6 @@ void vsp1_du_atomic_flush(struct device *dev,
> > unsigned int pipe_index, int vsp1_du_map_sg(struct device *dev,
> > struct sg_table *sgt); void vsp1_du_unmap_sg(struct device *dev,
> > struct sg_table *sgt);
> >
> > +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask);
> > +
> > #endif /* __MEDIA_VSP1_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-11 9:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20231128105129.161121-1-biju.das.jz@bp.renesas.com>
[not found] ` <20231128105129.161121-4-biju.das.jz@bp.renesas.com>
[not found] ` <sechknyg33iucaku37vfhk7ie7xgcealfqbvaopm4rrnqbo5g5@s35peonkzzoz>
[not found] ` <TYCPR01MB11269767836DEB995747B7ED3868CA@TYCPR01MB11269.jpnprd01.prod.outlook.com>
[not found] ` <zp2ews2by6fg5irmb7ms6blox6vruezdjlor3rutqtokbvlle2@tl775slyvhyf>
[not found] ` <TYCPR01MB112699C55873FA75B8F4469C18693A@TYCPR01MB11269.jpnprd01.prod.outlook.com>
[not found] ` <bw3qxved377k5pmh3dbnimiyxra7k6dgb2tmg23bvxnfglti4g@uqdxmgnqrkg2>
[not found] ` <TYCPR01MB11269CC132B84CFAD11D307578693A@TYCPR01MB11269.jpnprd01.prod.outlook.com>
[not found] ` <g4uqcavglw2md4ojiw7yxequy37ttozjazr3b4ypqzznlrsinv@zm6mvzaempwp>
[not found] ` <TYCPR01MB11269BCE6CAEE3C5063C4D1728693A@TYCPR01MB11269.jpnprd01.prod.outlook.com>
2024-01-04 14:17 ` [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support Biju Das
2024-01-10 19:38 ` Laurent Pinchart
2024-01-11 9:31 ` Biju Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).