Re: [RFC v2 3/8] video: tegra: host: Add channel and client support

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3064 bytes --]

On Thu, Nov 29, 2012 at 01:00:40PM +0200, Terje Bergström wrote:
> On 29.11.2012 12:04, Thierry Reding wrote:
> > Looking some more at how this is used, I'm starting to think that it
> > might be easier to export the various handlers and allow them to be
> > passed to the nvhost_intr_add_action() explicitly.
> 
> Oh, so you mean like "nvhost_intr_add_action(intr, id, threshold,
> nvhost_intr_action_submit_complete, channel, waiter, priv), and
> nvhost_intr_action_submit_complete is the function pointer?
> 
> There's one case to take care of: we merge the waits for the jobs into
> one waiter to save us from having too many irq calls. Perhaps that could
> be handled by a flag, or something like that.

Yes, something like ACTION_MERGE or something should work fine.
Alternatively you could handle it by providing two public functions, one
which adds to the list of jobs that can be merged, the other that adds
to the list that cannot be merged.

> >> +struct nvhost_job *nvhost_job_alloc(struct nvhost_channel *ch,
> >> +             int num_cmdbufs, int num_relocs, int num_waitchks)
> >> +{
> >> +     struct nvhost_job *job = NULL;
> >> +     size_t size = job_size(num_cmdbufs, num_relocs, num_waitchks);
> >> +
> >> +     if (!size)
> >> +             return NULL;
> >> +     job = vzalloc(size);
> > 
> > Why vzalloc()?
> 
> I guess it's basically moot, but we tried that when we had some memory
> fragmentation issues and it was left even though we did find out it's
> not needed.

I think kzalloc() would be a better choice here. Also, while at it you
may want to make the num_* parameters unsigned.

> >> +     }
> >> +
> >> +     /* get current syncpt values for waitchk */
> >> +     for_each_set_bit(i, &waitchk_mask[0], sizeof(waitchk_mask))
> >> +             nvhost_syncpt_update_min(sp, i);
> > 
> > Or since you only use the mask here, why not move the
> > nvhost_syncpt_update_min() into the above loop?
> 
> I want to call nvhost_syncpt_update_min() only once per syncpt register.
> If the job has 100 sync point increments for 2D sync point, I'd read the
> value from hardware 100 times, which is expensive.

Right, hadn't thought about the fact that you can have multiple waits
for a single syncpoint in the job.

Looking at the code again, I see that you use sizeof(waitchk_mask) as
the third parameter to the for_each_set_bit() macro. However the size
parameter is to be specified in bits, not bytes.

Also the name nvhost_syncpt_update_min() has had me confused. So say it
is used to update the value that you've cached in software from the real
value in the register. However I interpret update_min() as "update the
minimum value in the register". Maybe something like *_load_min() would
be clearer.

> Thanks. I've collected a massive amount of feedback already. v3 will
> take quite a while to appear after we've finished all the reviews of v2.

Yes, that should keep you busy for quite a while. =) But I also think
we've made good progress so far.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]