Hi Rob,
On Tue, Oct 22, 2019 at 03:02:06PM -0500, Rob Herring wrote:
> On Tue, Oct 22, 2019 at 6:30 AM Laurent Pinchart wrote:
> > On Mon, Oct 21, 2019 at 04:45:48PM -0500, Rob Herring wrote:
> >> Add support in CMA helpers to handle callers specifying
> >> DRM_MODE_DUMB_KERNEL_MAP flag. Existing behavior is maintained with this
> >> change. drm_gem_cma_dumb_create() always creates a kernel mapping as
> >> before. drm_gem_cma_dumb_create_internal() lets the caller set the flags
> >> as desired. Therefore, update all the existing callers of
> >> drm_gem_cma_dumb_create_internal() to also set the
> >> DRM_MODE_DUMB_KERNEL_MAP flag.
> >>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> >> Cc: Sean Paul <sean@xxxxxxxxxx>
> >> Cc: David Airlie <airlied@xxxxxxxx>
> >> Cc: Daniel Vetter <daniel@xxxxxxxx>
> >> Cc: "James (Qian) Wang" <james.qian.wang@xxxxxxx>
> >> Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> >> Cc: Brian Starkey <brian.starkey@xxxxxxx>
> >> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> >> Cc: Kevin Hilman <khilman@xxxxxxxxxxxx>
> >> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >> Cc: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> >> Cc: Sandy Huang <hjc@xxxxxxxxxxxxxx>
> >> Cc: "Heiko Stübner" <heiko@xxxxxxxxx>
> >> Cc: Yannick Fertre <yannick.fertre@xxxxxx>
> >> Cc: Philippe Cornu <philippe.cornu@xxxxxx>
> >> Cc: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> >> Cc: Vincent Abriou <vincent.abriou@xxxxxx>
> >> Cc: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
> >> Cc: Alexandre Torgue <alexandre.torgue@xxxxxx>
> >> Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> >> Cc: linux-amlogic@xxxxxxxxxxxxxxxxxxx
> >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> >> Cc: linux-rockchip@xxxxxxxxxxxxxxxxxxx
> >> Cc: linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> >> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> >> ---
> >> .../gpu/drm/arm/display/komeda/komeda_kms.c | 1 +
> >> drivers/gpu/drm/arm/malidp_drv.c | 1 +
> >> drivers/gpu/drm/drm_gem_cma_helper.c | 48 +++++++++++--------
> >> drivers/gpu/drm/meson/meson_drv.c | 1 +
> >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 1 +
> >> drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 1 +
> >> drivers/gpu/drm/stm/drv.c | 1 +
> >> drivers/gpu/drm/sun4i/sun4i_drv.c | 1 +
> >> 8 files changed, 36 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> >> index d49772de93e0..7cf0dc4cbfc1 100644
> >> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> >> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> >> @@ -31,6 +31,7 @@ static int komeda_gem_cma_dumb_create(struct drm_file *file,
> >> u32 pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> >>
> >> args->pitch = ALIGN(pitch, mdev->chip.bus_width);
> >> + args->flags = DRM_MODE_DUMB_KERNEL_MAP;
> >>
> >> return drm_gem_cma_dumb_create_internal(file, dev, args);
> >> }
> >> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> >> index 8a76315aaa0f..aeb1a779ecc1 100644
> >> --- a/drivers/gpu/drm/arm/malidp_drv.c
> >> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> >> @@ -465,6 +465,7 @@ static int malidp_dumb_create(struct drm_file *file_priv,
> >> u8 alignment = malidp_hw_get_pitch_align(malidp->dev, 1);
> >>
> >> args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp, 8), alignment);
> >> + args->flags = DRM_MODE_DUMB_KERNEL_MAP;
> >>
> >> return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
> >> }
> >> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> >> index 4cebfe01e6ea..f91e9e8adeaf 100644
> >> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> >> @@ -78,21 +78,8 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
> >> return ERR_PTR(ret);
> >> }
> >>
> >> -/**
> >> - * drm_gem_cma_create - allocate an object with the given size
> >> - * @drm: DRM device
> >> - * @size: size of the object to allocate
> >> - *
> >> - * This function creates a CMA GEM object and allocates a contiguous chunk of
> >> - * memory as backing store. The backing memory has the writecombine attribute
> >> - * set.
> >> - *
> >> - * Returns:
> >> - * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> >> - * error code on failure.
> >> - */
> >> -struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> >> - size_t size)
> >> +static struct drm_gem_cma_object *
> >> +drm_gem_cma_create_flags(struct drm_device *drm, size_t size, u32 flags)
> >> {
> >> struct drm_gem_cma_object *cma_obj;
> >> int ret;
> >> @@ -103,6 +90,9 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> >> if (IS_ERR(cma_obj))
> >> return cma_obj;
> >>
> >> + if (!(flags & DRM_MODE_DUMB_KERNEL_MAP))
> >> + cma_obj->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> >> +
> >> cma_obj->vaddr = dma_alloc_attrs(drm->dev, size, &cma_obj->paddr,
> >> GFP_KERNEL | __GFP_NOWARN,
> >> cma_obj->dma_attrs);
> >> @@ -119,6 +109,25 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> >> drm_gem_object_put_unlocked(&cma_obj->base);
> >> return ERR_PTR(ret);
> >> }
> >> +
> >> +/**
> >> + * drm_gem_cma_create - allocate an object with the given size
> >> + * @drm: DRM device
> >> + * @size: size of the object to allocate
> >> + *
> >> + * This function creates a CMA GEM object and allocates a contiguous chunk of
> >> + * memory as backing store. The backing memory has the writecombine attribute
> >> + * set.
> >> + *
> >
> > Shouldn't you mention here that the function always creates a kernel
> > mapping, and that callers that don't need the mapping should use
> > drm_gem_cma_dumb_create_internal() instead ?
>
> Are you confusing drm_gem_cma_create with drm_gem_cma_dumb_create?
> drm_gem_cma_dumb_create() uses defaults and
> drm_gem_cma_dumb_create_internal() allows the caller to tweak
> parameters. Nothing new there other than an additional param to tweak.
>
> > drm_gem_cma_dumb_create_internal() operates at a different level though,
> > and drm_gem_cma_create() is only exported for a single driver. There's
> > no equivalent to drm_gem_cma_create() that can skip the kernel mapping.
>
> Because we don't yet need one. drm_gem_cma_create_flags() can be made
> public when we do. I could do that now I guess and make
> drm_gem_cma_create an inline wrapper.
I don't mind not having drm_gem_cma_create_flags() made public (but you
can do so already if you prefer) if there's no user. My point is that we
now have a mechanism to skip creation of kernel mappings, and that
drm_gem_cma_create() will always result in the creation of a kernel
mapping. I thought it was worth mentioning it, but if you think that's
not needed, feel free to ignore the comment.
--
Regards,
Laurent Pinchart
