[Intel-gfx] [RFC PATCH 0/3] vlv sideband refactoring

[Daniel Vetter]

On Wed, May 15, 2013 at 09:40:29PM +0300, Jani Nikula wrote:
> Okay, I'm stuck with this a bit, and whichever approach I choose I
> expect there to be a bunch of bikeshedding. So I won't polish this
> further before comments.
> 
> Both the intel_dpio_{read,write} and valleyview_{punit,nc}_{read,write}
> use the IOSF sideband interface. They access the same registers and do
> mostly the same stuff, but no shared code. There are even duplicate
> register defines for the same registers. Both have locking, but the
> former use dpio_lock and the latter rps.hw_lock. It's racy.
> 
> These patches refactor the sideband access to a single function that
> expects dpio_lock to be held. The dpio_lock is only used for sideband
> stuff, so it's a better match than rps.hw_lock for the purpose. The rps
> stuff still needs rps.hw_lock, since it's used to protect more than just
> the register access, so rps code will need to hold both locks.
> 
> The bikeshedding department:
> 
> 1) Do we need to propagate sideband errors from the functions (like the
>    valleyview_punit_* functions do), or, since the return values are
>    never checked anywhere anyway, can we just convert to the
>    intel_dpio_* style (functions return the register value only) for all
>    of them?

I vote for void return and obnoxious WARNs (WARNs are optional if you
want). There's usually not much we can do if the hw has gone south like
that.

> 2) There will be quite a few more ports. Add new wrappers for each of
>    them, or create generic read/write functions that need a port
>    argument?

Imo the wrappers aren't too bad right now still, i.e. we can re-bikeshed
this once it happens.

> 3) Should the rps stuff take dpio_lock at a higher level than the
>    wrappers? This is pretty much a requirement for the generic r/w
>    function.

I'm more unhappy about the inconsistency in locking, since the dpio lock
is now held around large swaths of crtc disable/enable code, but only very
small parts for the rps stuff. I expect this to eventually bite us if dpio
sideband usage keeps growing. But again something we can fix once it blows
up.

> 4) Does dpio really need a devfn different from the rest?

I have no idea about this. Definitely looks ... interesting though ;-) I
think Jesse should take a look here.

> 5) Your additional bikeshedding here. ;)

Dropped one to also move the sbi stuff around.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch