On Tue, Jan 15, 2013 at 01:44:00PM +0200, Terje Bergstrom wrote:
> diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
[...]
> +static pid_t host1x_debug_null_kickoff_pid;
> +unsigned int host1x_debug_trace_cmdbuf;
> +
> +static pid_t host1x_debug_force_timeout_pid;
> +static u32 host1x_debug_force_timeout_val;
> +static u32 host1x_debug_force_timeout_channel;
Please group static and non-static variables.
> diff --git a/drivers/gpu/host1x/debug.h b/drivers/gpu/host1x/debug.h
[...]
> +struct output {
> + void (*fn)(void *ctx, const char *str, size_t len);
> + void *ctx;
> + char buf[256];
> +};
Do we really need this kind of abstraction? There really should be only
one location where debug information is obtained, so I don't see a need
for this.
> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
> struct host1x_syncpt_ops {
> void (*reset)(struct host1x_syncpt *);
> void (*reset_wait_base)(struct host1x_syncpt *);
> @@ -117,6 +133,7 @@ struct host1x {
> struct host1x_channel_ops channel_op;
> struct host1x_cdma_ops cdma_op;
> struct host1x_pushbuffer_ops cdma_pb_op;
> + struct host1x_debug_ops debug_op;
> struct host1x_syncpt_ops syncpt_op;
> struct host1x_intr_ops intr_op;
Again, better to pass in a const pointer to the ops structure.
> diff --git a/drivers/gpu/host1x/hw/debug_hw.c b/drivers/gpu/host1x/hw/debug_hw.c
> +static int show_channel_command(struct output *o, u32 addr, u32 val, int *count)
> +{
> + unsigned mask;
> + unsigned subop;
> +
> + switch (val >> 28) {
> + case 0x0:
These can easily be derived by looking at the debug output, but it may
still make sense to assign symbolic names to them.
> +static void show_channel_word(struct output *o, int *state, int *count,
> + u32 addr, u32 val, struct host1x_cdma *cdma)
> +{
> + static int start_count, dont_print;
What if two processes read debug information at the same time?
> +static void do_show_channel_gather(struct output *o,
> + phys_addr_t phys_addr,
> + u32 words, struct host1x_cdma *cdma,
> + phys_addr_t pin_addr, u32 *map_addr)
> +{
> + /* Map dmaget cursor to corresponding mem handle */
> + u32 offset;
> + int state, count, i;
> +
> + offset = phys_addr - pin_addr;
> + /*
> + * Sometimes we're given different hardware address to the same
> + * page - in these cases the offset will get an invalid number and
> + * we just have to bail out.
> + */
Why's that?
> + map_addr = host1x_memmgr_mmap(mem);
> + if (!map_addr) {
> + host1x_debug_output(o, "[could not mmap]\n");
> + return;
> + }
> +
> + /* Get base address from mem */
> + sgt = host1x_memmgr_pin(mem);
> + if (IS_ERR(sgt)) {
> + host1x_debug_output(o, "[couldn't pin]\n");
> + host1x_memmgr_munmap(mem, map_addr);
> + return;
> + }
Maybe you should stick with one of "could not" or "couldn't". Makes it
easier to search for.
> +static void show_channel_gathers(struct output *o, struct host1x_cdma *cdma)
> +{
> + struct host1x_job *job;
> +
> + list_for_each_entry(job, &cdma->sync_queue, list) {
> + int i;
> + host1x_debug_output(o,
> + "\n%p: JOB, syncpt_id=%d, syncpt_val=%d,"
> + " first_get=%08x, timeout=%d"
> + " num_slots=%d, num_handles=%d\n",
> + job,
> + job->syncpt_id,
> + job->syncpt_end,
> + job->first_get,
> + job->timeout,
> + job->num_slots,
> + job->num_unpins);
This could go on fewer lines.
> +static void host1x_debug_show_channel_cdma(struct host1x *m,
> + struct host1x_channel *ch, struct output *o, int chid)
> +{
[...]
> + switch (cbstat) {
> + case 0x00010008:
Again, symbolic names would be nice.
Thierry
Attachment:
pgpS2VdeI8b2C.pgp
Description: PGP signature
