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.lankho...@linux.intel.com>
> >> Cc: Maxime Ripard <mrip...@kernel.org>
> >> Cc: Sean Paul <s...@poorly.run>
> >> Cc: David Airlie <airl...@linux.ie>
> >> Cc: Daniel Vetter <dan...@ffwll.ch>
> >> Cc: "James (Qian) Wang" <james.qian.w...@arm.com>
> >> Cc: Liviu Dudau <liviu.du...@arm.com>
> >> Cc: Brian Starkey <brian.star...@arm.com>
> >> Cc: Neil Armstrong <narmstr...@baylibre.com>
> >> Cc: Kevin Hilman <khil...@baylibre.com>
> >> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> >> Cc: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> >> Cc: Sandy Huang <h...@rock-chips.com>
> >> Cc: "Heiko Stübner" <he...@sntech.de>
> >> Cc: Yannick Fertre <yannick.fer...@st.com>
> >> Cc: Philippe Cornu <philippe.co...@st.com>
> >> Cc: Benjamin Gaignard <benjamin.gaign...@linaro.org>
> >> Cc: Vincent Abriou <vincent.abr...@st.com>
> >> Cc: Maxime Coquelin <mcoquelin.st...@gmail.com>
> >> Cc: Alexandre Torgue <alexandre.tor...@st.com>
> >> Cc: Chen-Yu Tsai <w...@csie.org>
> >> Cc: linux-amlo...@lists.infradead.org
> >> Cc: linux-arm-ker...@lists.infradead.org
> >> Cc: linux-renesas-...@vger.kernel.org
> >> Cc: linux-rockc...@lists.infradead.org
> >> Cc: linux-st...@st-md-mailman.stormreply.com
> >> Signed-off-by: Rob Herring <r...@kernel.org>
> >> ---
> >> .../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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
