Re: [PATCHv7 10/10] drm: tegra: Add gr2d device

On Wed, Mar 13, 2013 at 02:36:26PM +0200, Terje Bergstrom wrote: [...]

diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c

[...]

+static inline struct host1x_drm_context *tegra_drm_get_context(

• 			struct list_head *contexts,
  

• 			struct host1x_drm_context *_ctx)
  

+{

• struct host1x_drm_context *ctx;
• list_for_each_entry(ctx, contexts, list)

• if (ctx == _ctx)
  

• 	return ctx;
  

• return NULL;

+}

Maybe make this a little more high-level, such as:

static bool host1x_drm_file_owns_context(struct host1x_drm_file *file, struct host1x_drm_context *context)

?

+static int tegra_drm_ioctl_open_channel(struct drm_device *drm, void *data,

• 			struct drm_file *file_priv)
  

+{

• struct tegra_drm_open_channel_args *args = data;
• struct host1x_client *client;
• struct host1x_drm_context *context;
• struct host1x_drm_file *fpriv = file_priv->driver_priv;
• struct host1x_drm *host1x = drm->dev_private;
• int err = -ENODEV;
• context = kzalloc(sizeof(*context), GFP_KERNEL);
• if (!context)

• return -ENOMEM;
  

• list_for_each_entry(client, &host1x->clients, list)

• if (client->class == args->class) {
  

• 	context->client = client;
  

Why do you assign this here? Should it perhaps be assigned only when the opening of the channel succeeds? The .open_channel() already receives a pointer to the client as parameter, so it doesn't have to be associated with the context.

+static int tegra_drm_ioctl_get_syncpoint(struct drm_device *drm, void *data,

• 			 struct drm_file *file_priv)
  

+{

• struct tegra_drm_get_syncpoint *args = data;
• struct host1x_drm_file *fpriv = file_priv->driver_priv;
• struct host1x_drm_context *context =

• (struct host1x_drm_context *)(uintptr_t)args->context;
  

• if (!tegra_drm_get_context(&fpriv->contexts, context))

• return -ENODEV;
  

• if (context->client->num_syncpts < args->param)

• return -ENODEV;
  

I think this might be easier to read as:

if (args->param >= context->client->num_syncpts) return -ENODEV;

• args->value = host1x_syncpt_id(context->client->syncpts[args->param]);

This could use a temporary variable "syncpt" to make it easier to read.

+static int tegra_drm_gem_create_ioctl(struct drm_device *drm, void *data,

• 		      struct drm_file *file_priv)
  

tegra_drm_ioctl_gem_create()?

+{

• struct tegra_drm_create *args = data;
• struct drm_gem_cma_object *cma_obj;
• struct tegra_drm_bo *cma_bo;

These can probably just be named "cma"/"gem" and "bo" instead.

+static int tegra_drm_ioctl_get_offset(struct drm_device *drm, void *data,

• 		      struct drm_file *file_priv)
  

I think this might be more generically named tegra_drm_ioctl_mmap() which might come in handy if we ever need to do something more than just retrieve the offset.

+{

• struct tegra_drm_get_offset *args = data;
• struct drm_gem_cma_object *cma_obj;
• struct drm_gem_object *obj;
• obj = drm_gem_object_lookup(drm, file_priv, args->handle);
• if (!obj)

• return -EINVAL;
  

• cma_obj = to_drm_gem_cma_obj(obj);

The above two lines should be separated by a blank line.

• args->offset = cma_obj->base.map_list.hash.key << PAGE_SHIFT;

Perhaps a better way would be to export the get_gem_mmap_offset() from drivers/gpu/drm/drm_gem_cma_helper.c and reuse that.

static struct drm_ioctl_desc tegra_drm_ioctls[] = {

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_GEM_CREATE,

• 	tegra_drm_gem_create_ioctl, DRM_UNLOCKED | DRM_AUTH),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_SYNCPT_READ,

• 	tegra_drm_ioctl_syncpt_read, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_SYNCPT_INCR,

• 	tegra_drm_ioctl_syncpt_incr, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_SYNCPT_WAIT,

• 	tegra_drm_ioctl_syncpt_wait, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_OPEN_CHANNEL,

• 	tegra_drm_ioctl_open_channel, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_CLOSE_CHANNEL,

• 	tegra_drm_ioctl_close_channel, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_GET_SYNCPOINT,

• 	tegra_drm_ioctl_get_syncpoint, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_SUBMIT,

• 	tegra_drm_ioctl_submit, DRM_UNLOCKED),
  

• DRM_IOCTL_DEF_DRV(TEGRA_DRM_GEM_GET_OFFSET,

• 	tegra_drm_ioctl_get_offset, DRM_UNLOCKED),
  

};

Maybe resort these to put the GEM-specific IOCTLs closer together?

static const struct file_operations tegra_drm_fops = { @@ -345,10 +585,17 @@ static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe)

static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) {

• struct host1x_drm_file *fpriv = file->driver_priv;

• struct host1x_drm_context *context, *tmp; struct drm_crtc *crtc;

  list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) tegra_dc_cancel_page_flip(crtc, file);

• list_for_each_entry_safe(context, tmp, &fpriv->contexts, list)

• host1x_drm_context_free(context);
  

• kfree(fpriv);

Another missing blank line between these.

diff --git a/drivers/gpu/host1x/drm/gr2d.c b/drivers/gpu/host1x/drm/gr2d.c

[...]

+static struct host1x_bo *handle_cma_to_host1x(struct drm_device *drm,

• 			      struct drm_file *file_priv,
  

• 			      u32 gem_handle)
  

+{

• struct drm_gem_object *gem_obj;
• struct drm_gem_cma_object *cma_obj;
• struct tegra_drm_bo *cma_bo;
• gem_obj = drm_gem_object_lookup(drm, file_priv, gem_handle);
• if (!gem_obj)

• return 0;
  

• mutex_lock(&drm->struct_mutex);
• drm_gem_object_unreference(gem_obj);
• mutex_unlock(&drm->struct_mutex);
• cma_obj = to_drm_gem_cma_obj(gem_obj);
• cma_bo = container_of(cma_obj, struct tegra_drm_bo, cma_obj);
• return &cma_bo->base;

+}

Maybe rename this to host1x_bo_lookup()? *_to_* are usually used for up- and downcasting; this function does a lot more.

+static int gr2d_submit(struct host1x_drm_context *context,

•        struct tegra_drm_submit_args *args,
  

•        struct drm_device *drm,
  

•        struct drm_file *file_priv)
  

+{

• struct host1x_job *job;
• int num_cmdbufs = args->num_cmdbufs;
• int num_relocs = args->num_relocs;
• int num_waitchks = args->num_waitchks;
• struct tegra_drm_cmdbuf __user *cmdbufs =

• (void * __user)(uintptr_t)args->cmdbufs;
  

• struct tegra_drm_reloc __user *relocs =

• (void * __user)(uintptr_t)args->relocs;
  

• struct tegra_drm_waitchk __user *waitchks =

• (void * __user)(uintptr_t)args->waitchks;
  

• struct tegra_drm_syncpt_incr syncpt_incr;
• int err;
• /* We don't yet support other than one syncpt_incr struct per submit */
• if (args->num_syncpt_incrs != 1)

• return -EINVAL;
  

• job = host1x_job_alloc(context->channel, args->num_cmdbufs,

• 	       args->num_relocs, args->num_waitchks);
  

• if (!job)

• return -ENOMEM;
  

• job->num_relocs = args->num_relocs;
• job->num_waitchk = args->num_waitchks;
• job->client = (u32)args->context;
• job->class = context->client->class;
• job->serialize = true;
• while (num_cmdbufs) {

• struct tegra_drm_cmdbuf cmdbuf;
  

• struct host1x_bo *mem_handle;
  

• err = copy_from_user(&cmdbuf, cmdbufs, sizeof(cmdbuf));
  

Could use an blank line to separate variable declarations from the code. Also maybe rename mem_handle to bo which is much shorter.

• goto fail;
  

• err = host1x_job_pin(job, context->client->dev);
• if (err)

• goto fail;
  

• err = copy_from_user(&syncpt_incr,

• 	     (void * __user)(uintptr_t)args->syncpt_incrs,
  

• 	     sizeof(syncpt_incr));
  

• if (err)

• goto fail;
  

• job->syncpt_id = syncpt_incr.id;
• job->syncpt_incrs = syncpt_incr.incrs;
• job->timeout = 10000;
• job->is_addr_reg = gr2d_is_addr_reg;
• if (args->timeout && args->timeout < 10000)

Another missing blank line.

Also you now create a lookup table (or bitfield actually) as we discussed but you still pass around a function to check the lookup table against. What I originally intended was to not pass a function around at all but directly use the lookup table/bitfield. However I think we can leave this as-is for now and change it later if required.

+static int gr2d_probe(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct host1x_drm *host1x = host1x_get_drm_data(dev->parent);
• int err;
• struct gr2d *gr2d = NULL;
• struct host1x_syncpt **syncpts;

I don't think there's a need for this temporary variable. You could just use gr2d->client.syncpts directly.

• gr2d = devm_kzalloc(dev, sizeof(*gr2d), GFP_KERNEL);
• if (!gr2d)

• return -ENOMEM;
  

• syncpts = devm_kzalloc(dev, sizeof(*syncpts), GFP_KERNEL);
• if (!syncpts)

• return -ENOMEM;
  

• gr2d->clk = devm_clk_get(dev, NULL);
• if (IS_ERR(gr2d->clk)) {

• dev_err(dev, "cannot get clock\n");
  

• return PTR_ERR(gr2d->clk);
  

• }
• err = clk_prepare_enable(gr2d->clk);
• if (err) {

• dev_err(dev, "cannot turn on clock\n");
  

• return err;
  

• }
• gr2d->channel = host1x_channel_alloc(dev);
• if (!gr2d->channel)

• return -ENOMEM;
  

• *syncpts = host1x_syncpt_request(dev, 0);
• if (!(*syncpts)) {

• host1x_channel_free(gr2d->channel);
  

• return -ENOMEM;
  

• }
• gr2d->client.ops = &gr2d_client_ops;
• gr2d->client.dev = dev;
• gr2d->client.class = HOST1X_CLASS_GR2D;
• gr2d->client.syncpts = syncpts;
• gr2d->client.num_syncpts = 1;
• err = host1x_register_client(host1x, &gr2d->client);
• if (err < 0) {

• dev_err(dev, "failed to register host1x client: %d\n", err);
  

• return err;
  

• }
• gr2d_init_addr_reg_map(dev, gr2d);
• dev_set_drvdata(dev, gr2d);

Nit: I think it'd be nicer to use platform_set_drvdata() here to mirror the platform_get_drvdata() from gr2d_remove().

diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h

[...]

+struct tegra_drm_create {

struct tegra_drm_gem_create?

• __u64 size;
• u32 flags;
• u32 handle;
• u32 offset;
• u32 padding;

Also since we have a separate IOCTL for this, I think you can leave out the offset field (and also padding since it isn't required anymore).

+struct tegra_drm_get_offset {

• __u32 handle;
• __u32 offset;

+};

struct tegra_drm_gem_mmap to go along with the name change of the IOCTL.

+struct tegra_drm_syncpt_incr_args {

• __u32 id;
• __u32 pad;

+};

Shouldn't this second field be something like "value" to allow this IOCTL to increment by more than 1?

+#define DRM_TEGRA_NO_TIMEOUT (0xffffffff)

No need for the parentheses.

+struct tegra_drm_reloc {

• __u32 cmdbuf_mem;
• __u32 cmdbuf_offset;
• __u32 target;
• __u32 target_offset;
• __u32 shift;
• __u32 pad;

+};

I've found this to be a little inconsistent. Why does the cmdbuf_mem have the _mem suffix, but not the target field? Given that this will eventually be used by userspace software once merged it will be fixed forever. I had noticed the same for the in-kernel structures but didn't think it important enough to hold up the patches. In this case I think we should make them consistent, though.

+#define DRM_TEGRA_DRM_GEM_CREATE 0x00 +#define DRM_TEGRA_DRM_SYNCPT_READ 0x01 +#define DRM_TEGRA_DRM_SYNCPT_INCR 0x02 +#define DRM_TEGRA_DRM_SYNCPT_WAIT 0x03 +#define DRM_TEGRA_DRM_OPEN_CHANNEL 0x04 +#define DRM_TEGRA_DRM_CLOSE_CHANNEL 0x05 +#define DRM_TEGRA_DRM_GET_SYNCPOINT 0x06 +#define DRM_TEGRA_DRM_SUBMIT 0x08 +#define DRM_TEGRA_DRM_GEM_GET_OFFSET 0x09

+#define DRM_IOCTL_TEGRA_DRM_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_GEM_CREATE, struct tegra_drm_create) +#define DRM_IOCTL_TEGRA_DRM_SYNCPT_READ DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SYNCPT_READ, struct tegra_drm_syncpt_read_args) +#define DRM_IOCTL_TEGRA_DRM_SYNCPT_INCR DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SYNCPT_INCR, struct tegra_drm_syncpt_incr_args) +#define DRM_IOCTL_TEGRA_DRM_SYNCPT_WAIT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SYNCPT_WAIT, struct tegra_drm_syncpt_wait_args) +#define DRM_IOCTL_TEGRA_DRM_OPEN_CHANNEL DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_OPEN_CHANNEL, struct tegra_drm_open_channel_args) +#define DRM_IOCTL_TEGRA_DRM_CLOSE_CHANNEL DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_CLOSE_CHANNEL, struct tegra_drm_open_channel_args) +#define DRM_IOCTL_TEGRA_DRM_GET_SYNCPOINT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_GET_SYNCPOINT, struct tegra_drm_get_syncpoint) +#define DRM_IOCTL_TEGRA_DRM_SUBMIT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SUBMIT, struct tegra_drm_submit_args) +#define DRM_IOCTL_TEGRA_DRM_GEM_GET_OFFSET DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_GEM_GET_OFFSET, struct tegra_drm_get_offset)

Same comment regarding reordering and GET_OFFSET -> MMAP rename.

Thierry