Due to the length of the patch there are many things to fix. This review
alone won't cover all of them, but is mainly an attempt to reduce the
amount of code and to split this.
On Wed, Mar 11, 2015 at 3:33 PM, Deepak Goyal <dgoyal@xxxxxxxxxx> wrote:
> It adds PMU boot support.It loads PMU
> firmware into PMU falcon.RM/Kernel driver
> receives INIT ack (through interrupt mechanism)
> from PMU when PMU boots with success.
This commit log is strangely formatted. You want to break lines of git
commit lots around column 70, not 50. Also don't forget the space after
the end of your sentences.
The log itself also lacks informative value, especially considering the
length of this patch. Please assume your reader is completely unfamiliar
with your work, and explain in detail what your patch does, even the
parts that seem obvious. Some questions that come to mind when reading
the log:
- What is the PMU firmware do?
- What is RM? (this is not the terminology used by Nouveau, so better to
avoid using it altogether)
- What value does this patch add to the project?
I understand that this patch clears the way for follow-up patches that
will actually add features. Please state this clearly in the log, and
explain what these features are. No code can be merged upstream until
its benefits are clearly understood.
Review follows, I have changed the order of files to comment on the
structures before the code.
> diff --git a/drm/nouveau/nvkm/subdev/pmu/priv.h
b/drm/nouveau/nvkm/subdev/pmu/priv.h
> index 998410563bfd..c4686e418582 100644
> --- a/drm/nouveau/nvkm/subdev/pmu/priv.h
> +++ b/drm/nouveau/nvkm/subdev/pmu/priv.h
> @@ -2,7 +2,91 @@
> #define __NVKM_PMU_PRIV_H__
> #include <subdev/pmu.h>
> #include <subdev/pmu/fuc/os.h>
> +#include <core/object.h>
> +#include <core/device.h>
> +#include <core/parent.h>
> +#include <core/mm.h>
> +#include <linux/rwsem.h>
> +#include <linux/slab.h>
> +#include <subdev/mmu.h>
> +#include <core/gpuobj.h>
>
> +static inline u32 u64_hi32(u64 n)
> +{
> + return (u32)((n >> 32) & ~(u32)0);
> +}
> +
> +static inline u32 u64_lo32(u64 n)
> +{
> + return (u32)(n & ~(u32)0);
> +}
Use the lower_32_bits() and upper_32_bits() macros instead.
> +
> +/* #define ALLOCATOR_DEBUG */
This line is useless...
> +
> +/* main struct */
... and this comment uninformative.
> +struct nvkm_pmu_allocator {
> +
> + char name[32]; /* name for allocator */
> +/*struct rb_root rb_root;*/ /* rb tree root for blocks */
Do not comment out members that we don't need. If it's unneeded, just
remove it.
> +
> + u32 base; /* min value of this linear
space */
> + u32 limit; /* max value = limit - 1 */
> +
> + unsigned long *bitmap; /* bitmap */
> +
> + struct gk20a_alloc_block *block_first; /* first block in list */
> + struct gk20a_alloc_block *block_recent; /* last visited block */
> +
> + u32 first_free_addr; /* first free addr, non-contigous
> + allocation preferred start,
> + in order to pick up small
holes */
> + u32 last_free_addr; /* last free addr, contiguous
> + allocation preferred start */
> + u32 cached_hole_size; /* max free hole size up to
> + last_free_addr */
> + u32 block_count; /* number of blocks */
> +
> + struct rw_semaphore rw_sema; /* lock */
> + struct kmem_cache *block_cache; /* slab cache */
> +
> + /* if enabled, constrain to [base, limit) */
> + struct {
> + bool enable;
> + u32 base;
> + u32 limit;
> + } constraint;
> +
> + int (*alloc)(struct nvkm_pmu_allocator *allocator,
> + u32 *addr, u32 len, u32 align);
> + int (*free)(struct nvkm_pmu_allocator *allocator,
> + u32 addr, u32 len, u32 align);
> +
> +};
> +
> +int nvkm_pmu_allocator_init(struct nvkm_pmu_allocator *allocator,
> + const char *name, u32 base, u32 size);
> +void nvkm_pmu_allocator_destroy(struct nvkm_pmu_allocator *allocator);
> +
> +int nvkm_pmu_allocator_block_alloc(struct nvkm_pmu_allocator *allocator,
> + u32 *addr, u32 len, u32 align);
> +
> +int nvkm_pmu_allocator_block_free(struct nvkm_pmu_allocator *allocator,
> + u32 addr, u32 len, u32 align);
So from the nvkm_pmu_allocator struct and these function prototypes,
this looks like a pretty casual address space allocator. Nouveau already
has such an allocator: nvkm_mm. Check it out, it will do all that you
need and you can remove a lot of code from this patch.
> +
> +#if defined(ALLOCATOR_DEBUG)
> +
> +#define allocator_dbg(alloctor, format, arg...)
\
> +do { \
> + if (1) \
> + pr_debug("nvkm_pmu_allocator (%s) %s: " format "\n",\
> + alloctor->name, __func__, ##arg);\
> +} while (0)
> +
> +#else /* ALLOCATOR_DEBUG */
> +
> +#define allocator_dbg(format, arg...)
I'd prefer if you use the nv_debug() macro in place of this one, but it
will go away with the allocator anyway...
> +
> +#endif /* ALLOCATOR_DEBUG */
> #define nvkm_pmu_create(p, e, o, d)
\
> nvkm_pmu_create_((p), (e), (o), sizeof(**d), (void **)d)
> #define nvkm_pmu_destroy(p)
\
> @@ -26,6 +110,179 @@ int _nvkm_pmu_ctor(struct nvkm_object *, struct
nvkm_object *,
> int _nvkm_pmu_init(struct nvkm_object *);
> int _nvkm_pmu_fini(struct nvkm_object *, bool);
> void nvkm_pmu_pgob(struct nvkm_pmu *pmu, bool enable);
> +#define PMU_PG_IDLE_THRESHOLD 15000
> +#define PMU_PG_POST_POWERUP_IDLE_THRESHOLD 1000000
I do not see these macros being used anywhere in your code.
> +
> +/* state transition :
> + OFF => [OFF_ON_PENDING optional] => ON_PENDING => ON => OFF
> + ON => OFF is always synchronized */
> +#define PMU_ELPG_STAT_OFF 0 /* elpg is off */
> +#define PMU_ELPG_STAT_ON 1 /* elpg is on */
> +/* elpg is off, ALLOW cmd has been sent, wait for ack */
> +#define PMU_ELPG_STAT_ON_PENDING 2
> +/* elpg is on, DISALLOW cmd has been sent, wait for ack */
> +#define PMU_ELPG_STAT_OFF_PENDING 3
> +/* elpg is off, caller has requested on, but ALLOW
> +cmd hasn't been sent due to ENABLE_ALLOW delay */
> +#define PMU_ELPG_STAT_OFF_ON_PENDING 4
Same here. If they are used by a future patch, introduce them at the
time they actually become useful.
> +
> +/* Falcon Register index */
> +#define PMU_FALCON_REG_R0 (0)
> +#define PMU_FALCON_REG_R1 (1)
> +#define PMU_FALCON_REG_R2 (2)
> +#define PMU_FALCON_REG_R3 (3)
> +#define PMU_FALCON_REG_R4 (4)
> +#define PMU_FALCON_REG_R5 (5)
> +#define PMU_FALCON_REG_R6 (6)
> +#define PMU_FALCON_REG_R7 (7)
> +#define PMU_FALCON_REG_R8 (8)
> +#define PMU_FALCON_REG_R9 (9)
> +#define PMU_FALCON_REG_R10 (10)
> +#define PMU_FALCON_REG_R11 (11)
> +#define PMU_FALCON_REG_R12 (12)
> +#define PMU_FALCON_REG_R13 (13)
> +#define PMU_FALCON_REG_R14 (14)
> +#define PMU_FALCON_REG_R15 (15)
> +#define PMU_FALCON_REG_IV0 (16)
> +#define PMU_FALCON_REG_IV1 (17)
> +#define PMU_FALCON_REG_UNDEFINED (18)
> +#define PMU_FALCON_REG_EV (19)
> +#define PMU_FALCON_REG_SP (20)
> +#define PMU_FALCON_REG_PC (21)
> +#define PMU_FALCON_REG_IMB (22)
> +#define PMU_FALCON_REG_DMB (23)
> +#define PMU_FALCON_REG_CSW (24)
> +#define PMU_FALCON_REG_CCR (25)
> +#define PMU_FALCON_REG_SEC (26)
> +#define PMU_FALCON_REG_CTX (27)
> +#define PMU_FALCON_REG_EXCI (28)
> +#define PMU_FALCON_REG_RSVD0 (29)
> +#define PMU_FALCON_REG_RSVD1 (30)
> +#define PMU_FALCON_REG_RSVD2 (31)
> +#define PMU_FALCON_REG_SIZE (32)
These ones are ok since it would not make sense to define only part of
the regs...
> +
> +/* Choices for pmu_state */
> +#define PMU_STATE_OFF 0 /* PMU is off */
> +#define PMU_STATE_STARTING 1 /* PMU is on, but not booted */
> +#define PMU_STATE_INIT_RECEIVED 2 /* PMU init message
received */
> +#define PMU_STATE_ELPG_BOOTING 3 /* PMU is booting */
> +#define PMU_STATE_ELPG_BOOTED 4 /* ELPG is initialized */
> +#define PMU_STATE_LOADING_PG_BUF 5 /* Loading PG buf */
> +#define PMU_STATE_LOADING_ZBC 6 /* Loading ZBC buf */
> +#define PMU_STATE_STARTED 7 /* Fully unitialized */
But here again, the last 5 states are not used yet, so please introduce
them as they become needed.
> +
> +#define PMU_QUEUE_COUNT 5
> +
> +#define PMU_MAX_NUM_SEQUENCES (256)
> +#define PMU_SEQ_BIT_SHIFT (5)
> +#define PMU_SEQ_TBL_SIZE \
> + (PMU_MAX_NUM_SEQUENCES >> PMU_SEQ_BIT_SHIFT)
> +
> +#define PMU_SHA1_GID_SIGNATURE 0xA7C66AD2
> +#define PMU_SHA1_GID_SIGNATURE_SIZE 4
> +
> +#define PMU_SHA1_GID_SIZE 16
> +
> +struct pmu_queue {
> +
Empty blank line.
> + /* used by hw, for BIOS/SMI queue */
> + u32 mutex_id;
> + u32 mutex_lock;
> + /* used by sw, for LPQ/HPQ queue */
> + struct mutex mutex;
> +
> + /* current write position */
> + u32 position;
> + /* physical dmem offset where this queue begins */
> + u32 offset;
> + /* logical queue identifier */
> + u32 id;
> + /* physical queue index */
> + u32 index;
> + /* in bytes */
> + u32 size;
> +
> + /* open-flag */
> + u32 oflag;
> + bool opened; /* opened implies locked */
> +};
> +
> +struct pmu_sha1_gid {
> + bool valid;
> + u8 gid[PMU_SHA1_GID_SIZE];
> +};
> +
> +struct pmu_sha1_gid_data {
> + u8 signature[PMU_SHA1_GID_SIGNATURE_SIZE];
> + u8 gid[PMU_SHA1_GID_SIZE];
> +};
> +
> +struct pmu_desc {
> +
Empty blank line.
> + struct pmu_ucode_desc *desc;
> + struct pmu_buf_desc ucode;
> +
> + struct pmu_buf_desc pg_buf;
This member doesn't seem to be needed now.
> + /* TBD: remove this if ZBC seq is fixed */
> + struct pmu_buf_desc seq_buf;
> + struct pmu_buf_desc trace_buf;
> + bool buf_loaded;
buf_loaded is never referenced in this code.
> +
> + struct pmu_sha1_gid gid_info;
> +
> + struct pmu_queue queue[PMU_QUEUE_COUNT];
> +
> + struct pmu_sequence *seq;
Wrong. pmu_sequence is defined in gk20a.h. This file is a generic one.
Why would PMUs for other GPUs embed GK20A-specific structures?
Actually it seems like the whole pmu_desc should be moved to
GK20A-specific files, since it is not used elsewhere for now.
> + unsigned long pmu_seq_tbl[PMU_SEQ_TBL_SIZE];
> + u32 next_seq_desc;
> +
> + struct pmu_mutex *mutex;
> + u32 mutex_cnt;
> +
> + struct mutex pmu_copy_lock;
> + struct mutex pmu_seq_lock;
> +
> + struct nvkm_pmu_allocator dmem;
So as explained above, this should be replaced by a nvkm_mm.
> +
> + u32 *ucode_image;
> + bool pmu_ready;
> +
> + u32 zbc_save_done;
Yet another unreferenced member...
> +
> + u32 stat_dmem_offset;
And another one.
> +
> + u32 elpg_stat;
And another one.
> +
> + int pmu_state;
> +
> +#define PMU_ELPG_ENABLE_ALLOW_DELAY_MSEC 1 /* msec */
And another one.
> + struct work_struct isr_workq;
> + struct mutex elpg_mutex; /* protect elpg enable/disable */
And another one.
> +/* disable -1, enable +1, <=0 elpg disabled, > 0 elpg enabled */
> + int elpg_refcnt;
Here too.
> +
> + bool initialized;
> +
> + void (*remove_support)(struct pmu_desc *pmu);
So this function pointer is set, but never called! Is it unneeded, or
have you forgot to call it when you should have?
> + bool sw_ready;
> + bool perfmon_ready;
Unneeded member again.
> +
> + u32 sample_buffer;
> + u32 load_shadow;
> + u32 load_avg;
> +
> + struct mutex isr_mutex;
> + bool isr_enabled;
> +
> + bool zbc_ready;
This is only set to false in the destroy() function, so I guess you
don't need this now...
> + unsigned long perfmon_events_cnt;
> + bool perfmon_sampling_enabled;
> + u8 pmu_mode;
> + u32 falcon_id;
> + u32 aelpg_param[5];
And all these 5 members are also not needed now it seems.
> + void *pmu_chip_data;
From how you are using this member (to store a pointer to a kzalloc'd
pmu_gk20a_data), it seems to be unneeded. Put the content of
pmu_gk20a_data into gk20a_pmu_priv, and get rid of both this member and
pmu_gk20a_data.
And actually since both members of pmu_gk20a_data are completely
unreferenced, they can be added in a later patch anyway, when they
actually become useful.
> + struct nvkm_pmu *pmu;
> +};
>
> struct nvkm_pmu_impl {
> struct nvkm_oclass base;
> @@ -39,5 +296,12 @@ struct nvkm_pmu_impl {
> } data;
>
> void (*pgob)(struct nvkm_pmu *, bool);
> + struct pmu_desc pmudata;
> };
> +
> +static inline struct nvkm_pmu *impl_from_pmu(struct pmu_desc *pmu)
> +{
> + return pmu->pmu;
> +}
> +
> #endif
> diff --git a/drm/nouveau/include/nvkm/subdev/pmu.h
b/drm/nouveau/include/nvkm/subdev/pmu.h
> index 7b86acc634a0..659b4e0ba02b 100644
> --- a/drm/nouveau/include/nvkm/subdev/pmu.h
> +++ b/drm/nouveau/include/nvkm/subdev/pmu.h
> @@ -1,7 +1,20 @@
> #ifndef __NVKM_PMU_H__
> #define __NVKM_PMU_H__
> #include <core/subdev.h>
> +#include <core/device.h>
> +#include <subdev/mmu.h>
> +#include <linux/debugfs.h>
>
> +struct pmu_buf_desc {
> + struct nvkm_gpuobj *pmubufobj;
> + struct nvkm_vma pmubufvma;
Your struct is already called "pmu_buf", so maybe call these members
"obj" and "vma" simply.
> + size_t size;
> +};
> +struct pmu_priv_vm {
> + struct nvkm_gpuobj *mem;
> + struct nvkm_gpuobj *pgd;
> + struct nvkm_vm *vm;
> +};
> struct nvkm_pmu {
> struct nvkm_subdev base;
>
> @@ -20,9 +33,20 @@ struct nvkm_pmu {
> u32 message;
> u32 data[2];
> } recv;
> -
> + wait_queue_head_t init_wq;
This wq is initialized and never used.
> + bool gr_initialised;
Member only written once.
> + struct dentry *debugfs;
> + struct pmu_buf_desc *pg_buf;
This member is never used, and by transition neither is the pg_buf of
struct pmu_desc.
> + struct pmu_priv_vm *pmuvm;
> int (*message)(struct nvkm_pmu *, u32[2], u32, u32, u32, u32);
> void (*pgob)(struct nvkm_pmu *, bool);
> + int (*pmu_mutex_acquire)(struct nvkm_pmu *, u32 id, u32 *token);
Never used because you are calling the function you affect to the
pointer directly in the code (which happens to also be called
pmu_mutex_acquire!)
> + int (*pmu_mutex_release)(struct nvkm_pmu *, u32 id, u32 *token);
Same here.
> + int (*pmu_load_norm)(struct nvkm_pmu *pmu, u32 *load);
> + int (*pmu_load_update)(struct nvkm_pmu *pmu);
> + void (*pmu_reset_load_counters)(struct nvkm_pmu *pmu);
> + void (*pmu_get_load_counters)(struct nvkm_pmu *pmu, u32
*busy_cycles,
> + u32 *total_cycles);
These four ones are never called. Introduce members and functions only
when they become needed.
> };
>
> static inline struct nvkm_pmu *
> diff --git a/drm/nouveau/nvkm/subdev/pmu/base.c
b/drm/nouveau/nvkm/subdev/pmu/base.c
> index 054b2d2eec35..6afd389b9764 100644
> --- a/drm/nouveau/nvkm/subdev/pmu/base.c
> +++ b/drm/nouveau/nvkm/subdev/pmu/base.c
> @@ -25,6 +25,114 @@
>
> #include <subdev/timer.h>
>
> +/* init allocator struct */
> +int nvkm_pmu_allocator_init(struct nvkm_pmu_allocator *allocator,
> + const char *name, u32 start, u32 len)
> +{
> + memset(allocator, 0, sizeof(struct nvkm_pmu_allocator));
> +
> + strncpy(allocator->name, name, 32);
> +
> + allocator->base = start;
> + allocator->limit = start + len - 1;
> +
> + allocator->bitmap = kcalloc(BITS_TO_LONGS(len), sizeof(long),
> + GFP_KERNEL);
> + if (!allocator->bitmap)
> + return -ENOMEM;
> +
> + allocator_dbg(allocator, "%s : base %d, limit %d",
> + allocator->name, allocator->base);
> +
> + init_rwsem(&allocator->rw_sema);
> +
> + allocator->alloc = nvkm_pmu_allocator_block_alloc;
> + allocator->free = nvkm_pmu_allocator_block_free;
> +
> + return 0;
> +}
> +
> +/* destroy allocator, free all remaining blocks if any */
> +void nvkm_pmu_allocator_destroy(struct nvkm_pmu_allocator *allocator)
> +{
> + down_write(&allocator->rw_sema);
> +
> + kfree(allocator->bitmap);
> +
> + memset(allocator, 0, sizeof(struct nvkm_pmu_allocator));
> +}
> +
> +/*
> + * *addr != ~0 for fixed address allocation. if *addr == 0, base addr is
> + * returned to caller in *addr.
> + *
> + * contiguous allocation, which allocates one block of
> + * contiguous address.
> +*/
> +int nvkm_pmu_allocator_block_alloc(struct nvkm_pmu_allocator *allocator,
> + u32 *addr, u32 len, u32 align)
> +{
> + unsigned long _addr;
> +
> + allocator_dbg(allocator, "[in] addr %d, len %d", *addr, len);
> +
> + if ((*addr != 0 && *addr < allocator->base) || /* check addr
range */
> + *addr + len > allocator->limit || /* check addr range */
> + *addr & (align - 1) || /* check addr alignment */
> + len == 0) /* check len */
> + return -EINVAL;
> +
> + len = ALIGN(len, align);
> + if (!len)
> + return -ENOMEM;
> +
> + down_write(&allocator->rw_sema);
> +
> + _addr = bitmap_find_next_zero_area(allocator->bitmap,
> + allocator->limit - allocator->base + 1,
> + *addr ? (*addr - allocator->base) : 0,
> + len,
> + align - 1);
> + if ((_addr > allocator->limit - allocator->base + 1) ||
> + (*addr && *addr != (_addr + allocator->base))) {
> + up_write(&allocator->rw_sema);
> + return -ENOMEM;
> + }
> +
> + bitmap_set(allocator->bitmap, _addr, len);
> + *addr = allocator->base + _addr;
> +
> + up_write(&allocator->rw_sema);
> +
> + allocator_dbg(allocator, "[out] addr %d, len %d", *addr, len);
> +
> + return 0;
> +}
> +
> +/* free all blocks between start and end */
> +int nvkm_pmu_allocator_block_free(struct nvkm_pmu_allocator *allocator,
> + u32 addr, u32 len, u32 align)
> +{
> + allocator_dbg(allocator, "[in] addr %d, len %d", addr, len);
> +
> + if (addr + len > allocator->limit || /* check addr range */
> + addr < allocator->base ||
> + addr & (align - 1)) /* check addr alignment */
> + return -EINVAL;
> +
> + len = ALIGN(len, align);
> + if (!len)
> + return -EINVAL;
> +
> + down_write(&allocator->rw_sema);
> + bitmap_clear(allocator->bitmap, addr - allocator->base, len);
> + up_write(&allocator->rw_sema);
> +
> + allocator_dbg(allocator, "[out] addr %d, len %d", addr, len);
> +
> + return 0;
> +}
> +
So all this code should go away when you switch to nvkm_mm. It was
out-of-place anyway: this is a standard address space allocator and has
nothing specific to PMU.
That's a lot of things to fix already, so I will hold my review of
pmu/gk20a.c for next time. Just a few remarks about the most obvious
problems though:
The file is a mess. Functions appear without any logical order, so you
end up making declarations that could be avoided if things were ordered
a bit better. For instance, pmu_read_message() is only used by
pmu_process_message(), but you have 3 functions between these two. A
logical ordering of the code makes it much easier to read: "building
blocks" functions first, more complex functions later. Ideally you would
end up with a C file that has no forward-declarations.
Again, some functions are absolutely not used, sometimes in worrying
ways. Examples are gk20a_pmu_destroy and gk20a_pmu_create_, but I
suspect there are others.
For gk20a_pmu_create_, I don't even know why it is here in the first
place. It seems like its code should be gk20a_pmu_ctor() instead, and it
sets function pointers that are apparently never called because they are
remaining NULL and things seem to go just fine?
This patch should definitely be split into different bits to allow a
more pleasant review. Right now it is almost impossible to understand
what it does. Suggestion for splitting:
1) Add firmware loading ability, bootstrap PMU (since these two tasks
cannot be separated I guess)
2) Add message receiving/posting ability
3) DebugFS support
This should be a good beginning to make things more readable. There are
other things to comment on, but let's start with this.
Keep in mind that upstreaming is more than just trying to make the
downstream code fit as-is in the upstream kernel. You need to reshape
things when it makes sense, and replace custom-built solutions with the
ones that already exist. Also important is to make sure you uncover
things in a logical way, in chunks as small as possible to the
unfamiliar reader can understand them (for this particular series I
don't think we can go with less-than-300-lines patches though). In other
words, it is ok to send a 3000 lines patch series, if everything appears
progressively and logically. A 3000 lines patch however is likely to be
frown upon.
Looking forward to seeing v2 and hopefully diving deeper into this -
good luck!
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
