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
