Message-ID: <20130205074301.GA20437@avionic-0098.mockup.avionic-design.de>
Date:	Tue, 5 Feb 2013 08:43:01 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Terje Bergström <tbergstrom@...dia.com>
Cc:	Arto Merilainen <amerilainen@...dia.com>,
	"airlied@...ux.ie" <airlied@...ux.ie>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

On Mon, Feb 04, 2013 at 07:30:08PM -0800, Terje Bergström wrote:
> On 04.02.2013 01:09, Thierry Reding wrote:
> > On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
> >> Add host1x, the driver for host1x and its client unit 2D.
> > 
> > Maybe this could be a bit more verbose. Perhaps describe what host1x is.
> 
> Sure. I could just steal the paragraph from Stephen:
> 
> The Tegra host1x module is the DMA engine for register access to Tegra's
> graphics- and multimedia-related modules. The modules served by host1x
> are referred to as clients. host1x includes some other  functionality,
> such as synchronization.

Yes, that sound good.

> >> +     err = host1x_syncpt_init(host);
> >> +     if (err)
> >> +             return err;
> > [...]
> >> +     host1x_syncpt_reset(host);
> > 
> > Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
> > it might be useful to have host1x_syncpt_reset() as a separate function
> > but couldn't it be called as part of host1x_syncpt_init()?
> 
> host1x_syncpt_init() is used for initializing the syncpt structures, and
> is called in probe. host1x_syncpt_reset() should be called whenever we
> think hardware state is lost, for example if VDD_CORE was rail gated due
> to system suspend.

My point was that you could include the call to host1x_syncpt_reset()
within host1x_syncpt_init(). That will keep unneeded code out of the
host1x_probe() function. Also you don't want to use the syncpoints
uninitialized, right?

> >> +#include "hw/syncpt_hw.c"
> > 
> > Why include the source file here? Can't you compile it separately
> > instead?
> 
> It's because we need to compile with the hardware headers of that host1x
> version, because we haven't been good at keeping compatibility. So
> host1x01.c #includes version 01 headers, and syncpt_hw.c in this
> compilation unit gets compiled with that. 02 would include 02 headers,
> and syncpt_hw.c would get compiled with its register definitions etc.

Okay, fair enough.

> >> + */
> >> +static u32 syncpt_load_min(struct host1x_syncpt *sp)
> >> +{
> >> +     struct host1x *dev = sp->dev;
> >> +     u32 old, live;
> >> +
> >> +     do {
> >> +             old = host1x_syncpt_read_min(sp);
> >> +             live = host1x_sync_readl(dev,
> >> +                             HOST1X_SYNC_SYNCPT_0 + sp->id * 4);
> >> +     } while ((u32)atomic_cmpxchg(&sp->min_val, old, live) != old);
> > 
> > I think this warrants a comment.
> 
> Sure. It just loops in case there's a race writing to min_val.

Oh, I see. That'd make a good comment. Is the cast to (u32) really
necessary?

> >> +/*
> >> + * Resets syncpoint and waitbase values to sw shadows
> >> + */
> >> +void host1x_syncpt_reset(struct host1x *dev)
> > 
> > Maybe host1x_syncpt_flush() would be a better name given the above
> > description? Reset does have this hardware reset connotation so my first
> > intuition had been that this would reset the syncpt value to 0.
> 
> Right, it actually reloads values stored in shadow registers back to
> host1x. Flush doesn't feel like it's conveying the meaning. Would
> host1x_syncpt_restore() work? That'd match with host1x_syncpt_save(),
> which just updates all shadow registers from hardware and is used just
> before host1x loses power.

Save/restore has the disadvantage of the direction not being implicit.
Save could mean save to hardware or save to software. The same is true
for restore. However if the direction is clearly defined, save and
restore work for me.

Maybe the comment could be changed to be more explicit. Something like:

	/*
	 * Write cached syncpoint and waitbase values to hardware.
	 */

And for host1x_syncpt_save():

	/*
	 * For client-managed registers, update the cached syncpoint and
	 * waitbase values by reading from the registers.
	 */

> >> +/*
> >> + * Updates the last value read from hardware.
> >> + */
> >> +u32 host1x_syncpt_load_min(struct host1x_syncpt *sp)
> >> +{
> >> +     u32 val;
> >> +     val = sp->dev->syncpt_op.load_min(sp);
> >> +     trace_host1x_syncpt_load_min(sp->id, val);
> >> +
> >> +     return val;
> >> +}
> > 
> > I don't know I understand what this means exactly. Does it read the
> > value that hardware last incremented? Perhaps this will become clearer
> > when you add a comment to the syncpt_load_min() implementation.
> 
> It just loads the current syncpt value to shadow register. The shadow
> register is called min, because host1x tracks the range of sync point
> increments that hardware is still going to do, so min is the lower
> boundary of the range.
> 
> max tells what the sync point is expected to reach for hardware to be
> considered idle.
> 
> host1x will f.ex. nop out waits for sync point values outside the range,
> because hardware isn't good at handling syncpt value wrapping.

Maybe the function should be called host1x_syncpt_load() if there is no
equivalent way to load the maximum value (since there is no register to
read from).

> > Also the syncpoint is not actually allocated here, so maybe
> > host1x_syncpt_request() would be a better name. As a nice side-effect it
> > makes the naming more similar to the IRQ API and might be easier to work
> > with.
> 
> I'm not entirely sure about the difference, but isn't the number to be
> allocated usually passed to a function ending in _request? Allocate
> would just allocate the next available - as host1x_syncpt_allocate does.

That's certainly true for interrupts. However, if you look at the DMA
subsystem for example, you can also request an unnamed resource.

The difference is sufficiently subtle that host1x_syncpt_allocate()
would work for me too, though. I just have a slight preference for
host1x_syncpt_request().

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists