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