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...@bp.renesas.com>
> 
> 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

Reply via email to