Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v9 RESEND 4/5] drm: Add RZ/G2L DU Support > > Hi Biju, > > On Thu, Jun 01, 2023 at 12:08:44PM +0000, Biju Das wrote: > > > Subject: Re: [PATCH v9 RESEND 4/5] drm: Add RZ/G2L DU Support > > > > > > Hi Biju, > > > > > > Thank you for the patch. > > > > > > This is a partial review, because the driver is big, and because > > > some changes in v10 will (hopefully) simplify the code and make > > > review easier. > > > > I agree v10 will simplify the code as I have do clean-ups based on > > your review commnet. > > > > > On Tue, May 02, 2023 at 11:09:11AM +0100, 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> > > > > --- > > > > Ref: > > > > > > > > v8->v9: > > > > * Dropped reset_control_assert() from error patch for > rzg2l_du_crtc_get() as > > > > suggested by Philipp Zabel. > > > > v7->v8: > > > > * Dropped RCar du lib and created RZ/G2L DU DRM driver by > creating rz_du folder. > > > > * Updated KConfig and Makefile. > > > > v6->v7: > > > > * Split DU lib and RZ/G2L du driver as separate patch series as > > > > DU support added to more platforms based on RZ/G2L alike SoCs. > > > > * Rebased to latest drm-tip. > > > > * Added patch #2 for binding support for RZ/V2L DU > > > > * Added patch #4 for driver support for RZ/V2L DU > > > > * Added patch #5 for SoC DTSI support for RZ/G2L DU > > > > * Added patch #6 for SoC DTSI support for RZ/V2L DU > > > > * Added patch #7 for Enabling DU on SMARC EVK based on > RZ/{G2L,V2L} SoCs. > > > > * Added patch #8 for Enabling DU on SMARC EVK based on RZ/G2LC > SoC. > > > > --- > > > > drivers/gpu/drm/renesas/Kconfig | 1 + > > > > drivers/gpu/drm/renesas/Makefile | 1 + > > > > drivers/gpu/drm/renesas/rz-du/Kconfig | 20 + > > > > drivers/gpu/drm/renesas/rz-du/Makefile | 8 + > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 714 > > > > ++++++++++++++++ drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h | > > > > 99 +++ drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c | 188 +++++ > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h | 89 ++ > > > > .../gpu/drm/renesas/rz-du/rzg2l_du_encoder.c | 112 +++ > > > > .../gpu/drm/renesas/rz-du/rzg2l_du_encoder.h | 28 + > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c | 770 > > > > ++++++++++++++++++ drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.h > > > > | 43 + drivers/gpu/drm/renesas/rz-du/rzg2l_du_regs.h | 67 ++ > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.c | 430 ++++++++++ > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.h | 94 +++ > > > > 15 files changed, 2664 insertions(+) create mode 100644 > > > > drivers/gpu/drm/renesas/rz-du/Kconfig > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/Makefile > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h > > > > create mode 100644 > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c > > > > create mode 100644 > > > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.h > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.h > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_regs.h > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.c > > > > create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.h > > [snip] > > > > > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c > > > > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c > > > > new file mode 100644 > > > > index 000000000000..d61d433d72e6 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c > > > > @@ -0,0 +1,714 @@ > > [snip] > > > > > +/* > > > > +----------------------------------------------------------------- > > > > +------------ > > > > + * CRTC Functions > > > > + */ > > > > + > > > > +int __rzg2l_du_crtc_plane_atomic_check(struct drm_plane *plane, > > > > + struct drm_plane_state *state, > > > > + const struct rzg2l_du_format_info > **format) > > > > > > This function is only called from rzg2l_du_vsp_plane_atomic_check(), > > > I would inline it there. > > > > Agreed. > > > > > > +{ > > > > + struct drm_device *dev = plane->dev; > > > > + struct drm_crtc_state *crtc_state; > > > > + int ret; > > > > + > > > > + if (!state->crtc) { > > > > + /* > > > > + * The visible field is not reset by the DRM core but > only > > > > + * updated by drm_plane_helper_check_state(), set it > manually. > > > > + */ > > > > + state->visible = false; > > > > + *format = NULL; > > > > + return 0; > > > > + } > > > > + > > > > + crtc_state = drm_atomic_get_crtc_state(state->state, state- > >crtc); > > > > + if (IS_ERR(crtc_state)) > > > > + return PTR_ERR(crtc_state); > > > > + > > > > + ret = drm_atomic_helper_check_plane_state(state, crtc_state, > > > > + DRM_PLANE_NO_SCALING, > > > > + DRM_PLANE_NO_SCALING, > > > > + true, true); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + if (!state->visible) { > > > > + *format = NULL; > > > > + return 0; > > > > + } > > > > + > > > > + *format = rzg2l_du_format_info(state->fb->format->format); > > > > + if (*format == NULL) { > > > > > > Can this happen, or does the DRM core already checks that the > > > framebuffer format is supported by the plane ? > > > > This will make sure the format is as per rzg2l_du_format_info, > > Otherwise print unsupported format. > > I understand that, but it wasn't my question. Does the DRM core check if > the framebuffer pixel format is supported by the plane, using the > plane's supported pixel formats specified by the driver when the plane > was created ? If it does, you could drop this check, as > rzg2l_du_format_info() should support all the formats that the driver > specifies as supported by the plane. > > I believe the DRM core handles this already, given the > drm_plane_check_pixel_format() call in drm_atomic_plane_check(). Yes, it handles already. I will drop the check. As you said it calls drm_atomic_plane_check() which calls drm_plane_check_pixel_format() > > > > > + dev_dbg(dev->dev, "%s: unsupported format %08x\n", > __func__, > > > > + state->fb->format->format); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return 0; > > > > +} > > [snip] > > > > > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > > > > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > > > > new file mode 100644 > > > > index 000000000000..0fea1fea837c > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > > > > @@ -0,0 +1,188 @@ > > [snip] > > > > > +static int rzg2l_du_probe(struct platform_device *pdev) { > > > > + struct rzg2l_du_device *rcdu; > > > > + int ret; > > > > + > > > > + if (drm_firmware_drivers_only()) > > > > + return -ENODEV; > > > > + > > > > + /* Allocate and initialize the RZ/G2L device structure. */ > > > > + rcdu = devm_drm_dev_alloc(&pdev->dev, &rzg2l_du_driver, > > > > + struct rzg2l_du_device, ddev); > > > > + if (IS_ERR(rcdu)) > > > > + return PTR_ERR(rcdu); > > > > + > > > > + rcdu->dev = &pdev->dev; > > > > + rcdu->info = of_device_get_match_data(rcdu->dev); > > > > + > > > > + platform_set_drvdata(pdev, rcdu); > > > > + > > > > + /* I/O resources */ > > > > + rcdu->mmio = devm_platform_ioremap_resource(pdev, 0); > > > > + if (IS_ERR(rcdu->mmio)) > > > > + return PTR_ERR(rcdu->mmio); > > > > + > > > > + /* > > > > + * When sourcing frames from a VSP the DU doesn't perform > any memory > > > > + * access so set the DMA coherent mask to 40 bits to accept > all buffers. > > > > + */ > > > > + ret = dma_coerce_mask_and_coherent(&pdev->dev, > DMA_BIT_MASK(40)); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* DRM/KMS objects */ > > > > + ret = rzg2l_du_modeset_init(rcdu); > > > > + if (ret < 0) { > > > > + if (ret != -EPROBE_DEFER) > > > > + dev_err(&pdev->dev, > > > > + "failed to initialize DRM/KMS (%d)\n", > ret); > > > > > > Use dev_err_probe() > > > > As per your patch [1], I guess it is not required > > > > [1] > > If you add dev_err_probe() calls in rzg2l_du_modeset_init() as > appropriate, like done in [1] :-) OK will do similar change. > > > > > + goto error; > > > > + } > > > > + > > > > + /* > > > > + * Register the DRM device with the core and the connectors > with > > > > + * sysfs. > > > > + */ > > > > + ret = drm_dev_register(&rcdu->ddev, 0); > > > > + if (ret) > > > > + goto error; > > > > + > > > > + DRM_INFO("Device %s probed\n", dev_name(&pdev->dev)); > > > > > > Use drm_info(). > > > > Agreed. > > > > > > + > > > > + drm_fbdev_generic_setup(&rcdu->ddev, 32); > > > > + > > > > + return 0; > > > > + > > > > +error: > > > > + drm_kms_helper_poll_fini(&rcdu->ddev); > > > > + return ret; > > > > +} > > [snip] > > > > > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h > > > > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h > > > > new file mode 100644 > > > > index 000000000000..3b84e91aa64a > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h > > > > @@ -0,0 +1,89 @@ > > [snip] > > > > > +enum rzg2l_du_output { > > > > + RZG2L_DU_OUTPUT_DSI0, > > > > + RZG2L_DU_OUTPUT_DPAD0, > > > > + RZG2L_DU_OUTPUT_MAX, > > > > +}; > > > > + > > > > +/* > > > > + * struct rzg2l_du_output_routing - Output routing specification > > > > + * @possible_crtcs: bitmask of possible CRTCs for the output > > > > + * @port: device tree port number corresponding to this output > > > > +route > > > > + * > > > > + * The DU has 2 possible outputs (DPAD0, DSI0). Output routing > > > > +data > > > > + * specify the valid SoC outputs, which CRTCs can drive the > > > > +output, and the type > > > > + * of in-SoC encoder for the output. > > > > + */ > > > > +struct rzg2l_du_output_routing { > > > > + unsigned int possible_crtcs; > > > > + unsigned int port; > > > > +}; > > > > + > > > > +/* > > > > + * struct rzg2l_du_device_info - DU model-specific information > > > > + * @channels_mask: bit mask of available DU channels > > > > + * @routes: array of CRTC to output routes, indexed by output > > > > +(RZG2L_DU_OUTPUT_*) */ struct rzg2l_du_device_info { > > > > + unsigned int channels_mask; > > > > + struct rzg2l_du_output_routing routes[RZG2L_DU_OUTPUT_MAX]; > }; > > > > > > The driver supports a single SoC, with two outputs, connected to the > > > same DU channel. Do you really need to copy the rzg2l_du_device_info > > > abstraction from the rcar-du driver, or could you simplify the code > ? > > > > After adding basic support, as an optimization will simplify the code > > later. Hope it is ok for you?? > > I'm OK with patches on top. Please don't forget about them :-) Agreed. Cheers, Biju