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...@bp.renesas.com>
I'd still like a review from Geert or Laurent, I don't know the SoC.
> +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.
Maxime
signature.asc
Description: PGP signature
