| Message ID | 1363178186-2017-11-git-send-email-tbergstrom@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On 15.03.2013 14:13, Thierry Reding wrote: > 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) > > ? We only need the true/false value, so changing signature makes sense. > >> +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. True, we can move the assignment to happen after checking the open_channel result. > >> +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; Ok, will do. > >> + args->value = host1x_syncpt_id(context->client->syncpts[args->param]); > > This could use a temporary variable "syncpt" to make it easier to read. Ok. > >> +static int tegra_drm_gem_create_ioctl(struct drm_device *drm, void *data, >> + struct drm_file *file_priv) > > tegra_drm_ioctl_gem_create()? Sure. > >> +{ >> + 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. Ok. > >> +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. Yeah, that changes the API semantics a bit, but in general there shouldn't be a need to just get the offset without doing the actual mmap. > >> +{ >> + 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. Ok. > >> + >> + 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. Ok, we'll add that as a patch to the series. > >> 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? Ok. > >> 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. Will fix. > >> 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. Yep, will do. > >> +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. Sure, mem_handle is anyway legacy naming. > >> + 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. Oops, the intention was to pass the table around, but I and Arto both missed that. We'll fix that. > >> +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. Ok, it might make a bit longer lines, but it doesn't look too bad. > >> + 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(). Yeah, we'll fix that. > >> diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h > [...] >> +struct tegra_drm_create { > > struct tegra_drm_gem_create? Yep. > >> + __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). This is a genuine mistake, offset should've been dropped. > >> +struct tegra_drm_get_offset { >> + __u32 handle; >> + __u32 offset; >> +}; > > struct tegra_drm_gem_mmap to go along with the name change of the IOCTL. Ok. > >> +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? This is actually an IOCTL that normally shouldn't be used. It's still there for testing. The intention with this is that user space can submit a job that waits for a sync point value. The user space can then investigate the state of the channel to verify that host1x driver is working correctly. After investigation, user space can increment the sync point to unblock the channel. It's a bit tricky to find out the correct sync point value to wait for, but for test cases we can usually hack it together by just shutting down all other users of host1x and reading current value. >> +#define DRM_TEGRA_NO_TIMEOUT (0xffffffff) > > No need for the parentheses. Sure, will remove. > >> +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. Ok. I propose we rename target to target_mem. We could also rename tegra_drm_waitchk.mem to cmdbuf_mem and tegra_drm_waitchk.offset to cmdbuf_offset for consistency. > >> +#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. Ok. Terje
On 15.03.2013 14:13, Thierry Reding wrote: > 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. Yeah, I think it's better if we leave this as is now. We should actually have one table for host1x and one for 2D. The host1x one should be shared between the drivers, but the table for client unit should be local to the driver. Let's take this up again when we have another driver. > >> +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. I actually ended up with pretty long lines when I tried this, so I hope it's ok to leave as is. > >> + gr2d = devm_kzalloc(dev, sizeof(*gr2d), GFP_KERNEL); >> + if (!gr2d) >> + return -ENOMEM; >> + >> + syncpts = devm_kzalloc(dev, sizeof(*syncpts), GFP_KERNEL); F.ex. this line would split two two lines. Otherwise the changes should be now pretty much ok. You sent a bunch of changes (thanks) that I merged to my tree. I'll just need to do some testing and re-split the patches and send v8. Terje
diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile index e85db5a..29c0c6b 100644 --- a/drivers/gpu/host1x/Makefile +++ b/drivers/gpu/host1x/Makefile @@ -16,4 +16,5 @@ ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG host1x-$(CONFIG_DRM_TEGRA) += drm/drm.o drm/fb.o drm/dc.o host1x-$(CONFIG_DRM_TEGRA) += drm/output.o drm/rgb.o drm/hdmi.o host1x-$(CONFIG_DRM_TEGRA) += drm/cma.o +host1x-$(CONFIG_DRM_TEGRA) += drm/gr2d.o obj-$(CONFIG_TEGRA_HOST1X) += host1x.o diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index 6af8081..0091632 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -211,11 +211,17 @@ static int __init tegra_host1x_init(void) err = platform_driver_register(&tegra_hdmi_driver); if (err < 0) goto unregister_dc; + + err = platform_driver_register(&tegra_gr2d_driver); + if (err < 0) + goto unregister_hdmi; #endif return 0; #ifdef CONFIG_DRM_TEGRA +unregister_hdmi: + platform_driver_unregister(&tegra_hdmi_driver); unregister_dc: platform_driver_unregister(&tegra_dc_driver); unregister_host1x: @@ -228,6 +234,7 @@ module_init(tegra_host1x_init); static void __exit tegra_host1x_exit(void) { #ifdef CONFIG_DRM_TEGRA + platform_driver_unregister(&tegra_gr2d_driver); platform_driver_unregister(&tegra_hdmi_driver); platform_driver_unregister(&tegra_dc_driver); #endif diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c index dbd4808..9f78f52 100644 --- a/drivers/gpu/host1x/drm/drm.c +++ b/drivers/gpu/host1x/drm/drm.c @@ -1,6 +1,6 @@ /* * Copyright (C) 2012 Avionic Design GmbH - * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved. + * Copyright (C) 2012-2013 NVIDIA CORPORATION. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -8,13 +8,21 @@ */ #include <asm/dma-iommu.h> +#include <drm/drm.h> +#include <drm/drmP.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/tegra_drm.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/dma-mapping.h> +#include "cma.h" +#include "dev.h" #include "drm.h" +#include "host1x_bo.h" #include "host1x_client.h" +#include "syncpt.h" #define DRIVER_NAME "tegra" #define DRIVER_DESC "NVIDIA Tegra graphics" @@ -77,8 +85,10 @@ static int host1x_parse_dt(struct host1x_drm *host1x) static const char * const compat[] = { "nvidia,tegra20-dc", "nvidia,tegra20-hdmi", + "nvidia,tegra20-gr2d", "nvidia,tegra30-dc", "nvidia,tegra30-hdmi", + "nvidia,tegra30-gr2d", }; unsigned int i; int err; @@ -273,9 +283,24 @@ static int tegra_drm_unload(struct drm_device *drm) static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp) { + struct host1x_drm_file *fpriv; + + fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL); + if (!fpriv) + return -ENOMEM; + + INIT_LIST_HEAD(&fpriv->contexts); + filp->driver_priv = fpriv; + return 0; } +static void host1x_drm_context_free(struct host1x_drm_context *context) +{ + context->client->ops->close_channel(context); + kfree(context); +} + static void tegra_drm_lastclose(struct drm_device *drm) { struct host1x_drm *host1x = drm->dev_private; @@ -283,7 +308,222 @@ static void tegra_drm_lastclose(struct drm_device *drm) drm_fbdev_cma_restore_mode(host1x->fbdev); } +static int tegra_drm_ioctl_syncpt_read(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + struct tegra_drm_syncpt_read_args *args = data; + struct host1x *host = host1x_get_host(drm->dev); + struct host1x_syncpt *sp = host1x_syncpt_get(host, args->id); + + if (!sp) + return -EINVAL; + + args->value = host1x_syncpt_read_min(sp); + return 0; +} + +static int tegra_drm_ioctl_syncpt_incr(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + struct tegra_drm_syncpt_incr_args *args = data; + struct host1x *host = host1x_get_host(drm->dev); + struct host1x_syncpt *sp = host1x_syncpt_get(host, args->id); + + if (!sp) + return -EINVAL; + + host1x_syncpt_incr(sp); + return 0; +} + +static int tegra_drm_ioctl_syncpt_wait(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + struct tegra_drm_syncpt_wait_args *args = data; + struct host1x *host = host1x_get_host(drm->dev); + struct host1x_syncpt *sp = host1x_syncpt_get(host, args->id); + + if (!sp) + return -EINVAL; + + return host1x_syncpt_wait(sp, args->thresh, args->timeout, + &args->value); +} + +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; +} + +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; + err = client->ops->open_channel(client, context); + if (err) + break; + + list_add(&context->list, &fpriv->contexts); + args->context = (uintptr_t)context; + return 0; + } + + kfree(context); + return err; +} + +static int tegra_drm_ioctl_close_channel(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + struct tegra_drm_open_channel_args *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 -EINVAL; + + list_del(&context->list); + host1x_drm_context_free(context); + + return 0; +} + +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; + + args->value = host1x_syncpt_id(context->client->syncpts[args->param]); + + return 0; +} + +static int tegra_drm_ioctl_submit(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + struct tegra_drm_submit_args *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; + + return context->client->ops->submit(context, args, drm, file_priv); +} + +static int tegra_drm_gem_create_ioctl(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + struct tegra_drm_create *args = data; + struct drm_gem_cma_object *cma_obj; + struct tegra_drm_bo *cma_bo; + int ret; + + cma_bo = kzalloc(sizeof(*cma_bo), GFP_KERNEL); + if (!cma_bo) + return -ENOMEM; + + host1x_bo_init(&cma_bo->base, &tegra_drm_bo_ops); + + cma_obj = &cma_bo->cma_obj; + ret = drm_gem_cma_object_init(drm, cma_obj, args->size); + if (ret) + goto err_cma_create; + + ret = drm_gem_handle_create(file_priv, &cma_obj->base, &args->handle); + if (ret) + goto err_handle_create; + + drm_gem_object_unreference(&cma_obj->base); + + return 0; + +err_handle_create: + drm_gem_cma_free_object(&cma_obj->base); +err_cma_create: + kfree(cma_bo); + return -ENOMEM; +} + +static int tegra_drm_ioctl_get_offset(struct drm_device *drm, void *data, + struct drm_file *file_priv) +{ + 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); + + args->offset = cma_obj->base.map_list.hash.key << PAGE_SHIFT; + + drm_gem_object_unreference(&cma_obj->base); + + return 0; +} + +static void tegra_drm_gem_free_object(struct drm_gem_object *obj) +{ + struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj); + struct tegra_drm_bo *cma_bo = + container_of(cma_obj, struct tegra_drm_bo, cma_obj); + + drm_gem_cma_object_deinit(obj); + kfree(cma_bo); +} + 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), }; 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); + } #ifdef CONFIG_DEBUG_FS @@ -407,7 +654,7 @@ struct drm_driver tegra_drm_driver = { .debugfs_cleanup = tegra_debugfs_cleanup, #endif - .gem_free_object = drm_gem_cma_free_object, + .gem_free_object = tegra_drm_gem_free_object, .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, diff --git a/drivers/gpu/host1x/drm/drm.h b/drivers/gpu/host1x/drm/drm.h index 0b8738f..6a8d7e3 100644 --- a/drivers/gpu/host1x/drm/drm.h +++ b/drivers/gpu/host1x/drm/drm.h @@ -1,6 +1,6 @@ /* * Copyright (C) 2012 Avionic Design GmbH - * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved. + * Copyright (C) 2012-2013 NVIDIA CORPORATION. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -17,6 +17,9 @@ #include <drm/drm_gem_cma_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fixed.h> +#include <drm/tegra_drm.h> + +#include "host1x.h" struct host1x_drm { struct drm_device *drm; @@ -38,9 +41,26 @@ struct host1x_drm { struct host1x_client; +struct host1x_drm_context { + struct host1x_client *client; + struct host1x_channel *channel; + struct list_head list; +}; + struct host1x_client_ops { int (*drm_init)(struct host1x_client *client, struct drm_device *drm); int (*drm_exit)(struct host1x_client *client); + int (*open_channel)(struct host1x_client *client, + struct host1x_drm_context *context); + void (*close_channel)(struct host1x_drm_context *context); + int (*submit)(struct host1x_drm_context *context, + struct tegra_drm_submit_args *args, + struct drm_device *drm, + struct drm_file *file_priv); +}; + +struct host1x_drm_file { + struct list_head contexts; }; struct host1x_client { @@ -49,6 +69,12 @@ struct host1x_client { const struct host1x_client_ops *ops; + enum host1x_class class; + struct host1x_channel *channel; + + struct host1x_syncpt **syncpts; + unsigned int num_syncpts; + struct list_head list; }; diff --git a/drivers/gpu/host1x/drm/gr2d.c b/drivers/gpu/host1x/drm/gr2d.c new file mode 100644 index 0000000..f060069 --- /dev/null +++ b/drivers/gpu/host1x/drm/gr2d.c @@ -0,0 +1,330 @@ +/* + * drivers/video/tegra/host/gr2d/gr2d.c + * + * Tegra Graphics 2D + * + * Copyright (c) 2012-2013, NVIDIA Corporation. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/export.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/clk.h> +#include <drm/tegra_drm.h> + +#include "channel.h" +#include "drm.h" +#include "drm/cma.h" +#include "job.h" +#include "host1x.h" +#include "host1x_bo.h" +#include "host1x_client.h" +#include "syncpt.h" + +struct gr2d { + struct host1x_client client; + struct clk *clk; + struct host1x_channel *channel; + unsigned long *addr_regs; +}; + +static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 reg); + +static int gr2d_client_init(struct host1x_client *client, + struct drm_device *drm) +{ + return 0; +} + +static int gr2d_client_exit(struct host1x_client *client) +{ + return 0; +} + +static int gr2d_open_channel(struct host1x_client *client, + struct host1x_drm_context *context) +{ + struct gr2d *gr2d = dev_get_drvdata(client->dev); + context->channel = host1x_channel_get(gr2d->channel); + + if (!context->channel) + return -ENOMEM; + + return 0; +} + +static void gr2d_close_channel(struct host1x_drm_context *context) +{ + host1x_channel_put(context->channel); +} + +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; +} + +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)); + if (err) + goto fail; + + mem_handle = handle_cma_to_host1x(drm, file_priv, cmdbuf.mem); + if (!mem_handle) + goto fail; + + host1x_job_add_gather(job, mem_handle, + cmdbuf.words, cmdbuf.offset); + num_cmdbufs--; + cmdbufs++; + } + + err = copy_from_user(job->relocarray, relocs, + sizeof(*relocs) * num_relocs); + if (err) + goto fail; + + while (num_relocs--) { + job->relocarray[num_relocs].cmdbuf_mem = + handle_cma_to_host1x(drm, file_priv, + (u32)job->relocarray[num_relocs].cmdbuf_mem); + job->relocarray[num_relocs].target = + handle_cma_to_host1x(drm, file_priv, + (u32)job->relocarray[num_relocs].target); + + if (!job->relocarray[num_relocs].target || + !job->relocarray[num_relocs].cmdbuf_mem) + goto fail; + } + + err = copy_from_user(job->waitchk, waitchks, + sizeof(*waitchks) * num_waitchks); + if (err) + 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) + job->timeout = args->timeout; + + err = host1x_job_submit(job); + if (err) + goto fail_submit; + + args->fence = job->syncpt_end; + + host1x_job_put(job); + return 0; + +fail_submit: + host1x_job_unpin(job); +fail: + host1x_job_put(job); + return err; +} + +static struct host1x_client_ops gr2d_client_ops = { + .drm_init = gr2d_client_init, + .drm_exit = gr2d_client_exit, + .open_channel = gr2d_open_channel, + .close_channel = gr2d_close_channel, + .submit = gr2d_submit, +}; + +static void gr2d_init_addr_reg_map(struct device *dev, struct gr2d *gr2d) +{ + const u32 gr2d_addr_regs[] = {0x1a, 0x1b, 0x26, 0x2b, 0x2c, 0x2d, 0x31, + 0x32, 0x48, 0x49, 0x4a, 0x4b, 0x4c}; + unsigned long *bitmap; + int i; + + bitmap = devm_kzalloc(dev, DIV_ROUND_UP(256, BITS_PER_BYTE), + GFP_KERNEL); + + for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); ++i) { + u32 reg = gr2d_addr_regs[i]; + bitmap[BIT_WORD(reg)] |= BIT_MASK(reg); + } + + gr2d->addr_regs = bitmap; +} + +static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 reg) +{ + struct gr2d *gr2d = dev_get_drvdata(dev); + + switch (class) { + case HOST1X_CLASS_HOST1X: + return reg == 0x2b; + case HOST1X_CLASS_GR2D: + reg &= 0xff; + if (gr2d->addr_regs[BIT_WORD(reg)] & BIT_MASK(reg)) + return 1; + default: + return 0; + } +} + +static const struct of_device_id gr2d_match[] = { + { .compatible = "nvidia,tegra30-gr2d" }, + { .compatible = "nvidia,tegra20-gr2d" }, + { }, +}; + +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; + + 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); + + return 0; +} + +static int __exit gr2d_remove(struct platform_device *pdev) +{ + struct host1x_drm *host1x = host1x_get_drm_data(pdev->dev.parent); + struct gr2d *gr2d = platform_get_drvdata(pdev); + int err; + + err = host1x_unregister_client(host1x, &gr2d->client); + if (err < 0) { + dev_err(&pdev->dev, "failed to unregister client: %d\n", err); + return err; + } + + host1x_syncpt_free(*gr2d->client.syncpts); + return 0; +} + +struct platform_driver tegra_gr2d_driver = { + .probe = gr2d_probe, + .remove = __exit_p(gr2d_remove), + .driver = { + .owner = THIS_MODULE, + .name = "gr2d", + .of_match_table = gr2d_match, + } +}; diff --git a/drivers/gpu/host1x/host1x.h b/drivers/gpu/host1x/host1x.h index bca6563..128cc9e 100644 --- a/drivers/gpu/host1x/host1x.h +++ b/drivers/gpu/host1x/host1x.h @@ -22,7 +22,8 @@ #define __LINUX_HOST1X_H enum host1x_class { - HOST1X_CLASS_HOST1X = 0x1 + HOST1X_CLASS_HOST1X = 0x1, + HOST1X_CLASS_GR2D = 0x51 }; #endif diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h new file mode 100644 index 0000000..6f7f7e6 --- /dev/null +++ b/include/drm/tegra_drm.h @@ -0,0 +1,131 @@ +/* + * Copyright (c) 2012-2013, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef _TEGRA_DRM_H_ +#define _TEGRA_DRM_H_ + +struct tegra_drm_create { + __u64 size; + u32 flags; + u32 handle; + u32 offset; + u32 padding; +}; + +struct tegra_drm_get_offset { + __u32 handle; + __u32 offset; +}; + +struct tegra_drm_syncpt_read_args { + __u32 id; + __u32 value; +}; + +struct tegra_drm_syncpt_incr_args { + __u32 id; + __u32 pad; +}; + +struct tegra_drm_syncpt_wait_args { + __u32 id; + __u32 thresh; + __u32 timeout; + __u32 value; +}; + +#define DRM_TEGRA_NO_TIMEOUT (0xffffffff) + +struct tegra_drm_open_channel_args { + __u32 class; + __u32 pad; + __u64 context; +}; + +struct tegra_drm_get_syncpoint { + __u64 context; + __u32 param; + __u32 value; +}; + +struct tegra_drm_syncpt_incr { + __u32 id; + __u32 incrs; +}; + +struct tegra_drm_cmdbuf { + __u32 mem; + __u32 offset; + __u32 words; + __u32 pad; +}; + +struct tegra_drm_reloc { + __u32 cmdbuf_mem; + __u32 cmdbuf_offset; + __u32 target; + __u32 target_offset; + __u32 shift; + __u32 pad; +}; + +struct tegra_drm_waitchk { + __u32 mem; + __u32 offset; + __u32 syncpt_id; + __u32 thresh; +}; + +struct tegra_drm_submit_args { + __u64 context; + __u32 num_syncpt_incrs; + __u32 num_cmdbufs; + __u32 num_relocs; + __u32 submit_version; + __u32 num_waitchks; + __u32 waitchk_mask; + __u32 timeout; + __u32 pad; + __u64 syncpt_incrs; + __u64 cmdbufs; + __u64 relocs; + __u64 waitchks; + __u32 fence; /* Return value */ + + __u32 reserved[5]; /* future expansion */ +}; + +#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) + +#endif