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

[Maxime Ripard]

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 <mripard at kernel.org>
> > Sent: Wednesday, December 13, 2023 3:47 PM
> > To: Biju Das <biju.das.jz at bp.renesas.com>
> > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > 
> > On Tue, Nov 28, 2023 at 10:51:27AM +0000, 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 <biju.das.jz at bp.renesas.com>
> > 
> > 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;
> > > +
> > > +	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() ?
> 
> 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.

> > 
> > > +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?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231215/e280dbaf/attachment.sig>