RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Laurent,
Thanks for the feedback.
> -Original Message-
> From: Laurent Pinchart
> 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 +, 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 +, 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.
> > >
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Biju,
On Thu, Jan 04, 2024 at 02:17:39PM +, 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 +, 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,
L
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime, On Wed, Jan 10, 2024 at 05:14:41PM +0100, Maxime Ripard wrote: > On Thu, Jan 04, 2024 at 02:34:33PM +, Biju Das wrote: > > On Friday, December 15, 2023 2:24 PM, Maxime Ripard wrote: > > > On Fri, Dec 15, 2023 at 01:25:48PM +, Biju Das wrote: > > > > On Friday, December 15, 2023 10:24 AM, Maxime Ripard wrote: > > > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote: > > > > > > Hi Maxime Ripard, > > > > > > > > > > > > Thanks for the feedback. > > > > > > > > > > Thanks, that's super helpful. The architecture is thus similar to > > > > > vc4 > > > > > > > > > > Some general questions related to bugs we had at some point with vc4: > > > > > > > > > > * Where is the display list stored? In RAM or in a dedicated SRAM? > > > > > > > > [1] It is in DDR (RAM). > > > > > > > > > > > > > > * Are the pointer to the current display list latched? > > > > > > > > > > * Is the display list itself latched? If it's not, what happens when > > > > > the display list is changed while the frame is being generated? > > > > > > > > There is some protocol defined for SW side and HW side for updating > > > > display list See [1] 33.4.8.1 Operation flow of VSPD and DU. > > > > > > > > All the display list operations are manged here[2] > > > > > > > > [1] > > > > https://www.renesas.com/us/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0 > > > > > > > > [2] > > > > https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/media/platform/renesas/vsp1/vsp1_dl.c#L863 > > > > > > I'm sorry, but I'm not going to read a 3500+ to try to figure it out. > > > Could you answer the questions above? > > > > The answer for your question is, > > > > If a previous display list has been queued to the hardware but not > > processed yet, the VSP can start processing it at any time. In that > > case we can't replace the queued list by the new one, as we could > > race with the hardware. We thus mark the update as pending, it will > > be queued up to the hardware by the frame end interrupt handler. > > Ok, so you need to make sure that the list entries are allocated and > tied to the state. That way, you'll know for sure it'll get destroyed > only once the state isn't used anymore, so after the vblank. Because the VSP started as a memory-to-memory processing IP without being tied to the display, it got supported by a V4L2 (and MC) driver. Later on, the R-Car Gen2 added a direct connection between the output of *some* VSP instances and one input of the DU (display engine), using the VSP as an extra "plane" from a KMS point of view. Using the VSP was optional, as the DU had "normal" planes. R-Car Gen3 dropped support for direct memory access in the DU, making usage of the VSP mandatory. As using the VSP is mandatory for display operation on R-Car Gen3, we ruled out forcing userspace to deal with a KMS *and* a V4L2 device manually to get anything out on the screen. We also ruled out redeveloping VSP support inside the DU driver, as that would have duplicated (complex) code. Instead, the DU driver communicates with the VSP driver within the kernel, completely transparently for userspace. This in-kernel API is defined in include/media/vsp1.h, and consists of the following operations that have been modelled on the KMS atomic API: - One-time setup at driver initialization time: int vsp1_du_init(struct device *dev); - Configuration at CRTC enable/disable time: int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index, const struct vsp1_du_lif_config *cfg); This includes configuration of the output width/height. (LIF stands for "LCD InterFace" if I recall correctly, it's the block in the VSP that connects to the DU.) - Atomic updates void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index); int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index, unsigned int rpf, const struct vsp1_du_atomic_config *cfg); void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index, const struct vsp1_du_atomic_pipe_config *cfg); These operations configure planes (the VSP has a blending engine with 2 to 5 inputs depending on the exact SoC) and writeback (the VSP being historically just a memory-to-memory engine, it supports writing the output to memory in addition to forwarding it to the DU). The display lists are fully handled inside the VSP driver, the DU doesn't need to manage them. You can ignore the implementation details of the VSP itself. -- Regards, Laurent Pinchart
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Thu, Jan 04, 2024 at 02:34:33PM +, Biju Das wrote: > Hi Maxime Ripard, > > > -Original Message- > > From: Maxime Ripard > > Sent: Friday, December 15, 2023 2:24 PM > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > > > On Fri, Dec 15, 2023 at 01:25:48PM +, Biju Das wrote: > > > Hi Maxime Ripard, > > > > > > > -Original Message- > > > > From: Maxime Ripard > > > > Sent: Friday, December 15, 2023 10:24 AM > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > > > > > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote: > > > > > Hi Maxime Ripard, > > > > > > > > > > Thanks for the feedback. > > > > > > > > Thanks, that's super helpful. The architecture is thus similar to > > > > vc4 > > > > > > > > Some general questions related to bugs we had at some point with vc4: > > > > > > > > * Where is the display list stored? In RAM or in a dedicated SRAM? > > > > > > [1] It is in DDR (RAM). > > > > > > > > > > > * Are the pointer to the current display list latched? > > > > > > > > * Is the display list itself latched? If it's not, what happens when > > > > the display list is changed while the frame is being generated? > > > > > > There is some protocol defined for SW side and HW side for updating > > > display list See [1] 33.4.8.1 Operation flow of VSPD and DU. > > > > > > All the display list operations are manged here[2] > > > > > > [1] > > > https://www.renesas.com/us/en/document/mah/rzg2l-group-rzg2lc-group-us > > > ers-manual-hardware-0 > > > > > > [2] > > > https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/media/platfor > > > m/renesas/vsp1/vsp1_dl.c#L863 > > > > I'm sorry, but I'm not going to read a 3500+ to try to figure it out. > > Could you answer the questions above? > > The answer for your question is, > > If a previous display list has been queued to the hardware but not > processed yet, the VSP can start processing it at any time. In that > case we can't replace the queued list by the new one, as we could > race with the hardware. We thus mark the update as pending, it will > be queued up to the hardware by the frame end interrupt handler. Ok, so you need to make sure that the list entries are allocated and tied to the state. That way, you'll know for sure it'll get destroyed only once the state isn't used anymore, so after the vblank. Maxime signature.asc Description: PGP signature
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard, > -Original Message- > From: Maxime Ripard > Sent: Friday, December 15, 2023 2:24 PM > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > On Fri, Dec 15, 2023 at 01:25:48PM +, Biju Das wrote: > > Hi Maxime Ripard, > > > > > -Original Message- > > > From: Maxime Ripard > > > Sent: Friday, December 15, 2023 10:24 AM > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > > > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote: > > > > Hi Maxime Ripard, > > > > > > > > Thanks for the feedback. > > > > > > Thanks, that's super helpful. The architecture is thus similar to > > > vc4 > > > > > > Some general questions related to bugs we had at some point with vc4: > > > > > > * Where is the display list stored? In RAM or in a dedicated SRAM? > > > > [1] It is in DDR (RAM). > > > > > > > > * Are the pointer to the current display list latched? > > > > > > * Is the display list itself latched? If it's not, what happens when > > > the display list is changed while the frame is being generated? > > > > There is some protocol defined for SW side and HW side for updating > > display list See [1] 33.4.8.1 Operation flow of VSPD and DU. > > > > All the display list operations are manged here[2] > > > > [1] > > https://www.renesas.com/us/en/document/mah/rzg2l-group-rzg2lc-group-us > > ers-manual-hardware-0 > > > > [2] > > https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/media/platfor > > m/renesas/vsp1/vsp1_dl.c#L863 > > I'm sorry, but I'm not going to read a 3500+ to try to figure it out. > Could you answer the questions above? The answer for your question is, If a previous display list has been queued to the hardware but not processed yet, the VSP can start processing it at any time. In that case we can't replace the queued list by the new one, as we could race with the hardware. We thus mark the update as pending, it will be queued up to the hardware by the frame end interrupt handler. Cheers, Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Biju Das
> 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
> > 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 +, 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
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> Hi Maxime Ripard,
>
> > -Original Message-
> > From: Maxime Ripard
> > 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 +, 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.
>
>
> Currently we disable IRQ in rzg2l_du_crtc_atomic_disable() context
>
> And enable IRQ in rzg2l_du_vsp_atomic_flush().
Typo, it is rzg2l_du_crtc_atomic_flush().
Cheers,
Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Maxime Ripard
> 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 +, 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.
Currently we disable IRQ in rzg2l_du_crtc_atomic_disable() context
And enable IRQ in rzg2l_du_vsp_atomic_flush().
Cheers,
Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Maxime Ripard
> Sent: Friday, December 15, 2023 2:23 PM
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> On Fri, Dec 15, 2023 at 02:19:25PM +, Biju Das wrote:
> > Hi Maxime Ripard,
> >
> > > -Original Message-
> > > From: Biju Das
> > > Sent: Friday, December 15, 2023 1:52 PM
> > > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > Hi Maxime Ripard,
> > >
> > > > -Original Message-
> > > > From: Maxime Ripard
> > > > Sent: Friday, December 15, 2023 12:58 PM
> > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > >
> > > > On Fri, Dec 15, 2023 at 11:37:07AM +, Biju Das wrote:
> > > > > Hi Maxime Ripard,
> > > > >
> > > > > > -Original Message-
> > > > > > From: Maxime Ripard
> > > > > > Sent: Friday, December 15, 2023 10:24 AM
> > > > > > To: Biju Das
> > > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU
> > > > > > Support
> > > > > >
> > > > > > On Thu, Dec 14, 2023 at 03:24:17PM +0000, Biju Das wrote:
> > > > > > > Hi Maxime Ripard,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > -Original Message-
> > > > > > > > From: Maxime Ripard
> > > > > > > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > > > > > > To: Biju Das
> > > > > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU
> > > > > > > > Support
> > > > > > > >
> > > > > > > > > + */
> > > > > > > > > + rzg2l_du_crtc_get(rcrtc);
> > > > > > > >
> > > > > > > > That's a bit suspicious. Have you looked at
> > > > > > > > drm_atomic_helper_commit_tail_rpm() ?
> > > > > > >
> > > > > > > Ok, I will drop this and start using
> > > > > > > drm_atomic_helper_commit_tail_rpm()
> > > > > > > instead of rzg2l_du_atomic_commit_tail().
> > > > > >
> > > > > > It was more of a suggestion, really. I'm not sure whether it
> > > > > > works for you, but it usually addresses similar issues in
> drivers.
> > > > >
> > > > > I confirm, it is working in my case, even after removing
> > > > > rzg2l_du_crtc_get() and using the helper function
> > > > drm_atomic_helper_commit_tail_rpm().
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > > +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###
> > >
> >
> > Shall I send V16 now as I am going for xmas vacation or Do you prefer
> > to wait?
>
> Waiting is fine, most of us are going to be in holidays too so you won't
> get any reviews either :)
Agreed.
Cheers,
Biju
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Fri, Dec 15, 2023 at 01:25:48PM +, Biju Das wrote: > Hi Maxime Ripard, > > > -Original Message- > > From: Maxime Ripard > > Sent: Friday, December 15, 2023 10:24 AM > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote: > > > Hi Maxime Ripard, > > > > > > Thanks for the feedback. > > > > Thanks, that's super helpful. The architecture is thus similar to vc4 > > > > Some general questions related to bugs we had at some point with vc4: > > > > * Where is the display list stored? In RAM or in a dedicated SRAM? > > [1] It is in DDR (RAM). > > > > > * Are the pointer to the current display list latched? > > > > * Is the display list itself latched? If it's not, what happens when > > the display list is changed while the frame is being generated? > > There is some protocol defined for SW side and HW side for updating display > list > See [1] 33.4.8.1 Operation flow of VSPD and DU. > > All the display list operations are manged here[2] > > [1] > https://www.renesas.com/us/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0 > > [2] > https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/media/platform/renesas/vsp1/vsp1_dl.c#L863 I'm sorry, but I'm not going to read a 3500+ to try to figure it out. Could you answer the questions above? Maxime signature.asc Description: PGP signature
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Fri, Dec 15, 2023 at 02:19:25PM +, Biju Das wrote:
> Hi Maxime Ripard,
>
> > -Original Message-
> > From: Biju Das
> > Sent: Friday, December 15, 2023 1:52 PM
> > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > Hi Maxime Ripard,
> >
> > > -Original Message-
> > > From: Maxime Ripard
> > > Sent: Friday, December 15, 2023 12:58 PM
> > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > On Fri, Dec 15, 2023 at 11:37:07AM +, Biju Das wrote:
> > > > Hi Maxime Ripard,
> > > >
> > > > > -Original Message-----
> > > > > From: Maxime Ripard
> > > > > Sent: Friday, December 15, 2023 10:24 AM
> > > > > To: Biju Das
> > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > > >
> > > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote:
> > > > > > Hi Maxime Ripard,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > -Original Message-
> > > > > > > From: Maxime Ripard
> > > > > > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > > > > > To: Biju Das
> > > > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU
> > > > > > > Support
> > > > > > >
> > > > > > > > +*/
> > > > > > > > + rzg2l_du_crtc_get(rcrtc);
> > > > > > >
> > > > > > > That's a bit suspicious. Have you looked at
> > > > > > > drm_atomic_helper_commit_tail_rpm() ?
> > > > > >
> > > > > > Ok, I will drop this and start using
> > > > > > drm_atomic_helper_commit_tail_rpm()
> > > > > > instead of rzg2l_du_atomic_commit_tail().
> > > > >
> > > > > It was more of a suggestion, really. I'm not sure whether it works
> > > > > for you, but it usually addresses similar issues in drivers.
> > > >
> > > > I confirm, it is working in my case, even after removing
> > > > rzg2l_du_crtc_get() and using the helper function
> > > drm_atomic_helper_commit_tail_rpm().
> > > >
> > > > >
> > > > > > >
> > > > > > > > +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###
> >
>
> Shall I send V16 now as I am going for xmas vacation or
> Do you prefer to wait?
Waiting is fine, most of us are going to be in holidays too so you won't
get any reviews either :)
Maxime
signature.asc
Description: PGP signature
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Biju Das
> Sent: Friday, December 15, 2023 1:52 PM
> Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> Hi Maxime Ripard,
>
> > -Original Message-
> > From: Maxime Ripard
> > Sent: Friday, December 15, 2023 12:58 PM
> > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > On Fri, Dec 15, 2023 at 11:37:07AM +, Biju Das wrote:
> > > Hi Maxime Ripard,
> > >
> > > > -Original Message-
> > > > From: Maxime Ripard
> > > > Sent: Friday, December 15, 2023 10:24 AM
> > > > To: Biju Das
> > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > >
> > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote:
> > > > > Hi Maxime Ripard,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > -Original Message-
> > > > > > From: Maxime Ripard
> > > > > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > > > > To: Biju Das
> > > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU
> > > > > > Support
> > > > > >
> > > > > > > + */
> > > > > > > + rzg2l_du_crtc_get(rcrtc);
> > > > > >
> > > > > > That's a bit suspicious. Have you looked at
> > > > > > drm_atomic_helper_commit_tail_rpm() ?
> > > > >
> > > > > Ok, I will drop this and start using
> > > > > drm_atomic_helper_commit_tail_rpm()
> > > > > instead of rzg2l_du_atomic_commit_tail().
> > > >
> > > > It was more of a suggestion, really. I'm not sure whether it works
> > > > for you, but it usually addresses similar issues in drivers.
> > >
> > > I confirm, it is working in my case, even after removing
> > > rzg2l_du_crtc_get() and using the helper function
> > drm_atomic_helper_commit_tail_rpm().
> > >
> > > >
> > > > > >
> > > > > > > +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###
>
Shall I send V16 now as I am going for xmas vacation or
Do you prefer to wait?
Cheers,
Biju
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Fri, Dec 15, 2023 at 01:52:28PM +, 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.
Maxime
signature.asc
Description: PGP signature
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Maxime Ripard
> Sent: Friday, December 15, 2023 12:58 PM
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> On Fri, Dec 15, 2023 at 11:37:07AM +, Biju Das wrote:
> > Hi Maxime Ripard,
> >
> > > -Original Message-
> > > From: Maxime Ripard
> > > Sent: Friday, December 15, 2023 10:24 AM
> > > To: Biju Das
> > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote:
> > > > Hi Maxime Ripard,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > -----Original Message-----
> > > > > From: Maxime Ripard
> > > > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > > > To: Biju Das
> > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > > >
> > > > > > +*/
> > > > > > + rzg2l_du_crtc_get(rcrtc);
> > > > >
> > > > > That's a bit suspicious. Have you looked at
> > > > > drm_atomic_helper_commit_tail_rpm() ?
> > > >
> > > > Ok, I will drop this and start using
> > > > drm_atomic_helper_commit_tail_rpm()
> > > > instead of rzg2l_du_atomic_commit_tail().
> > >
> > > It was more of a suggestion, really. I'm not sure whether it works
> > > for you, but it usually addresses similar issues in drivers.
> >
> > I confirm, it is working in my case, even after removing
> > rzg2l_du_crtc_get() and using the helper function
> drm_atomic_helper_commit_tail_rpm().
> >
> > >
> > > > >
> > > > > > +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###
Cheers,
Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard, > -Original Message- > From: Maxime Ripard > Sent: Friday, December 15, 2023 10:24 AM > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote: > > Hi Maxime Ripard, > > > > Thanks for the feedback. > > Thanks, that's super helpful. The architecture is thus similar to vc4 > > Some general questions related to bugs we had at some point with vc4: > > * Where is the display list stored? In RAM or in a dedicated SRAM? [1] It is in DDR (RAM). > > * Are the pointer to the current display list latched? > > * Is the display list itself latched? If it's not, what happens when > the display list is changed while the frame is being generated? There is some protocol defined for SW side and HW side for updating display list See [1] 33.4.8.1 Operation flow of VSPD and DU. All the display list operations are manged here[2] [1] https://www.renesas.com/us/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0 [2] https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/media/platform/renesas/vsp1/vsp1_dl.c#L863 Cheers, Biju
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Fri, Dec 15, 2023 at 11:37:07AM +, Biju Das wrote:
> Hi Maxime Ripard,
>
> > -Original Message-
> > From: Maxime Ripard
> > Sent: Friday, December 15, 2023 10:24 AM
> > To: Biju Das
> > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote:
> > > Hi Maxime Ripard,
> > >
> > > Thanks for the feedback.
> > >
> > > > -Original Message-
> > > > From: Maxime Ripard
> > > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > > To: Biju Das
> > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > >
> > > > > + */
> > > > > + rzg2l_du_crtc_get(rcrtc);
> > > >
> > > > That's a bit suspicious. Have you looked at
> > > > drm_atomic_helper_commit_tail_rpm() ?
> > >
> > > Ok, I will drop this and start using
> > > drm_atomic_helper_commit_tail_rpm()
> > > instead of rzg2l_du_atomic_commit_tail().
> >
> > It was more of a suggestion, really. I'm not sure whether it works for
> > you, but it usually addresses similar issues in drivers.
>
> I confirm, it is working in my case, even after removing rzg2l_du_crtc_get()
> and using the helper function drm_atomic_helper_commit_tail_rpm().
>
> >
> > > >
> > > > > +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?
Maxime
signature.asc
Description: PGP signature
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Maxime Ripard
> Sent: Friday, December 15, 2023 10:24 AM
> To: Biju Das
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote:
> > Hi Maxime Ripard,
> >
> > Thanks for the feedback.
> >
> > > -Original Message-
> > > From: Maxime Ripard
> > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > To: Biju Das
> > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > > +*/
> > > > + rzg2l_du_crtc_get(rcrtc);
> > >
> > > That's a bit suspicious. Have you looked at
> > > drm_atomic_helper_commit_tail_rpm() ?
> >
> > Ok, I will drop this and start using
> > drm_atomic_helper_commit_tail_rpm()
> > instead of rzg2l_du_atomic_commit_tail().
>
> It was more of a suggestion, really. I'm not sure whether it works for
> you, but it usually addresses similar issues in drivers.
I confirm, it is working in my case, even after removing rzg2l_du_crtc_get()
and using the helper function drm_atomic_helper_commit_tail_rpm().
>
> > >
> > > > +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);
}
Cheers,
Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Maxime Ripard
> Sent: Friday, December 15, 2023 9:25 AM
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> On Fri, Dec 15, 2023 at 07:47:07AM +, Biju Das wrote:
> > Hi Maxime Ripard,
> >
> > > -Original Message-
> > > From: Biju Das
> > > Sent: Thursday, December 14, 2023 8:50 PM
> > > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > Hi Maxime Ripard,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Biju Das
> > > > Sent: Thursday, December 14, 2023 3:24 PM
> > > > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > >
> > > > >
> > > > > > +
> > > > > > + for (i = 0; i < num_planes; ++i) {
> > > > > > + enum drm_plane_type type = i < num_crtcs
> > > > > > +? DRM_PLANE_TYPE_PRIMARY
> > > > > > +: DRM_PLANE_TYPE_OVERLAY;
> > > > > > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > > > > > +
> > > > > > + plane->vsp = vsp;
> > > > > > + plane->index = i;
> > > > > > + ret = drm_universal_plane_init(&rcdu->ddev, &plane-
> > > >plane,
> > > > > > + crtcs,
> > > &rzg2l_du_vsp_plane_funcs,
> > > > > > + rzg2l_du_vsp_formats,
> > > > > > +
> > > ARRAY_SIZE(rzg2l_du_vsp_formats),
> > > > > > + NULL, type, NULL);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > >
> > > > > you need to use drmm variant here too.
> > > >
> > > > I did rebased to latest drm_misc_next and I don't find the
> > > > drmm_universal_plane_init()
> > > >
> > > > Can you please point me to the API?
> > >
> > > We cannot use drmm_universal_plane_alloc() in this architecture.
> > >
> > > rzg2l_du_vsps_init() stores the VSP pointer and pipe index from DT
> first.
> > >
> > > Then all the planes are created using rzg2l_du_vsp_init()
> > >
> > > CRTC uses VSP pointer and pipe_index to set the
> > > plane(rzg2l_du_crtc_create()).
> > >
> > > CRTC->vsp->planes[rcrtc->vsp_pipe].plane
> > >
> >
> > Thinking again, it is possible to use drmm_universal_plane_alloc()
> > here
> >
> > Introduce a linked list [1] and use an API [2] to retrieve plane by
> > matching vsp_pipe index against plane->index.
> >
> > With this we don't need drmm_kcalloc() any more.
> >
> > I will implement and test this and send v16, if there are no comments.
> >
> >
> > [1]
> > struct rzg2l_du_vsp_plane {
> > struct drm_plane plane;
> > struct rzg2l_du_vsp *vsp;
> > unsigned int index;
> > struct rzg2l_du_vsp_plane *next;
> > };
> >
> > [2]
> > struct drm_plane *rzg2l_du_vsp_get_drm_plane(struct rzg2l_du_crtc *crtc,
> > unsigned int pipe_index);
>
> Why do you need a linked list for? There's one already: the DRM device
> will store all the planes registered in the DRM device, and you can
> already iterate over all the planes with drm_for_each_plane, or
> drm_for_each_plane_mask if you want to reduce the scope of the iterator.
Thanks for the pointer, it is simple now.
Vsp->planes[] aswell as linked list is not required using drm_for_each_plane()
Cheers,
Biju
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote:
> Hi Maxime Ripard,
>
> Thanks for the feedback.
>
> > -Original Message-
> > From: Maxime Ripard
> > Sent: Wednesday, December 13, 2023 3:47 PM
> > To: Biju Das
> > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote:
> > > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > > Video Signal Processor (VSPD), and Display Unit (DU).
> > >
> > > It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> > > along with 2 RPFs to support the blending of two picture layers and
> > > raster operations (ROPs).
> > >
> > > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> > > alike SoCs.
> > >
> > > Signed-off-by: Biju Das
> >
> > I'd still like a review from Geert or Laurent, I don't know the SoC.
>
> Since Laurent is super busy, maybe Kieran and Jacopo can provide feedback if
> any.
>
> The display blocks is shown in [1] and the pipe line is as below
>
> Memory-> fcpvd -->VSPD --> DU --> DSI --> Display panel.
>
> Fcpvd: Frame Compression Processor
> VSPD: Video Signal Processor, Basically a blitter engine which directly
> render images to DU
> DU: Display Unit.
>
> On R-Car fpvcd, VSPD and (DU with planes) IPs are separate blocks
> whereas here it is integrated inside LCDC.
>
> fcpvd and VSPD are in media subsystem and we are reusing the code here.
> The vspd is based on display list, it downloads the register contents from
> linked list in memory
> and execute composite operation and generates interrupts for display start
> and frame end.
>
> du_vsp registers the frame completion callback with vspd driver in media
> framework.
> and we call the drm_crtc_handle_vblank() in this context.
>
> [1]
> https://pasteboard.co/MDmbXdK3psSD.png
>
> ● FCPVD
> − Support out-of-order for the whole outstanding transactions
> − Read linear addressing image data
> − Read display list data
> − Write image data
> ● VSPD
> − Supports various data formats and conversion
> − Supports YCbCr444/422/420, RGB, α RGB, α plane
> − Color space conversion and changes to the number of colors by dithering
> − Color keying
> − Supports combination between pixel alpha and global alpha
> − Supports generating pre multiplied alpha
> − Video processing
> − Blending of two picture layers and raster operations (ROPs)
> − Clipping
> − 1D look up table
> − Vertical flipping in case of output to memory
> − Direct connection to display module
> − Supporting 1920 pixels in horizontal direction
> − Writing back image data which is transferred to Display Unit (DU) to memory
> ● DU
> − Supporting Display Parallel Interface (DPI) and MIPI LINK Video Interface
> − Display timing master
> − Generating video timings (Front porch, Back porch, Sync active, Active
> video area)
> − Selecting the polarity of output DCLK, HSYNC, VSYNC, and DE
> − Supporting Progressive (Non-interlace)
> − Not supports Interlace
> − Input data format (from VSPD): RGB888, RGB666 (not supports dithering of
> RGB565)
> − Output data format: same as Input data format
> − Supporting Full HD (1920 pixels × 1080 lines) for MIPI-DSI Output
> − Supporting WXGA (1280 pixels × 800 lines) for Parallel Output
Thanks, that's super helpful. The architecture is thus similar to vc4
Some general questions related to bugs we had at some point with vc4:
* Where is the display list stored? In RAM or in a dedicated SRAM?
* Are the pointer to the current display list latched?
* Is the display list itself latched? If it's not, what happens when
the display list is changed while the frame is being generated?
> >
> > > +static int rzg2l_du_crtc_get(struct rzg2l_du_crtc *rcrtc) {
> > > + int ret;
> > > +
> > > + /*
> > > + * Guard against double-get, as the function is called from both the
> > > + * .atomic_enable() and .atomic_flush() handlers.
> > > + */
> > > + if (rcrtc->initialized)
> > > + return 0;
> > > +
> > > + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.aclk);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.pclk);
> > > + if (ret < 0)
> > > + goto error_bus_clock;
> > > +
> > > + ret = reset_control_deassert(rcrtc->rstc);
> > > + if (ret < 0)
> > > + goto error_peri_clock;
&
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Fri, Dec 15, 2023 at 07:47:07AM +, Biju Das wrote:
> Hi Maxime Ripard,
>
> > -Original Message-
> > From: Biju Das
> > Sent: Thursday, December 14, 2023 8:50 PM
> > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > Hi Maxime Ripard,
> >
> >
> > > -Original Message-
> > > From: Biju Das
> > > Sent: Thursday, December 14, 2023 3:24 PM
> > > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > >
> > > > > +
> > > > > + for (i = 0; i < num_planes; ++i) {
> > > > > + enum drm_plane_type type = i < num_crtcs
> > > > > + ? DRM_PLANE_TYPE_PRIMARY
> > > > > + : DRM_PLANE_TYPE_OVERLAY;
> > > > > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > > > > +
> > > > > + plane->vsp = vsp;
> > > > > + plane->index = i;
> > > > > + ret = drm_universal_plane_init(&rcdu->ddev, &plane-
> > >plane,
> > > > > +crtcs,
> > &rzg2l_du_vsp_plane_funcs,
> > > > > +rzg2l_du_vsp_formats,
> > > > > +
> > ARRAY_SIZE(rzg2l_du_vsp_formats),
> > > > > +NULL, type, NULL);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > >
> > > > you need to use drmm variant here too.
> > >
> > > I did rebased to latest drm_misc_next and I don't find the
> > > drmm_universal_plane_init()
> > >
> > > Can you please point me to the API?
> >
> > We cannot use drmm_universal_plane_alloc() in this architecture.
> >
> > rzg2l_du_vsps_init() stores the VSP pointer and pipe index from DT first.
> >
> > Then all the planes are created using rzg2l_du_vsp_init()
> >
> > CRTC uses VSP pointer and pipe_index to set the
> > plane(rzg2l_du_crtc_create()).
> >
> > CRTC->vsp->planes[rcrtc->vsp_pipe].plane
> >
>
> Thinking again, it is possible to use drmm_universal_plane_alloc() here
>
> Introduce a linked list [1] and use an API [2] to retrieve plane
> by matching vsp_pipe index against plane->index.
>
> With this we don't need drmm_kcalloc() any more.
>
> I will implement and test this and send v16, if there are no comments.
>
>
> [1]
> struct rzg2l_du_vsp_plane {
> struct drm_plane plane;
> struct rzg2l_du_vsp *vsp;
> unsigned int index;
> struct rzg2l_du_vsp_plane *next;
> };
>
> [2]
> struct drm_plane *rzg2l_du_vsp_get_drm_plane(struct rzg2l_du_crtc *crtc,
>unsigned int pipe_index);
Why do you need a linked list for? There's one already: the DRM device
will store all the planes registered in the DRM device, and you can
already iterate over all the planes with drm_for_each_plane, or
drm_for_each_plane_mask if you want to reduce the scope of the iterator.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Thu, Dec 14, 2023 at 08:50:14PM +, Biju Das wrote:
> Hi Maxime Ripard,
>
>
> > -Original Message-
> > From: Biju Das
> > Sent: Thursday, December 14, 2023 3:24 PM
> > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > >
> > > > +
> > > > + for (i = 0; i < num_planes; ++i) {
> > > > + enum drm_plane_type type = i < num_crtcs
> > > > +? DRM_PLANE_TYPE_PRIMARY
> > > > +: DRM_PLANE_TYPE_OVERLAY;
> > > > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > > > +
> > > > + plane->vsp = vsp;
> > > > + plane->index = i;
> > > > + ret = drm_universal_plane_init(&rcdu->ddev,
> > > > &plane->plane,
> > > > + crtcs,
> > > > &rzg2l_du_vsp_plane_funcs,
> > > > + rzg2l_du_vsp_formats,
> > > > +
> > > > ARRAY_SIZE(rzg2l_du_vsp_formats),
> > > > + NULL, type, NULL);
> > > > + if (ret < 0)
> > > > + return ret;
> > >
> > > you need to use drmm variant here too.
> >
> > I did rebased to latest drm_misc_next and I don't find the
> > drmm_universal_plane_init()
> >
> > Can you please point me to the API?
>
> We cannot use drmm_universal_plane_alloc() in this architecture.
>
> rzg2l_du_vsps_init() stores the VSP pointer and pipe index from DT first.
>
> Then all the planes are created using rzg2l_du_vsp_init()
>
> CRTC uses VSP pointer and pipe_index to set the plane(rzg2l_du_crtc_create()).
>
> CRTC->vsp->planes[rcrtc->vsp_pipe].plane
Actually, I don't think you need vsp->planes at all. The only real use
you have for it is to find the primary plane in rzg2l_du_crtc_create()
and you could just pass it as an argument to the function, and make
rzg2l_du_vsps_init return it for example (or fill a pointer on success,
or something).
And I also don't think you need to store the crtcs either, you can
probably just get away with storing the vsp pointer in the crtc when you
allocate it.
Maxime
signature.asc
Description: PGP signature
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Biju Das
> Sent: Thursday, December 14, 2023 8:50 PM
> Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> Hi Maxime Ripard,
>
>
> > -Original Message-
> > From: Biju Das
> > Sent: Thursday, December 14, 2023 3:24 PM
> > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> >
> > >
> > > > +
> > > > + for (i = 0; i < num_planes; ++i) {
> > > > + enum drm_plane_type type = i < num_crtcs
> > > > +? DRM_PLANE_TYPE_PRIMARY
> > > > +: DRM_PLANE_TYPE_OVERLAY;
> > > > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > > > +
> > > > + plane->vsp = vsp;
> > > > + plane->index = i;
> > > > + ret = drm_universal_plane_init(&rcdu->ddev, &plane-
> >plane,
> > > > + crtcs,
> &rzg2l_du_vsp_plane_funcs,
> > > > + rzg2l_du_vsp_formats,
> > > > +
> ARRAY_SIZE(rzg2l_du_vsp_formats),
> > > > + NULL, type, NULL);
> > > > + if (ret < 0)
> > > > + return ret;
> > >
> > > you need to use drmm variant here too.
> >
> > I did rebased to latest drm_misc_next and I don't find the
> > drmm_universal_plane_init()
> >
> > Can you please point me to the API?
>
> We cannot use drmm_universal_plane_alloc() in this architecture.
>
> rzg2l_du_vsps_init() stores the VSP pointer and pipe index from DT first.
>
> Then all the planes are created using rzg2l_du_vsp_init()
>
> CRTC uses VSP pointer and pipe_index to set the
> plane(rzg2l_du_crtc_create()).
>
> CRTC->vsp->planes[rcrtc->vsp_pipe].plane
>
Thinking again, it is possible to use drmm_universal_plane_alloc() here
Introduce a linked list [1] and use an API [2] to retrieve plane
by matching vsp_pipe index against plane->index.
With this we don't need drmm_kcalloc() any more.
I will implement and test this and send v16, if there are no comments.
[1]
struct rzg2l_du_vsp_plane {
struct drm_plane plane;
struct rzg2l_du_vsp *vsp;
unsigned int index;
struct rzg2l_du_vsp_plane *next;
};
[2]
struct drm_plane *rzg2l_du_vsp_get_drm_plane(struct rzg2l_du_crtc *crtc,
unsigned int pipe_index);
Cheers,
Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
> -Original Message-
> From: Biju Das
> Sent: Thursday, December 14, 2023 3:24 PM
> Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> >
> > > +
> > > + for (i = 0; i < num_planes; ++i) {
> > > + enum drm_plane_type type = i < num_crtcs
> > > + ? DRM_PLANE_TYPE_PRIMARY
> > > + : DRM_PLANE_TYPE_OVERLAY;
> > > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > > +
> > > + plane->vsp = vsp;
> > > + plane->index = i;
> > > + ret = drm_universal_plane_init(&rcdu->ddev, &plane->plane,
> > > +crtcs, &rzg2l_du_vsp_plane_funcs,
> > > +rzg2l_du_vsp_formats,
> > > +ARRAY_SIZE(rzg2l_du_vsp_formats),
> > > +NULL, type, NULL);
> > > + if (ret < 0)
> > > + return ret;
> >
> > you need to use drmm variant here too.
>
> I did rebased to latest drm_misc_next and I don't find the
> drmm_universal_plane_init()
>
> Can you please point me to the API?
We cannot use drmm_universal_plane_alloc() in this architecture.
rzg2l_du_vsps_init() stores the VSP pointer and pipe index from DT first.
Then all the planes are created using rzg2l_du_vsp_init()
CRTC uses VSP pointer and pipe_index to set the plane(rzg2l_du_crtc_create()).
CRTC->vsp->planes[rcrtc->vsp_pipe].plane
Cheers,
Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard, > -Original Message- > From: Biju Das > Sent: Thursday, December 14, 2023 3:24 PM > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > . > > > + > > > + vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL); > > > + if (!vsp->planes) > > > + return -ENOMEM; > > > > drmm_kcalloc or drmm_kmalloc_array > > I get crashes at random places, if I use drmm_kcalloc(). > Not sure it is related to this? There is no crash if I use kcalloc. The random crash is fixed it is due to double free. I forgot to remove kfree in clean up function. Cheers, Biju
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime Ripard,
Thanks for the feedback.
> -Original Message-
> From: Maxime Ripard
> Sent: Wednesday, December 13, 2023 3:47 PM
> To: Biju Das
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
>
> On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> > along with 2 RPFs to support the blending of two picture layers and
> > raster operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> > alike SoCs.
> >
> > Signed-off-by: Biju Das
>
> I'd still like a review from Geert or Laurent, I don't know the SoC.
Since Laurent is super busy, maybe Kieran and Jacopo can provide feedback if
any.
The display blocks is shown in [1] and the pipe line is as below
Memory-> fcpvd -->VSPD --> DU --> DSI --> Display panel.
Fcpvd: Frame Compression Processor
VSPD: Video Signal Processor, Basically a blitter engine which directly render
images to DU
DU: Display Unit.
On R-Car fpvcd, VSPD and (DU with planes) IPs are separate blocks
whereas here it is integrated inside LCDC.
fcpvd and VSPD are in media subsystem and we are reusing the code here.
The vspd is based on display list, it downloads the register contents from
linked list in memory
and execute composite operation and generates interrupts for display start and
frame end.
du_vsp registers the frame completion callback with vspd driver in media
framework.
and we call the drm_crtc_handle_vblank() in this context.
[1]
https://pasteboard.co/MDmbXdK3psSD.png
● FCPVD
− Support out-of-order for the whole outstanding transactions
− Read linear addressing image data
− Read display list data
− Write image data
● VSPD
− Supports various data formats and conversion
− Supports YCbCr444/422/420, RGB, α RGB, α plane
− Color space conversion and changes to the number of colors by dithering
− Color keying
− Supports combination between pixel alpha and global alpha
− Supports generating pre multiplied alpha
− Video processing
− Blending of two picture layers and raster operations (ROPs)
− Clipping
− 1D look up table
− Vertical flipping in case of output to memory
− Direct connection to display module
− Supporting 1920 pixels in horizontal direction
− Writing back image data which is transferred to Display Unit (DU) to memory
● DU
− Supporting Display Parallel Interface (DPI) and MIPI LINK Video Interface
− Display timing master
− Generating video timings (Front porch, Back porch, Sync active, Active video
area)
− Selecting the polarity of output DCLK, HSYNC, VSYNC, and DE
− Supporting Progressive (Non-interlace)
− Not supports Interlace
− Input data format (from VSPD): RGB888, RGB666 (not supports dithering of
RGB565)
− Output data format: same as Input data format
− Supporting Full HD (1920 pixels × 1080 lines) for MIPI-DSI Output
− Supporting WXGA (1280 pixels × 800 lines) for Parallel Output
>
> > +static int rzg2l_du_crtc_get(struct rzg2l_du_crtc *rcrtc) {
> > + int ret;
> > +
> > + /*
> > +* Guard against double-get, as the function is called from both the
> > +* .atomic_enable() and .atomic_flush() handlers.
> > +*/
> > + if (rcrtc->initialized)
> > + return 0;
> > +
> > + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.aclk);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.pclk);
> > + if (ret < 0)
> > + goto error_bus_clock;
> > +
> > + ret = reset_control_deassert(rcrtc->rstc);
> > + if (ret < 0)
> > + goto error_peri_clock;
> > +
> > + rzg2l_du_crtc_setup(rcrtc);
> > + rcrtc->initialized = true;
> > +
> > + return 0;
> > +
> > +error_peri_clock:
> > + clk_disable_unprepare(rcrtc->rzg2l_clocks.pclk);
> > +error_bus_clock:
> > + clk_disable_unprepare(rcrtc->rzg2l_clocks.aclk);
> > + return ret;
> > +}
>
> [...]
>
> > +static void rzg2l_du_crtc_atomic_flush(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state) {
> > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > + struct drm_device *dev = rcrtc->crtc.dev;
> > + unsigned long flags;
> > +
> > + WARN_ON(!crtc->state->enable);
> > +
> > + /*
> > +* If a mode set is in progress we can be called with the CRTC
> disabled.
> > +* We thus need to first get and setup the CRTC in order to
> configure
> > +
RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Laurent and Maxime, > -Original Message- > From: Laurent Pinchart > Sent: Wednesday, December 13, 2023 3:51 PM > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > On Wed, Dec 13, 2023 at 04:47:09PM +0100, Maxime Ripard wrote: > > On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote: > > > The LCD controller is composed of Frame Compression Processor > > > (FCPVD), Video Signal Processor (VSPD), and Display Unit (DU). > > > > > > It has DPI/DSI interfaces and supports a maximum resolution of 1080p > > > along with 2 RPFs to support the blending of two picture layers and > > > raster operations (ROPs). > > > > > > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L > > > alike SoCs. > > > > > > Signed-off-by: Biju Das > > > > I'd still like a review from Geert or Laurent, I don't know the SoC. > > I won't have time soon, and this driver has been blocked for way too long > due to that :-S It is blocked for more than year now. FYI, All the bits and pieces of this display pipeline (FCP, VSPD, DSI, ADV7535) is mainlined and then backported to linux 5.10.y-cip[1] and linux-6.1.y-cip[2] along with MALI GPU and MESA support. [1] https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/log/?h=linux-5.10.y-cip [2] https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/log/?h=linux-6.1.y-cip [3] https://github.com/renesas-rz/meta-rz-panfrost/blob/main/recipes-graphics/mesa/files/0001-kmsro-Add-rcar-du-entry-point-for-Renesas-RZ-G2Lx-se.patch Only DU driver is pending for completion for RZ/G2L family of SoCs. Cheers, Biju
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Wed, Dec 13, 2023 at 4:50 PM Laurent Pinchart
wrote:
> On Wed, Dec 13, 2023 at 04:47:09PM +0100, Maxime Ripard wrote:
> > On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote:
> > > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > > Video Signal Processor (VSPD), and Display Unit (DU).
> > >
> > > It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> > > along with 2 RPFs to support the blending of two picture layers and
> > > raster operations (ROPs).
> > >
> > > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> > > alike SoCs.
> > >
> > > Signed-off-by: Biju Das
> >
> > I'd still like a review from Geert or Laurent, I don't know the SoC.
I don't consider myself sufficiently familiar with DRM and the DU.
Kieran?
> I won't have time soon, and this driver has been blocked for way too
> long due to that :-S
Indeed...
[deleted 278 lines of needlessly quoted text]
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Wed, Dec 13, 2023 at 04:47:09PM +0100, Maxime Ripard wrote:
> On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> > along with 2 RPFs to support the blending of two picture layers and
> > raster operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> > alike SoCs.
> >
> > Signed-off-by: Biju Das
>
> I'd still like a review from Geert or Laurent, I don't know the SoC.
I won't have time soon, and this driver has been blocked for way too
long due to that :-S
> > +static int rzg2l_du_crtc_get(struct rzg2l_du_crtc *rcrtc)
> > +{
> > + int ret;
> > +
> > + /*
> > +* Guard against double-get, as the function is called from both the
> > +* .atomic_enable() and .atomic_flush() handlers.
> > +*/
> > + if (rcrtc->initialized)
> > + return 0;
> > +
> > + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.aclk);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.pclk);
> > + if (ret < 0)
> > + goto error_bus_clock;
> > +
> > + ret = reset_control_deassert(rcrtc->rstc);
> > + if (ret < 0)
> > + goto error_peri_clock;
> > +
> > + rzg2l_du_crtc_setup(rcrtc);
> > + rcrtc->initialized = true;
> > +
> > + return 0;
> > +
> > +error_peri_clock:
> > + clk_disable_unprepare(rcrtc->rzg2l_clocks.pclk);
> > +error_bus_clock:
> > + clk_disable_unprepare(rcrtc->rzg2l_clocks.aclk);
> > + return ret;
> > +}
>
> [...]
>
> > +static void rzg2l_du_crtc_atomic_flush(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state)
> > +{
> > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > + struct drm_device *dev = rcrtc->crtc.dev;
> > + unsigned long flags;
> > +
> > + WARN_ON(!crtc->state->enable);
> > +
> > + /*
> > +* If a mode set is in progress we can be called with the CRTC disabled.
> > +* We thus need to first get and setup the CRTC in order to configure
> > +* planes. We must *not* put the CRTC, as it must be kept awake until
> > +* the .atomic_enable() call that will follow. The get operation in
> > +* .atomic_enable() will in that case be a no-op, and the CRTC will be
> > +* put later in .atomic_disable().
> > +*/
> > + rzg2l_du_crtc_get(rcrtc);
>
> That's a bit suspicious. Have you looked at
> drm_atomic_helper_commit_tail_rpm() ?
>
> > +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
>
> > +int rzg2l_du_crtc_create(struct rzg2l_du_device *rcdu)
> > +{
> > + struct rzg2l_du_crtc *rcrtc = &rcdu->crtcs[0];
> > + struct drm_crtc *crtc = &rcrtc->crtc;
> > + struct drm_plane *primary;
> > + int ret;
> > +
> > + rcrtc->rstc = devm_reset_control_get_shared(rcdu->dev, NULL);
> > + if (IS_ERR(rcrtc->rstc)) {
> > + dev_err(rcdu->dev, "can't get cpg reset\n");
> > + return PTR_ERR(rcrtc->rstc);
> > + }
> > +
> > + rcrtc->rzg2l_clocks.aclk = devm_clk_get(rcdu->dev, "aclk");
> > + if (IS_ERR(rcrtc->rzg2l_clocks.aclk)) {
> > + dev_err(rcdu->dev, "no axi clock for DU\n");
> > + return PTR_ERR(rcrtc->rzg2l_clocks.aclk);
> > + }
> > +
> > + rcrtc->rzg2l_clocks.pclk = devm_clk_get(rcdu->dev, "pclk");
> > + if (IS_ERR(rcrtc->rzg2l_clocks.pclk)) {
> > + dev_err(rcdu->dev, "no peripheral clock for DU\n");
> > + return PTR_ERR(rcrtc->rzg2l_clocks.pclk);
> > + }
> > +
> > + rcrtc->rzg2l_clocks.dclk = devm_clk_get(rcdu->dev, "vclk");
> > + if (IS_ERR(rcrtc->rzg2l_clocks.dclk)) {
> > + dev_err(rcdu->dev, "no video clock for DU\n");
> > + return PTR_ERR(rcrtc->rzg2l_clocks.dclk);
> > + }
> > +
> > + init_waitqueue_head(&rcrtc->flip_wait);
> > + rcrtc->dev = rcdu;
> > +
> > + primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> > +
> > + ret = drm_crtc_init_with_planes(&rcdu->ddev, crtc, primary, NULL,
> > + &crtc_funcs_rz, NULL);
> > + if (ret < 0)
> > + return ret;
>
> You should use the drmm variant here
>
> > +static void rzg2l_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > +{
> > + struct drm_device *dev = old_state->dev;
> > +
> > + /* Apply the atomic update. */
> > + drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > + drm_atomic_helper_commit_planes(dev, old_state,
> > +
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Wed, Dec 13, 2023 at 04:47:09PM +0100, Maxime Ripard wrote:
> On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote:
> > +int rzg2l_du_vsp_init(struct rzg2l_du_vsp *vsp, struct device_node *np,
> > + unsigned int crtcs)
> > +{
> > + struct rzg2l_du_device *rcdu = vsp->dev;
> > + struct platform_device *pdev;
> > + unsigned int num_crtcs = hweight32(crtcs);
> > + unsigned int num_planes = 2;
> > + unsigned int i;
> > + int ret;
> > +
> > + /* Find the VSP device and initialize it. */
> > + pdev = of_find_device_by_node(np);
> > + if (!pdev)
> > + return -ENXIO;
> > +
> > + vsp->vsp = &pdev->dev;
> > +
> > + ret = drmm_add_action_or_reset(&rcdu->ddev, rzg2l_du_vsp_cleanup, vsp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = vsp1_du_init(vsp->vsp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
> > + if (!vsp->planes)
> > + return -ENOMEM;
>
> drmm_kcalloc or drmm_kmalloc_array
Also, it doesn't look like you're using that array anywhere once the
action is gone.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
On Tue, Nov 28, 2023 at 10:51:27AM +, Biju Das wrote:
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
>
> It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> along with 2 RPFs to support the blending of two picture layers and
> raster operations (ROPs).
>
> The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> alike SoCs.
>
> Signed-off-by: Biju Das
I'd still like a review from Geert or Laurent, I don't know the SoC.
> +static int rzg2l_du_crtc_get(struct rzg2l_du_crtc *rcrtc)
> +{
> + int ret;
> +
> + /*
> + * Guard against double-get, as the function is called from both the
> + * .atomic_enable() and .atomic_flush() handlers.
> + */
> + if (rcrtc->initialized)
> + return 0;
> +
> + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.aclk);
> + if (ret < 0)
> + return ret;
> +
> + ret = clk_prepare_enable(rcrtc->rzg2l_clocks.pclk);
> + if (ret < 0)
> + goto error_bus_clock;
> +
> + ret = reset_control_deassert(rcrtc->rstc);
> + if (ret < 0)
> + goto error_peri_clock;
> +
> + rzg2l_du_crtc_setup(rcrtc);
> + rcrtc->initialized = true;
> +
> + return 0;
> +
> +error_peri_clock:
> + clk_disable_unprepare(rcrtc->rzg2l_clocks.pclk);
> +error_bus_clock:
> + clk_disable_unprepare(rcrtc->rzg2l_clocks.aclk);
> + return ret;
> +}
[...]
> +static void rzg2l_du_crtc_atomic_flush(struct drm_crtc *crtc,
> +struct drm_atomic_state *state)
> +{
> + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> + struct drm_device *dev = rcrtc->crtc.dev;
> + unsigned long flags;
> +
> + WARN_ON(!crtc->state->enable);
> +
> + /*
> + * If a mode set is in progress we can be called with the CRTC disabled.
> + * We thus need to first get and setup the CRTC in order to configure
> + * planes. We must *not* put the CRTC, as it must be kept awake until
> + * the .atomic_enable() call that will follow. The get operation in
> + * .atomic_enable() will in that case be a no-op, and the CRTC will be
> + * put later in .atomic_disable().
> + */
> + rzg2l_du_crtc_get(rcrtc);
That's a bit suspicious. Have you looked at drm_atomic_helper_commit_tail_rpm()
?
> +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
> +int rzg2l_du_crtc_create(struct rzg2l_du_device *rcdu)
> +{
> + struct rzg2l_du_crtc *rcrtc = &rcdu->crtcs[0];
> + struct drm_crtc *crtc = &rcrtc->crtc;
> + struct drm_plane *primary;
> + int ret;
> +
> + rcrtc->rstc = devm_reset_control_get_shared(rcdu->dev, NULL);
> + if (IS_ERR(rcrtc->rstc)) {
> + dev_err(rcdu->dev, "can't get cpg reset\n");
> + return PTR_ERR(rcrtc->rstc);
> + }
> +
> + rcrtc->rzg2l_clocks.aclk = devm_clk_get(rcdu->dev, "aclk");
> + if (IS_ERR(rcrtc->rzg2l_clocks.aclk)) {
> + dev_err(rcdu->dev, "no axi clock for DU\n");
> + return PTR_ERR(rcrtc->rzg2l_clocks.aclk);
> + }
> +
> + rcrtc->rzg2l_clocks.pclk = devm_clk_get(rcdu->dev, "pclk");
> + if (IS_ERR(rcrtc->rzg2l_clocks.pclk)) {
> + dev_err(rcdu->dev, "no peripheral clock for DU\n");
> + return PTR_ERR(rcrtc->rzg2l_clocks.pclk);
> + }
> +
> + rcrtc->rzg2l_clocks.dclk = devm_clk_get(rcdu->dev, "vclk");
> + if (IS_ERR(rcrtc->rzg2l_clocks.dclk)) {
> + dev_err(rcdu->dev, "no video clock for DU\n");
> + return PTR_ERR(rcrtc->rzg2l_clocks.dclk);
> + }
> +
> + init_waitqueue_head(&rcrtc->flip_wait);
> + rcrtc->dev = rcdu;
> +
> + primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> +
> + ret = drm_crtc_init_with_planes(&rcdu->ddev, crtc, primary, NULL,
> + &crtc_funcs_rz, NULL);
> + if (ret < 0)
> + return ret;
You should use the drmm variant here
> +static void rzg2l_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> +{
> + struct drm_device *dev = old_state->dev;
> +
> + /* Apply the atomic update. */
> + drm_atomic_helper_commit_modeset_disables(dev, old_state);
> + drm_atomic_helper_commit_planes(dev, old_state,
> + DRM_PLANE_COMMIT_ACTIVE_ONLY);
> + drm_atomic_helper_commit_modeset_enables(dev, old_state);
> +
> + drm_atomic_helper_commit_hw_done(old_state);
> + drm_atomic_helper_wait_for_flip_done(dev, old_state);
> +
> + drm_atomic_helper_cleanup_planes(dev,
