On Wed, Dec 13, 2023 at 04:47:09PM +0100, Maxime Ripard wrote:
> 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.
I won't have time soon, and this driver has been blocked for way too
long due to that :-S
> > +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() ?
>
> > +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
>
> > +int rzg2l_du_crtc_create(struct rzg2l_du_device *rcdu)
> > +{
> > + struct rzg2l_du_crtc *rcrtc = &rcdu->crtcs[0];
> > + struct drm_crtc *crtc = &rcrtc->crtc;
> > + struct drm_plane *primary;
> > + int ret;
> > +
> > + rcrtc->rstc = devm_reset_control_get_shared(rcdu->dev, NULL);
> > + if (IS_ERR(rcrtc->rstc)) {
> > + dev_err(rcdu->dev, "can't get cpg reset\n");
> > + return PTR_ERR(rcrtc->rstc);
> > + }
> > +
> > + rcrtc->rzg2l_clocks.aclk = devm_clk_get(rcdu->dev, "aclk");
> > + if (IS_ERR(rcrtc->rzg2l_clocks.aclk)) {
> > + dev_err(rcdu->dev, "no axi clock for DU\n");
> > + return PTR_ERR(rcrtc->rzg2l_clocks.aclk);
> > + }
> > +
> > + rcrtc->rzg2l_clocks.pclk = devm_clk_get(rcdu->dev, "pclk");
> > + if (IS_ERR(rcrtc->rzg2l_clocks.pclk)) {
> > + dev_err(rcdu->dev, "no peripheral clock for DU\n");
> > + return PTR_ERR(rcrtc->rzg2l_clocks.pclk);
> > + }
> > +
> > + rcrtc->rzg2l_clocks.dclk = devm_clk_get(rcdu->dev, "vclk");
> > + if (IS_ERR(rcrtc->rzg2l_clocks.dclk)) {
> > + dev_err(rcdu->dev, "no video clock for DU\n");
> > + return PTR_ERR(rcrtc->rzg2l_clocks.dclk);
> > + }
> > +
> > + init_waitqueue_head(&rcrtc->flip_wait);
> > + rcrtc->dev = rcdu;
> > +
> > + primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> > +
> > + ret = drm_crtc_init_with_planes(&rcdu->ddev, crtc, primary, NULL,
> > + &crtc_funcs_rz, NULL);
> > + if (ret < 0)
> > + return ret;
>
> You should use the drmm variant here
>
> > +static void rzg2l_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > +{
> > + struct drm_device *dev = old_state->dev;
> > +
> > + /* Apply the atomic update. */
> > + drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > + drm_atomic_helper_commit_planes(dev, old_state,
> > + DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > + drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > +
> > + drm_atomic_helper_commit_hw_done(old_state);
> > + drm_atomic_helper_wait_for_flip_done(dev, old_state);
> > +
> > + drm_atomic_helper_cleanup_planes(dev, old_state);
> > +}
>
> AFAIK, there's no reason to roll your commit_tail implementation
> yourself here. You should at the very least document why you needed to
> do so.
>
> > +int rzg2l_du_vsp_map_fb(struct rzg2l_du_vsp *vsp, struct drm_framebuffer *fb,
> > + struct sg_table sg_tables[3])
> > +{
> > + struct rzg2l_du_device *rcdu = vsp->dev;
> > + unsigned int i, j;
> > + int ret;
> > +
> > + for (i = 0; i < fb->format->num_planes; ++i) {
> > + struct drm_gem_dma_object *gem = drm_fb_dma_get_gem_obj(fb, i);
> > + struct sg_table *sgt = &sg_tables[i];
> > +
> > + if (gem->sgt) {
> > + struct scatterlist *src;
> > + struct scatterlist *dst;
> > +
> > + /*
> > + * If the GEM buffer has a scatter gather table, it has
> > + * been imported from a dma-buf and has no physical
> > + * address as it might not be physically contiguous.
> > + * Copy the original scatter gather table to map it to
> > + * the VSP.
> > + */
> > + ret = sg_alloc_table(sgt, gem->sgt->orig_nents,
> > + GFP_KERNEL);
> > + if (ret)
> > + goto fail;
> > +
> > + src = gem->sgt->sgl;
> > + dst = sgt->sgl;
> > + for (j = 0; j < gem->sgt->orig_nents; ++j) {
> > + sg_set_page(dst, sg_page(src), src->length,
> > + src->offset);
> > + src = sg_next(src);
> > + dst = sg_next(dst);
> > + }
> > + } else {
> > + ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr,
> > + gem->dma_addr, gem->base.size);
> > + if (ret)
> > + goto fail;
> > + }
> > +
> > + ret = vsp1_du_map_sg(vsp->vsp, sgt);
> > + if (ret) {
> > + sg_free_table(sgt);
> > + goto fail;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +fail:
> > + while (i--) {
> > + struct sg_table *sgt = &sg_tables[i];
> > +
> > + vsp1_du_unmap_sg(vsp->vsp, sgt);
> > + sg_free_table(sgt);
> > + }
> > +
> > + return ret;
> > +}
>
> Ditto, you should at least document why you're not using the helpers.
>
> > +static void rzg2l_du_vsp_cleanup(struct drm_device *dev, void *res)
> > +{
> > + struct rzg2l_du_vsp *vsp = res;
> > + unsigned int i;
> > +
> > + for (i = 0; i < vsp->num_planes; ++i) {
> > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > +
> > + drm_plane_cleanup(&plane->plane);
> > + }
> > +
> > + kfree(vsp->planes);
> > +
> > + put_device(vsp->vsp);
> > +}
> > +
> > +int rzg2l_du_vsp_init(struct rzg2l_du_vsp *vsp, struct device_node *np,
> > + unsigned int crtcs)
> > +{
> > + struct rzg2l_du_device *rcdu = vsp->dev;
> > + struct platform_device *pdev;
> > + unsigned int num_crtcs = hweight32(crtcs);
> > + unsigned int num_planes = 2;
> > + unsigned int i;
> > + int ret;
> > +
> > + /* Find the VSP device and initialize it. */
> > + pdev = of_find_device_by_node(np);
> > + if (!pdev)
> > + return -ENXIO;
> > +
> > + vsp->vsp = &pdev->dev;
> > +
> > + ret = drmm_add_action_or_reset(&rcdu->ddev, rzg2l_du_vsp_cleanup, vsp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = vsp1_du_init(vsp->vsp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
> > + if (!vsp->planes)
> > + return -ENOMEM;
>
> drmm_kcalloc or drmm_kmalloc_array
>
> > +
> > + for (i = 0; i < num_planes; ++i) {
> > + enum drm_plane_type type = i < num_crtcs
> > + ? DRM_PLANE_TYPE_PRIMARY
> > + : DRM_PLANE_TYPE_OVERLAY;
> > + struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > +
> > + plane->vsp = vsp;
> > + plane->index = i;
> > + ret = drm_universal_plane_init(&rcdu->ddev, &plane->plane,
> > + crtcs, &rzg2l_du_vsp_plane_funcs,
> > + rzg2l_du_vsp_formats,
> > + ARRAY_SIZE(rzg2l_du_vsp_formats),
> > + NULL, type, NULL);
> > + if (ret < 0)
> > + return ret;
>
> you need to use drmm variant here too.
--
Regards,
Laurent Pinchart
