Hi Laurent,
On 04/12/2020 22:01, Laurent Pinchart wrote:
> devm_kcalloc() is the wrong API to allocate planes, as the lifetime of
> the planes is tied to the DRM device, not the device to driver
> binding. drmm_kcalloc() isn't a good option either, as it would result
> in the planes being freed before being unregistered during the managed
> cleanup of the DRM objects. Use a plain kcalloc(), and cleanup the
> planes and free the memory in the existing rcar_du_vsp_cleanup()
> handler.
Managed memory always seems to hurt - which is a shame, because it
should be better throughout.
It's like we need a way to arbitrarily specify the lifetimes of objects
correctly against another object... without being tied to a dev ...
Anyway,
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 4dcb1bfbe201..78a886651d9f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -21,6 +21,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/of_platform.h>
> #include <linux/scatterlist.h>
> +#include <linux/slab.h>
> #include <linux/videodev2.h>
>
> #include <media/vsp1.h>
> @@ -344,6 +345,15 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
> static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res)
> {
> struct rcar_du_vsp *vsp = res;
> + unsigned int i;
> +
> + for (i = 0; i < vsp->num_planes; ++i) {
> + struct rcar_du_vsp_plane *plane = &vsp->planes[i];
> +
> + drm_plane_cleanup(&plane->plane);
> + }
> +
> + kfree(vsp->planes);
>
> put_device(vsp->vsp);
> }
> @@ -354,6 +364,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
> struct rcar_du_device *rcdu = vsp->dev;
> struct platform_device *pdev;
> unsigned int num_crtcs = hweight32(crtcs);
> + unsigned int num_planes;
> unsigned int i;
> int ret;
>
> @@ -376,14 +387,13 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
> * The VSP2D (Gen3) has 5 RPFs, but the VSP1D (Gen2) is limited to
> * 4 RPFs.
> */
> - vsp->num_planes = rcdu->info->gen >= 3 ? 5 : 4;
> + num_planes = rcdu->info->gen >= 3 ? 5 : 4;
>
> - vsp->planes = devm_kcalloc(rcdu->dev, vsp->num_planes,
> - sizeof(*vsp->planes), GFP_KERNEL);
> + vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
> if (!vsp->planes)
> return -ENOMEM;
>
> - for (i = 0; i < vsp->num_planes; ++i) {
> + for (i = 0; i < num_planes; ++i) {
> enum drm_plane_type type = i < num_crtcs
> ? DRM_PLANE_TYPE_PRIMARY
> : DRM_PLANE_TYPE_OVERLAY;
> @@ -409,8 +419,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
> } else {
> drm_plane_create_alpha_property(&plane->plane);
> drm_plane_create_zpos_property(&plane->plane, 1, 1,
> - vsp->num_planes - 1);
> + num_planes - 1);
> }
> +
> + vsp->num_planes++;> }
>
> return 0;
>
--
Regards
--
Kieran
