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 <[email protected]> > > Sent: Wednesday, December 13, 2023 3:47 PM > > To: Biju Das <[email protected]> > > 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 <[email protected]> > > > > 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
signature.asc
Description: PGP signature
