[PATCH v5 19/21] drm/tegra: Implement new UAPI
Mikko Perttunen
cyndis at kapsi.fi
Tue Mar 23 14:43:03 UTC 2021
On 3/23/21 3:25 PM, Thierry Reding wrote:
> On Mon, Jan 11, 2021 at 03:00:17PM +0200, Mikko Perttunen wrote:
>> Implement the non-submission parts of the new UAPI, including
>> channel management and memory mapping. The UAPI is under the
>> CONFIG_DRM_TEGRA_STAGING config flag for now.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen at nvidia.com>
>> ---
>> v5:
>> * Set iova_end in both mapping paths
>> v4:
>> * New patch, split out from combined UAPI + submit patch.
>> ---
>> drivers/gpu/drm/tegra/Makefile | 1 +
>> drivers/gpu/drm/tegra/drm.c | 41 ++--
>> drivers/gpu/drm/tegra/drm.h | 5 +
>> drivers/gpu/drm/tegra/uapi.h | 63 ++++++
>> drivers/gpu/drm/tegra/uapi/uapi.c | 307 ++++++++++++++++++++++++++++++
>
> I'd prefer if we kept the directory structure flat. There's something
> like 19 pairs of files in the top-level directory, which is reasonably
> manageable. Also, it looks like there's going to be a couple more files
> in this new subdirectory. I'd prefer if that was all merged into the
> single uapi.c source file to keep things simpler. These are all really
> small files, so there's no need to aggressively split things up. Helps
> with compilation time, too.
Will do, although I think having plenty of subdirectories makes things
more organized :)
>
> FWIW, I would've been fine with stashing all of this into drm.c as well
> since the rest of the UAPI is in that already. The churn in this patch
> is reasonably small, but it would've been even less if this was just all
> in drm.c.
I think we shouldn't have the uapi in drm.c -- it just makes the file a
bit of a dumping ground. I think drm.c should have the code that relates
to initialization and initial registration with DRM.
>
>> 5 files changed, 401 insertions(+), 16 deletions(-)
>> create mode 100644 drivers/gpu/drm/tegra/uapi.h
>> create mode 100644 drivers/gpu/drm/tegra/uapi/uapi.c
>>
>> diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
>> index d6cf202414f0..0abdb21b38b9 100644
>> --- a/drivers/gpu/drm/tegra/Makefile
>> +++ b/drivers/gpu/drm/tegra/Makefile
>> @@ -3,6 +3,7 @@ ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG
>>
>> tegra-drm-y := \
>> drm.o \
>> + uapi/uapi.o \
>> gem.o \
>> fb.o \
>> dp.o \
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index afd3f143c5e0..6a51035ce33f 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -20,6 +20,7 @@
>> #include <drm/drm_prime.h>
>> #include <drm/drm_vblank.h>
>>
>> +#include "uapi.h"
>> #include "drm.h"
>> #include "gem.h"
>>
>> @@ -33,11 +34,6 @@
>> #define CARVEOUT_SZ SZ_64M
>> #define CDMA_GATHER_FETCHES_MAX_NB 16383
>>
>> -struct tegra_drm_file {
>> - struct idr contexts;
>> - struct mutex lock;
>> -};
>> -
>> static int tegra_atomic_check(struct drm_device *drm,
>> struct drm_atomic_state *state)
>> {
>> @@ -90,7 +86,8 @@ static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp)
>> if (!fpriv)
>> return -ENOMEM;
>>
>> - idr_init_base(&fpriv->contexts, 1);
>> + idr_init_base(&fpriv->legacy_contexts, 1);
>> + xa_init_flags(&fpriv->contexts, XA_FLAGS_ALLOC1);
>> mutex_init(&fpriv->lock);
>> filp->driver_priv = fpriv;
>>
>> @@ -429,7 +426,7 @@ static int tegra_client_open(struct tegra_drm_file *fpriv,
>> if (err < 0)
>> return err;
>>
>> - err = idr_alloc(&fpriv->contexts, context, 1, 0, GFP_KERNEL);
>> + err = idr_alloc(&fpriv->legacy_contexts, context, 1, 0, GFP_KERNEL);
>> if (err < 0) {
>> client->ops->close_channel(context);
>> return err;
>> @@ -484,13 +481,13 @@ static int tegra_close_channel(struct drm_device *drm, void *data,
>>
>> mutex_lock(&fpriv->lock);
>>
>> - context = idr_find(&fpriv->contexts, args->context);
>> + context = idr_find(&fpriv->legacy_contexts, args->context);
>> if (!context) {
>> err = -EINVAL;
>> goto unlock;
>> }
>>
>> - idr_remove(&fpriv->contexts, context->id);
>> + idr_remove(&fpriv->legacy_contexts, context->id);
>> tegra_drm_context_free(context);
>>
>> unlock:
>> @@ -509,7 +506,7 @@ static int tegra_get_syncpt(struct drm_device *drm, void *data,
>>
>> mutex_lock(&fpriv->lock);
>>
>> - context = idr_find(&fpriv->contexts, args->context);
>> + context = idr_find(&fpriv->legacy_contexts, args->context);
>> if (!context) {
>> err = -ENODEV;
>> goto unlock;
>> @@ -538,7 +535,7 @@ static int tegra_submit(struct drm_device *drm, void *data,
>>
>> mutex_lock(&fpriv->lock);
>>
>> - context = idr_find(&fpriv->contexts, args->context);
>> + context = idr_find(&fpriv->legacy_contexts, args->context);
>> if (!context) {
>> err = -ENODEV;
>> goto unlock;
>> @@ -563,7 +560,7 @@ static int tegra_get_syncpt_base(struct drm_device *drm, void *data,
>>
>> mutex_lock(&fpriv->lock);
>>
>> - context = idr_find(&fpriv->contexts, args->context);
>> + context = idr_find(&fpriv->legacy_contexts, args->context);
>> if (!context) {
>> err = -ENODEV;
>> goto unlock;
>> @@ -732,10 +729,21 @@ static int tegra_gem_get_flags(struct drm_device *drm, void *data,
>>
>> static const struct drm_ioctl_desc tegra_drm_ioctls[] = {
>> #ifdef CONFIG_DRM_TEGRA_STAGING
>> - DRM_IOCTL_DEF_DRV(TEGRA_GEM_CREATE, tegra_gem_create,
>> + DRM_IOCTL_DEF_DRV(TEGRA_CHANNEL_OPEN, tegra_drm_ioctl_channel_open,
>> + DRM_RENDER_ALLOW),
>> + DRM_IOCTL_DEF_DRV(TEGRA_CHANNEL_CLOSE, tegra_drm_ioctl_channel_close,
>> + DRM_RENDER_ALLOW),
>
> I'd prefer to keep call these TEGRA_OPEN_CHANNEL and TEGRA_CLOSE_CHANNEL
> because I find that easier to think of. My reasoning goes: the TEGRA_
> prefix means we're operating at a global context and then we perform the
> OPEN_CHANNEL and CLOSE_CHANNEL operations. Whereas by the same reasoning
> TEGRA_CHANNEL_OPEN and TEGRA_CHANNEL_CLOSE suggest we're operating at
> the channel context and perform OPEN and CLOSE operations. For close you
> could make the argument that it makes sense, but you can't open a
> channel that you don't have yet.
I go by the same argument but consider TEGRA_CHANNEL_OPEN a bit of a
"static method" of channels, and as such acceptable :p But I do see your
point -- I can change it.
>
> And if that doesn't convince you, I think appending _LEGACY here like we
> do for CREATE and MMAP would be more consistent. Who's going to remember
> which one is new: TEGRA_CHANNEL_OPEN or TEGRA_OPEN_CHANNEL?
>
>> + DRM_IOCTL_DEF_DRV(TEGRA_CHANNEL_MAP, tegra_drm_ioctl_channel_map,
>> DRM_RENDER_ALLOW),
>> - DRM_IOCTL_DEF_DRV(TEGRA_GEM_MMAP, tegra_gem_mmap,
>> + DRM_IOCTL_DEF_DRV(TEGRA_CHANNEL_UNMAP, tegra_drm_ioctl_channel_unmap,
>> DRM_RENDER_ALLOW),
>> + DRM_IOCTL_DEF_DRV(TEGRA_GEM_CREATE, tegra_drm_ioctl_gem_create,
>> + DRM_RENDER_ALLOW),
>> + DRM_IOCTL_DEF_DRV(TEGRA_GEM_MMAP, tegra_drm_ioctl_gem_mmap,
>> + DRM_RENDER_ALLOW),
>> +
>> + DRM_IOCTL_DEF_DRV(TEGRA_GEM_CREATE_LEGACY, tegra_gem_create, DRM_RENDER_ALLOW),
>> + DRM_IOCTL_DEF_DRV(TEGRA_GEM_MMAP_LEGACY, tegra_gem_mmap, DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF_DRV(TEGRA_SYNCPT_READ, tegra_syncpt_read,
>> DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF_DRV(TEGRA_SYNCPT_INCR, tegra_syncpt_incr,
>> @@ -789,10 +797,11 @@ static void tegra_drm_postclose(struct drm_device *drm, struct drm_file *file)
>> struct tegra_drm_file *fpriv = file->driver_priv;
>>
>> mutex_lock(&fpriv->lock);
>> - idr_for_each(&fpriv->contexts, tegra_drm_context_cleanup, NULL);
>> + idr_for_each(&fpriv->legacy_contexts, tegra_drm_context_cleanup, NULL);
>> + tegra_drm_uapi_close_file(fpriv);
>> mutex_unlock(&fpriv->lock);
>>
>> - idr_destroy(&fpriv->contexts);
>> + idr_destroy(&fpriv->legacy_contexts);
>> mutex_destroy(&fpriv->lock);
>> kfree(fpriv);
>> }
>> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
>> index 0f38f159aa8e..1af57c2016eb 100644
>> --- a/drivers/gpu/drm/tegra/drm.h
>> +++ b/drivers/gpu/drm/tegra/drm.h
>> @@ -59,6 +59,11 @@ struct tegra_drm {
>> struct tegra_display_hub *hub;
>> };
>>
>> +static inline struct host1x *tegra_drm_to_host1x(struct tegra_drm *tegra)
>> +{
>> + return dev_get_drvdata(tegra->drm->dev->parent);
>> +}
>> +
>> struct tegra_drm_client;
>>
>> struct tegra_drm_context {
>> diff --git a/drivers/gpu/drm/tegra/uapi.h b/drivers/gpu/drm/tegra/uapi.h
>> new file mode 100644
>> index 000000000000..5c422607e8fa
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tegra/uapi.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright (c) 2020 NVIDIA Corporation */
>> +
>> +#ifndef _TEGRA_DRM_UAPI_H
>> +#define _TEGRA_DRM_UAPI_H
>> +
>> +#include <linux/dma-mapping.h>
>> +#include <linux/idr.h>
>> +#include <linux/kref.h>
>> +#include <linux/xarray.h>
>> +
>> +#include <drm/drm.h>
>> +
>> +struct drm_file;
>> +struct drm_device;
>> +
>> +struct tegra_drm_file {
>> + /* Legacy UAPI state */
>> + struct idr legacy_contexts;
>> + struct mutex lock;
>> +
>> + /* New UAPI state */
>> + struct xarray contexts;
>> +};
>> +
>> +struct tegra_drm_channel_ctx {
>> + struct tegra_drm_client *client;
>> + struct host1x_channel *channel;
>> + struct xarray mappings;
>> +};
>
> This is mostly the same as tegra_drm_context, so can't we just merge the
> two? There's going to be slight overlap, but overall things are going to
> be less confusing to follow.
>
> Even more so because I think we should consider phasing out the old UAPI
> eventually and then we can just remove the unneeded fields from this.
Okay.
>
>> +
>> +struct tegra_drm_mapping {
>> + struct kref ref;
>> +
>> + struct device *dev;
>> + struct host1x_bo *bo;
>> + struct sg_table *sgt;
>> + enum dma_data_direction direction;
>> + dma_addr_t iova;
>> + dma_addr_t iova_end;
>
> iova_end seems to never be used. Do we need it?
It is used in the firewall.
>
>> +};
>> +
>> +int tegra_drm_ioctl_channel_open(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +int tegra_drm_ioctl_channel_close(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +int tegra_drm_ioctl_channel_map(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +int tegra_drm_ioctl_channel_unmap(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +int tegra_drm_ioctl_gem_create(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +int tegra_drm_ioctl_gem_mmap(struct drm_device *drm, void *data,
>> + struct drm_file *file);
>> +
>> +void tegra_drm_uapi_close_file(struct tegra_drm_file *file);
>> +void tegra_drm_mapping_put(struct tegra_drm_mapping *mapping);
>> +struct tegra_drm_channel_ctx *
>> +tegra_drm_channel_ctx_lock(struct tegra_drm_file *file, u32 id);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/tegra/uapi/uapi.c b/drivers/gpu/drm/tegra/uapi/uapi.c
>> new file mode 100644
>> index 000000000000..d503b5e817c4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tegra/uapi/uapi.c
>> @@ -0,0 +1,307 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (c) 2020 NVIDIA Corporation */
>> +
>> +#include <linux/host1x.h>
>> +#include <linux/iommu.h>
>> +#include <linux/list.h>
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_file.h>
>> +
>> +#include "../uapi.h"
>> +#include "../drm.h"
>> +
>> +struct tegra_drm_channel_ctx *
>> +tegra_drm_channel_ctx_lock(struct tegra_drm_file *file, u32 id)
>> +{
>> + struct tegra_drm_channel_ctx *ctx;
>> +
>> + mutex_lock(&file->lock);
>> + ctx = xa_load(&file->contexts, id);
>> + if (!ctx)
>> + mutex_unlock(&file->lock);
>> +
>> + return ctx;
>> +}
>
> This interface seems slightly odd. Looking at how this is used I see how
> doing it this way saves a couple of lines. However, it also make this
> difficult to understand, so I wonder if it wouldn't be better to just
> open-code this in the three callsites to make the code flow a bit more
> idiomatic.
Ok, will do. (Another option may be to add a
tegra_drm_channel_ctx_unlock that just unlocks file->lock -- that'd
abstract it out even better, which I quite like -- but I'll go with your
preference)
>
>> +
>> +static void tegra_drm_mapping_release(struct kref *ref)
>> +{
>> + struct tegra_drm_mapping *mapping =
>> + container_of(ref, struct tegra_drm_mapping, ref);
>> +
>> + if (mapping->sgt)
>> + dma_unmap_sgtable(mapping->dev, mapping->sgt,
>> + mapping->direction, DMA_ATTR_SKIP_CPU_SYNC);
>> +
>> + host1x_bo_unpin(mapping->dev, mapping->bo, mapping->sgt);
>> + host1x_bo_put(mapping->bo);
>> +
>> + kfree(mapping);
>> +}
>> +
>> +void tegra_drm_mapping_put(struct tegra_drm_mapping *mapping)
>> +{
>> + kref_put(&mapping->ref, tegra_drm_mapping_release);
>> +}
>> +
>> +static void tegra_drm_channel_ctx_close(struct tegra_drm_channel_ctx *ctx)
>
> Yeah, the more often I read it, the more I'm in favour of just
> collapsing tegra_drm_channel_ctx into tegra_drm_channel if for nothing
> else but to get rid of that annoying _ctx suffix that's there for no
> other reason than to differentiate it from "legacy" contexts.
>
>> +{
>> + unsigned long mapping_id;
>
> It's clear from the context that this is a mapping ID, so I think you
> can just leave out the "mapping_" prefix to save a bit on screen space.
Sure.
>
>> + struct tegra_drm_mapping *mapping;
>> +
>> + xa_for_each(&ctx->mappings, mapping_id, mapping)
>> + tegra_drm_mapping_put(mapping);
>> +
>> + xa_destroy(&ctx->mappings);
>> +
>> + host1x_channel_put(ctx->channel);
>> +
>> + kfree(ctx);
>> +}
>> +
>> +int close_channel_ctx(int id, void *p, void *data)
>> +{
>> + struct tegra_drm_channel_ctx *ctx = p;
>> +
>> + tegra_drm_channel_ctx_close(ctx);
>> +
>> + return 0;
>> +}
>
> The signature looked strange, so I went looking for where this is called
> from and turns out I can't find any place where this is used. Do we need
> it?
Ah, maybe I have left it from some previous version. Will fix.
>
>> +
>> +void tegra_drm_uapi_close_file(struct tegra_drm_file *file)
>> +{
>> + unsigned long ctx_id;
>
> Just like for mappings above, I think it's fine to leave out the ctx_
> prefix here.
Yep.
>
>> + struct tegra_drm_channel_ctx *ctx;
>> +
>> + xa_for_each(&file->contexts, ctx_id, ctx)
>> + tegra_drm_channel_ctx_close(ctx);
>> +
>> + xa_destroy(&file->contexts);
>> +}
>> +
>> +int tegra_drm_ioctl_channel_open(struct drm_device *drm, void *data,
>> + struct drm_file *file)
>> +{
>> + struct tegra_drm_file *fpriv = file->driver_priv;
>> + struct tegra_drm *tegra = drm->dev_private;
>> + struct drm_tegra_channel_open *args = data;
>> + struct tegra_drm_client *client = NULL;
>> + struct tegra_drm_channel_ctx *ctx;
>> + int err;
>> +
>> + if (args->flags)
>> + return -EINVAL;
>> +
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return -ENOMEM;
>> +
>> + err = -ENODEV;
>> + list_for_each_entry(client, &tegra->clients, list) {
>> + if (client->base.class == args->host1x_class) {
>> + err = 0;
>> + break;
>> + }
>> + }
>> + if (err)
>> + goto free_ctx;
>
> This type of construct looks weird. I found that a good way around this
> is to split this off into a separate function that does the lookup and
> just returns NULL when it doesn't find one, which is very elegant:
>
> struct tegra_drm_client *tegra_drm_find_client(struct tegra_drm *tegra, u32 class)
> {
> struct tegra_drm_client *client;
>
> list_for_each_entry(client, &tegra->clients, list)
> if (client->base.class == class)
> return client;
>
> return NULL;
> }
>
> and then all of a sudden, the very cumbersome construct above becomes
> this pretty piece of code:
>
> client = tegra_drm_find_client(tegra, args->host1x_class);
> if (!client) {
> err = -ENODEV;
> goto free_ctx;
> }
>
> No need for initializing client to NULL or preventatively setting err =
> -ENODEV or anything.
Yep.
>
>> +
>> + if (client->shared_channel) {
>> + ctx->channel = host1x_channel_get(client->shared_channel);
>> + } else {
>> + ctx->channel = host1x_channel_request(&client->base);
>> + if (!ctx->channel) {
>> + err = -EBUSY;
>
> I -EBUSY really appropriate here? Can host1x_channel_request() fail for
> other reasons?
It could also fail due to being out of memory (failing to allocate space
for CDMA) - I guess we should plumb the error code here. But -EBUSY is
really the most likely thing to happen anyway. Perhaps that can be done
separately.
>
>> + goto free_ctx;
>> + }
>> + }
>> +
>> + err = xa_alloc(&fpriv->contexts, &args->channel_ctx, ctx,
>> + XA_LIMIT(1, U32_MAX), GFP_KERNEL);
>> + if (err < 0)
>> + goto put_channel;
>> +
>> + ctx->client = client;
>> + xa_init_flags(&ctx->mappings, XA_FLAGS_ALLOC1);
>> +
>> + args->hardware_version = client->version;
>> +
>> + return 0;
>> +
>> +put_channel:
>> + host1x_channel_put(ctx->channel);
>> +free_ctx:
>> + kfree(ctx);
>> +
>> + return err;
>> +}
>> +
>> +int tegra_drm_ioctl_channel_close(struct drm_device *drm, void *data,
>> + struct drm_file *file)
>> +{
>> + struct tegra_drm_file *fpriv = file->driver_priv;
>> + struct drm_tegra_channel_close *args = data;
>> + struct tegra_drm_channel_ctx *ctx;
>> +
>> + ctx = tegra_drm_channel_ctx_lock(fpriv, args->channel_ctx);
>> + if (!ctx)
>> + return -EINVAL;
>> +
>> + xa_erase(&fpriv->contexts, args->channel_ctx);
>> +
>> + mutex_unlock(&fpriv->lock);
>> +
>> + tegra_drm_channel_ctx_close(ctx);
>> +
>> + return 0;
>> +}
>> +
>> +int tegra_drm_ioctl_channel_map(struct drm_device *drm, void *data,
>> + struct drm_file *file)
>> +{
>> + struct tegra_drm_file *fpriv = file->driver_priv;
>> + struct drm_tegra_channel_map *args = data;
>> + struct tegra_drm_channel_ctx *ctx;
>> + struct tegra_drm_mapping *mapping;
>> + struct drm_gem_object *gem;
>> + u32 mapping_id;
>> + int err = 0;
>> +
>> + if (args->flags & ~DRM_TEGRA_CHANNEL_MAP_READWRITE)
>> + return -EINVAL;
>> +
>> + ctx = tegra_drm_channel_ctx_lock(fpriv, args->channel_ctx);
>> + if (!ctx)
>> + return -EINVAL;
>> +
>> + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>> + if (!mapping) {
>> + err = -ENOMEM;
>> + goto unlock;
>> + }
>> +
>> + kref_init(&mapping->ref);
>> +
>> + gem = drm_gem_object_lookup(file, args->handle);
>> + if (!gem) {
>> + err = -EINVAL;
>> + goto unlock;
>> + }
>> +
>> + mapping->dev = ctx->client->base.dev;
>> + mapping->bo = &container_of(gem, struct tegra_bo, gem)->base;
>
> We already have host1x_bo_lookup() in drm.c that you can use to avoid
> this strange cast.
Okay, will fix.
>
>> +
>> + if (!iommu_get_domain_for_dev(mapping->dev) ||
>> + ctx->client->base.group) {
>
> This expression is now used in at least two places, so I wonder if we
> should have a helper for it along with some documentation about why this
> is the right thing to do. I have a local patch that adds a comment to
> the other instance of this because I had forgotten why this was correct,
> so I can pick that up and refactor later on.
Actually, just last week I found out that the condition here is wrong
(at least for this particular instance) -- with the current condition,
if IOMMU is disabled we end up in the first branch, but if the BO in
question was imported through DMA-BUF the iova will be NULL -
host1x_bo_pin returns an SGT instead, so we need to go to the else path,
which works fine. (If ctx->client->base.group is set, this is not a
problem since import will IOMMU map the BO and set iova). I have a local
fix for this which I'll add to v6.
>
>> + host1x_bo_pin(mapping->dev, mapping->bo,
>> + &mapping->iova);
>> + } else {
>> + mapping->direction = DMA_TO_DEVICE;
>> + if (args->flags & DRM_TEGRA_CHANNEL_MAP_READWRITE)
>> + mapping->direction = DMA_BIDIRECTIONAL;
>> +
>> + mapping->sgt =
>> + host1x_bo_pin(mapping->dev, mapping->bo, NULL);
>> + if (IS_ERR(mapping->sgt)) {
>> + err = PTR_ERR(mapping->sgt);
>> + goto put_gem;
>> + }
>> +
>> + err = dma_map_sgtable(mapping->dev, mapping->sgt,
>> + mapping->direction,
>> + DMA_ATTR_SKIP_CPU_SYNC);
>> + if (err)
>> + goto unpin;
>> +
>> + /* TODO only map the requested part */
>> + mapping->iova = sg_dma_address(mapping->sgt->sgl);
>
> That comment seems misplaced here since the mapping already happens
> above. Also, wouldn't the same TODO apply to the host1x_bo_pin() path in
> the if block? Maybe the TODO should be at the top of the function?
>
> Alternatively, if this isn't implemented in this patch anyway, maybe
> just drop the comment altogether. In order to implement this, wouldn't
> the UAPI have to change as well? In that case it might be better to add
> the TODO somewhere in the UAPI header, or in a separate TODO file in the
> driver's directory.
Yeah, I'll drop the comment. The UAPI originally had support for this
but I dropped it upon Dmitry's objection.
>
>> + }
>> +
>> + mapping->iova_end = mapping->iova + gem->size;
>> +
>> + mutex_unlock(&fpriv->lock);
>> +
>> + err = xa_alloc(&ctx->mappings, &mapping_id, mapping,
>> + XA_LIMIT(1, U32_MAX), GFP_KERNEL);
>> + if (err < 0)
>> + goto unmap;
>> +
>> + args->mapping_id = mapping_id;
>> +
>> + return 0;
>> +
>> +unmap:
>> + if (mapping->sgt) {
>> + dma_unmap_sgtable(mapping->dev, mapping->sgt,
>> + mapping->direction, DMA_ATTR_SKIP_CPU_SYNC);
>> + }
>> +unpin:
>> + host1x_bo_unpin(mapping->dev, mapping->bo, mapping->sgt);
>> +put_gem:
>> + drm_gem_object_put(gem);
>> + kfree(mapping);
>> +unlock:
>> + mutex_unlock(&fpriv->lock);
>> + return err;
>> +}
>> +
>> +int tegra_drm_ioctl_channel_unmap(struct drm_device *drm, void *data,
>> + struct drm_file *file)
>> +{
>> + struct tegra_drm_file *fpriv = file->driver_priv;
>> + struct drm_tegra_channel_unmap *args = data;
>> + struct tegra_drm_channel_ctx *ctx;
>> + struct tegra_drm_mapping *mapping;
>> +
>> + ctx = tegra_drm_channel_ctx_lock(fpriv, args->channel_ctx);
>> + if (!ctx)
>> + return -EINVAL;
>> +
>> + mapping = xa_erase(&ctx->mappings, args->mapping_id);
>> +
>> + mutex_unlock(&fpriv->lock);
>> +
>> + if (mapping) {
>> + tegra_drm_mapping_put(mapping);
>> + return 0;
>> + } else {
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +int tegra_drm_ioctl_gem_create(struct drm_device *drm, void *data,
>> + struct drm_file *file)
>> +{
>> + struct drm_tegra_gem_create *args = data;
>> + struct tegra_bo *bo;
>> +
>> + if (args->flags)
>> + return -EINVAL;
>
> I'm not sure it's worth doing this, especially because this is now a new
> IOCTL that's actually a subset of the original. I think we should just
> keep the original and if we want to deprecate the flags, or replace them
> with new ones, let's just try and phase out the deprecated ones.
Ok, I'll look into it.
>
> Thierry
Thanks,
Mikko
>
>> +
>> + bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags,
>> + &args->handle);
>> + if (IS_ERR(bo))
>> + return PTR_ERR(bo);
>> +
>> + return 0;
>> +}
>> +
>> +int tegra_drm_ioctl_gem_mmap(struct drm_device *drm, void *data,
>> + struct drm_file *file)
>> +{
>> + struct drm_tegra_gem_mmap *args = data;
>> + struct drm_gem_object *gem;
>> + struct tegra_bo *bo;
>> +
>> + gem = drm_gem_object_lookup(file, args->handle);
>> + if (!gem)
>> + return -EINVAL;
>> +
>> + bo = to_tegra_bo(gem);
>> +
>> + args->offset = drm_vma_node_offset_addr(&bo->gem.vma_node);
>> +
>> + drm_gem_object_put(gem);
>> +
>> + return 0;
>> +}
>> --
>> 2.30.0
>>
More information about the dri-devel
mailing list