RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support

2024-01-11 Thread Biju Das
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

2024-01-10 Thread Laurent Pinchart
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

2024-01-10 Thread Laurent Pinchart
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

2024-01-10 Thread Maxime Ripard
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

2024-01-04 Thread Biju Das
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

2024-01-04 Thread Biju Das
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

2023-12-15 Thread Biju Das
> 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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Maxime Ripard
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

2023-12-15 Thread Maxime Ripard
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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Maxime Ripard
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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Maxime Ripard
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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Biju Das
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

2023-12-15 Thread Maxime Ripard
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

2023-12-15 Thread Maxime Ripard
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

2023-12-15 Thread Maxime Ripard
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

2023-12-14 Thread Biju Das
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

2023-12-14 Thread Biju Das
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

2023-12-14 Thread Biju Das
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

2023-12-14 Thread Biju Das
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

2023-12-13 Thread Biju Das
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

2023-12-13 Thread Geert Uytterhoeven
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

2023-12-13 Thread Laurent Pinchart
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

2023-12-13 Thread Maxime Ripard
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

2023-12-13 Thread Maxime Ripard
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,