Hi Terje,
I applied your patches on top of upstream 1224 kernel. Then I read the codes. So here is my review comments(I use "git diff" to print out, check below). I admit it's easy for me to not need to find the corresponding lines in your 8 patch mails, but I've no idea whether it is ok for you. If this makes you not feeling good, I'll do this in old ways. Basically, I think in this diff output, there are filename/line number/function name, so it should not be a hard work for you to understand my comments.
P.S: I haven't finished the review so here is what I found today.
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c index a936902..28954b3 100644 --- a/drivers/gpu/drm/tegra/gr2d.c +++ b/drivers/gpu/drm/tegra/gr2d.c @@ -247,6 +247,10 @@ static int __devinit gr2d_probe(struct platform_device *dev) { int err; struct gr2d *gr2d = NULL; + /* Cause drm_device is created in host1x driver. So + if host1x drivers loads after tegradrm, then in this + gr2d_probe function, this "drm_device" will be NULL. + How can we handle this? Defer driver probe? */ struct platform_device *drm_device = host1x_drm_device(to_platform_device(dev->dev.parent)); struct tegradrm *tegradrm = platform_get_drvdata(drm_device); @@ -273,6 +277,11 @@ static int __devinit gr2d_probe(struct platform_device *dev)
gr2d->syncpt = host1x_syncpt_alloc(dev, 0); if (!gr2d->syncpt) { + /* Do we really need this? + After "host1x_channel_alloc", the refcount of this + channel is 0. So calling host1x_channel_put here will + make the refcount going to negative. + I suppose we should call "host1x_free_channel" here. */ host1x_channel_put(gr2d->channel); return -ENOMEM; } @@ -284,6 +293,7 @@ static int __devinit gr2d_probe(struct platform_device *dev)
err = tegra_drm_register_client(tegradrm, &gr2d->client); if (err) + /* Add "host1x_free_channel" */ return err;
platform_set_drvdata(dev, gr2d); diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c index 3705cae..ed83b9e 100644 --- a/drivers/gpu/host1x/channel.c +++ b/drivers/gpu/host1x/channel.c @@ -95,6 +95,9 @@ struct host1x_channel *host1x_channel_alloc(struct platform_device *pdev) int max_channels = host1x->info.nb_channels; int err;
+ /* Is it necessary to add mutex protection here? + I'm just wondering in a smp system, multiple host1x clients + may try to alloc their channels simultaneously... */ if (chindex > max_channels) return NULL;
diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c index 86d5c70..f2bd5aa 100644 --- a/drivers/gpu/host1x/debug.c +++ b/drivers/gpu/host1x/debug.c @@ -164,6 +164,10 @@ static const struct file_operations host1x_debug_fops = {
void host1x_debug_init(struct host1x *host1x) { + /* I think it's better to set this directory name the same with + the driver's name -- defined in dev.c: + #define DRIVER_NAME "tegra-host1x" + */ struct dentry *de = debugfs_create_dir("tegra_host", NULL);
if (!de) diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index 07e8813..01ed10d 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -38,6 +38,7 @@
struct host1x *host1x;
+/* Called by drm unlocked ioctl function. So do we need a lock here? */ void host1x_syncpt_incr_byid(u32 id) { struct host1x_syncpt *sp = host1x->syncpt + id; @@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id) } EXPORT_SYMBOL(host1x_syncpt_read_byid);
+/* Called by drm unlocked ioctl function. So do we need a lock here? */ int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value) { struct host1x_syncpt *sp = host1x->syncpt + id; @@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)
err = host1x_alloc_resources(host); if (err) { + /* We don't have chip_ops right now, so here the + error message is somewhat improper */ dev_err(&dev->dev, "failed to init chip support\n"); goto fail; } @@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev) if (!host->syncpt) goto fail;
+ /* I suggest create a dedicate function for initializing nop sp. + First this "_host1x_syncpt_alloc" looks like an internal/static + function. + Then actually "host1x_syncpt_alloc" & "_host1x_syncpt_alloc" all + serve host1x client(e.g: gr2d) so it's not suitable to use them + for nop sp. + Just create a wrapper function to call _host1x_syncpt_alloc is OK. + This will make the code more readable. */ host->nop_sp = _host1x_syncpt_alloc(host, NULL, 0); if (!host->nop_sp) goto fail; @@ -191,6 +203,7 @@ static int host1x_probe(struct platform_device *dev) }
err = clk_prepare_enable(host->clk); + /* Add a dev_err here */ if (err < 0) goto fail;
@@ -209,9 +222,12 @@ static int host1x_probe(struct platform_device *dev) return 0;
fail: + /* host1x->syncpt isn't released here... */ + /* host1x->intr isn't release here... */ + /* Remove debugfs stuffs? */ host1x_syncpt_free(host->nop_sp); host1x_unregister_drm_device(host); - kfree(host); + kfree(host); /* not necessary*/ return err; }
@@ -222,6 +238,7 @@ static int __exit host1x_remove(struct platform_device *dev) host1x_syncpt_deinit(host); host1x_unregister_drm_device(host); clk_disable_unprepare(host->clk); + /* Remove debugfs stuffs? */ return 0; }
diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h index 05f8544..58f4c71 100644 --- a/drivers/gpu/host1x/dev.h +++ b/drivers/gpu/host1x/dev.h @@ -36,6 +36,7 @@ struct platform_device; struct output;
struct host1x_channel_ops { + /* Seems no one uses this member. Remove it. */ const char *soc_name; int (*init)(struct host1x_channel *, struct host1x *, @@ -125,12 +126,18 @@ struct host1x { struct host1x_syncpt *syncpt; struct host1x_intr intr; struct platform_device *dev; + + /* Seems no one uses this. Remove it. */ atomic_t clientid; struct host1x_device_info info; struct clk *clk;
+ /* Put some comments for this member. + For guys who're not familiar with nop sp, I think they'll + definitely get confused about this. */ struct host1x_syncpt *nop_sp;
+ /* Seems no one uses this member. Remove it. */ const char *soc_name; struct host1x_channel_ops channel_op; struct host1x_cdma_ops cdma_op; @@ -140,6 +147,7 @@ struct host1x { struct host1x_intr_ops intr_op;
struct host1x_channel chlist; + int allocated_channels;
struct dentry *debugfs; diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c index efcb9be..e112001 100644 --- a/drivers/gpu/host1x/intr.c +++ b/drivers/gpu/host1x/intr.c @@ -329,8 +329,13 @@ void host1x_intr_deinit(struct host1x_intr *intr) void host1x_intr_start(struct host1x_intr *intr, u32 hz) { struct host1x *host1x = intr_to_host1x(intr); + + /* Why we need to lock here? Seems like this function is + called by host1x's probe only. */ mutex_lock(&intr->mutex);
+ /* "init_host_sync" has already been called in function + host1x_intr_init. Remove this line. */ host1x->intr_op.init_host_sync(intr); host1x->intr_op.set_host_clocks_per_usec(intr, DIV_ROUND_UP(hz, 1000000)); diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c index 07cbca5..9a234ad 100644 --- a/drivers/gpu/host1x/syncpt.c +++ b/drivers/gpu/host1x/syncpt.c @@ -309,6 +309,8 @@ struct host1x_syncpt *host1x_syncpt_init(struct host1x *host) struct host1x_syncpt *syncpt, *sp; int i;
+ /* Consider devm_kzalloc here. Then you can forget the release + stuffs about this "syncpt". */ syncpt = sp = kzalloc(sizeof(struct host1x_syncpt) * host->info.nb_pts, GFP_KERNEL); if (!syncpt)
Mark On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
This set of patches adds support for Tegra20 and Tegra30 host1x and 2D. It is based on linux-next-20121220.
The fourth version has only few changes compared to previous version:
- Fixed some sparse warnings
- Fixed host1x Makefile to allow building as module
- Fixed host1x module unload
- Fixed tegradrm unload sequence
- host1x creates DRM dummy device and tegradrm uses it for storing DRM related data.
Some of the issues left open:
- Register definitions still use static inline. There has been a debate about code style versus ability to use compiler type checking and code coverage analysis. There was no conclusion, so I left it as was.
- Uses still CMA helpers. IOMMU support will replace CMA with host1x specific allocator.
host1x is the driver that controls host1x hardware. It supports host1x command channels, synchronization, and memory management. It is sectioned into logical driver under drivers/gpu/host1x and physical driver under drivers/host1x/hw. The physical driver is compiled with the hardware headers of the particular host1x version.
The hardware units are described (briefly) in the Tegra2 TRM. Wiki page https://gitorious.org/linux-tegra-drm/pages/Host1xIntroduction also contains a short description of the functionality.
The patch set removes responsibility of host1x from tegradrm. At the same time, it moves all drm related infrastructure in drivers/gpu/drm/tegra/host1x.c to drm.c. To replace the host1x central device, host1x creates a dummy device for tegradrm to hang global data to. This is a break in responsibility split of tegradrm taking care of DRM and host1x driver taking care of host1x and clients, but this structure was insisted. I've kept creation of dummy device as a separate patch to illustrate the alternatives. It can be later squashed into other patches.
The patch set adds 2D driver to tegradrm, which uses host1x for communicating with host1x to access sync points and channels. We expect to use the same infrastructure for other host1x clients, so we have kept host1x and tegradrm separate.
The patch set also adds user space API to tegradrm for accessing host1x and 2D.
Arto Merilainen (1): drm: tegra: Remove redundant host1x
Terje Bergstrom (7): gpu: host1x: Add host1x driver gpu: host1x: Add syncpoint wait and interrupts gpu: host1x: Add channel support gpu: host1x: Add debug support ARM: tegra: Add board data and 2D clocks drm: tegra: Add gr2d device gpu: host1x: Register DRM dummy device
arch/arm/mach-tegra/board-dt-tegra20.c | 1 + arch/arm/mach-tegra/board-dt-tegra30.c | 1 + arch/arm/mach-tegra/tegra20_clocks_data.c | 2 +- arch/arm/mach-tegra/tegra30_clocks_data.c | 1 + drivers/gpu/Makefile | 1 + drivers/gpu/drm/tegra/Kconfig | 2 +- drivers/gpu/drm/tegra/Makefile | 2 +- drivers/gpu/drm/tegra/dc.c | 26 +- drivers/gpu/drm/tegra/drm.c | 433 ++++++++++++++++++- drivers/gpu/drm/tegra/drm.h | 72 ++-- drivers/gpu/drm/tegra/fb.c | 17 +- drivers/gpu/drm/tegra/gr2d.c | 309 ++++++++++++++ drivers/gpu/drm/tegra/hdmi.c | 30 +- drivers/gpu/drm/tegra/host1x.c | 325 -------------- drivers/gpu/host1x/Kconfig | 28 ++ drivers/gpu/host1x/Makefile | 17 + drivers/gpu/host1x/cdma.c | 475 ++++++++++++++++++++ drivers/gpu/host1x/cdma.h | 107 +++++ drivers/gpu/host1x/channel.c | 137 ++++++ drivers/gpu/host1x/channel.h | 64 +++ drivers/gpu/host1x/cma.c | 117 +++++ drivers/gpu/host1x/cma.h | 43 ++ drivers/gpu/host1x/debug.c | 207 +++++++++ drivers/gpu/host1x/debug.h | 49 +++ drivers/gpu/host1x/dev.c | 242 +++++++++++ drivers/gpu/host1x/dev.h | 167 ++++++++ drivers/gpu/host1x/drm.c | 51 +++ drivers/gpu/host1x/drm.h | 25 ++ drivers/gpu/host1x/hw/Makefile | 6 + drivers/gpu/host1x/hw/cdma_hw.c | 480 +++++++++++++++++++++ drivers/gpu/host1x/hw/cdma_hw.h | 37 ++ drivers/gpu/host1x/hw/channel_hw.c | 147 +++++++ drivers/gpu/host1x/hw/debug_hw.c | 399 +++++++++++++++++ drivers/gpu/host1x/hw/host1x01.c | 46 ++ drivers/gpu/host1x/hw/host1x01.h | 25 ++ drivers/gpu/host1x/hw/host1x01_hardware.h | 150 +++++++ drivers/gpu/host1x/hw/hw_host1x01_channel.h | 98 +++++ drivers/gpu/host1x/hw/hw_host1x01_sync.h | 179 ++++++++ drivers/gpu/host1x/hw/hw_host1x01_uclass.h | 130 ++++++ drivers/gpu/host1x/hw/intr_hw.c | 178 ++++++++ drivers/gpu/host1x/hw/syncpt_hw.c | 157 +++++++ drivers/gpu/host1x/intr.c | 377 ++++++++++++++++ drivers/gpu/host1x/intr.h | 106 +++++ drivers/gpu/host1x/job.c | 618 +++++++++++++++++++++++++++ drivers/gpu/host1x/memmgr.c | 174 ++++++++ drivers/gpu/host1x/memmgr.h | 53 +++ drivers/gpu/host1x/syncpt.c | 398 +++++++++++++++++ drivers/gpu/host1x/syncpt.h | 134 ++++++ drivers/video/Kconfig | 2 + include/drm/tegra_drm.h | 131 ++++++ include/linux/host1x.h | 217 ++++++++++ include/trace/events/host1x.h | 296 +++++++++++++ 52 files changed, 7095 insertions(+), 394 deletions(-) create mode 100644 drivers/gpu/drm/tegra/gr2d.c delete mode 100644 drivers/gpu/drm/tegra/host1x.c create mode 100644 drivers/gpu/host1x/Kconfig create mode 100644 drivers/gpu/host1x/Makefile create mode 100644 drivers/gpu/host1x/cdma.c create mode 100644 drivers/gpu/host1x/cdma.h create mode 100644 drivers/gpu/host1x/channel.c create mode 100644 drivers/gpu/host1x/channel.h create mode 100644 drivers/gpu/host1x/cma.c create mode 100644 drivers/gpu/host1x/cma.h create mode 100644 drivers/gpu/host1x/debug.c create mode 100644 drivers/gpu/host1x/debug.h create mode 100644 drivers/gpu/host1x/dev.c create mode 100644 drivers/gpu/host1x/dev.h create mode 100644 drivers/gpu/host1x/drm.c create mode 100644 drivers/gpu/host1x/drm.h create mode 100644 drivers/gpu/host1x/hw/Makefile create mode 100644 drivers/gpu/host1x/hw/cdma_hw.c create mode 100644 drivers/gpu/host1x/hw/cdma_hw.h create mode 100644 drivers/gpu/host1x/hw/channel_hw.c create mode 100644 drivers/gpu/host1x/hw/debug_hw.c create mode 100644 drivers/gpu/host1x/hw/host1x01.c create mode 100644 drivers/gpu/host1x/hw/host1x01.h create mode 100644 drivers/gpu/host1x/hw/host1x01_hardware.h create mode 100644 drivers/gpu/host1x/hw/hw_host1x01_channel.h create mode 100644 drivers/gpu/host1x/hw/hw_host1x01_sync.h create mode 100644 drivers/gpu/host1x/hw/hw_host1x01_uclass.h create mode 100644 drivers/gpu/host1x/hw/intr_hw.c create mode 100644 drivers/gpu/host1x/hw/syncpt_hw.c create mode 100644 drivers/gpu/host1x/intr.c create mode 100644 drivers/gpu/host1x/intr.h create mode 100644 drivers/gpu/host1x/job.c create mode 100644 drivers/gpu/host1x/memmgr.c create mode 100644 drivers/gpu/host1x/memmgr.h create mode 100644 drivers/gpu/host1x/syncpt.c create mode 100644 drivers/gpu/host1x/syncpt.h create mode 100644 include/drm/tegra_drm.h create mode 100644 include/linux/host1x.h create mode 100644 include/trace/events/host1x.h