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@xxxxxxxxxx> > > Sent: Wednesday, December 13, 2023 3:47 PM > > To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > > > > 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
Attachment:
signature.asc
Description: PGP signature
