Re: [RFC][PATCH] drm/i915: Get rid of IS_DISPLAYREG()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 23, 2013 at 02:34:08PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 22, 2013 at 09:03:20PM +0100, Daniel Vetter wrote:

[cut]

> > On Tue, Jan 22, 2013 at 09:13:04PM +0200, ville.syrjala@linux.intel.com wrote:
> > Same thing about interrupt registers, imo those are ok as-is. The only
> > exception/share reg I've seen thus far is the PORT_HOTPLUG stuff. Since
> > that's much more display related than all the other irq fun, I guess we
> > could just leave it as-is.
> 
> I can leave the VLV_IIR & co. alone. But now that I think about I
> probably want to convert them to the (VLV_DISPLAY_BASE + x) format
> as well. Much easier to grep/seach.

Yeah, that might look better. The reason why I don't want to smash
everything into the same display_mmio_offset is that I kinda expect hw
guys to move this stuff around just for fun ...

[more cutting]

> > >  /* VGA port control */
> > > -#define ADPA			0x61100
> > > +#define ADPA			(dev_priv->info->mmio_offset + 0x61100)
> > >  #define PCH_ADPA                0xe1100
> > > -#define VLV_ADPA		(VLV_DISPLAY_BASE + ADPA)
> > 
> > The crt encoder should be fully converted to use platform regs already,
> > and has a case for VLV (using the above #define). I guess we could just
> > leave things as-is for now. If that looks strange, we can always change
> > things. But for the inital IS_DISPLAYREG kill patch I'd like to make it as
> > small as possible.
> 
> Sure. I can split it out (and try to split other things a bit
> better too). Then it's easier to decide which bits to take.

For  splitting I think it'd be nice if you do the conversions per register
block (or group of blocks if there's nothing interesting going on) all
while keeping display_mmio_offset == 0 for vlv. Then at the end we can rip
out IS_DISPLAYREG and set the mmio offset correctly.

[cut]

> > > +#define DPINVGTT				(dev_priv->info->mmio_offset + 0x7002c) /* VLV only */
> > >  #define   CURSORB_INVALID_GTT_INT_EN		(1<<23)
> > >  #define   CURSORA_INVALID_GTT_INT_EN		(1<<22)
> > >  #define   SPRITED_INVALID_GTT_INT_EN		(1<<21)
> > > @@ -2744,7 +2739,7 @@
> > >  #define   PLANEA_INVALID_GTT_STATUS		(1<<0)
> > >  #define   DPINVGTT_STATUS_MASK			0xff
> > >  
> > > -#define DSPARB			0x70030
> > > +#define DSPARB			(dev_priv->info->mmio_offset + 0x70030)
> > 
> > DSPARB is gen3/4 iirc, so no need to adjust.
> 
> According to my docs VLV has it too.

Hm, the only places where I've found it being used was in the gen2/3 fifo
size computation code, hence why I've thought it's not used on vlv. If the
reg matches what gen2/3 has in it, I guess we could leave the conversion
in the patch. Otherwise I think a new DSPAR_VLV (which then uses the
mmio_offset or VLV_DISPLAY_BASE) is probably better.

[cut]

> > Iirc the above is the ilk/snb sprite block, but vlv has a ivb/hsw-like
> > sprite block, which follows below. So again imo better to drop the above
> > hunk.
> 
> I dislike out the duplication in the sprite regs. All the regs seem
> pretty much identical even if their name has changed from DVS to
> SPR. It might make sense to have just some kind of sprite_offset
> handling. In fact all planes in general share quite a bit, so some
> kind of bigger refactoring may be warranted, especially when we get
> around to exposing the primary plane as a drm_plane.
> 
> But I'll drop the DVS register conversion for now.

Hm, unifying the sprite/plane stuff like that sounds like a good idea, if
we can extract the mmio offset at a decent place. Maybe we could create an
intel_hw_plane struct with mmio_offset and a bunch of vfuncs to setup
registers (source, sizes, pixel layout, ...) extracted from the current
code. intel_crtc/intel_sprite would then have such a thing embedded ...

Haven't looked through reg specs too much, but this might be useful in the
future.

[cut]

> > > -#define PIPEB_PP_STATUS         0x61300
> > > -#define PIPEB_PP_CONTROL        0x61304
> > > -#define PIPEB_PP_ON_DELAYS      0x61308
> > > -#define PIPEB_PP_OFF_DELAYS     0x6130c
> > > -#define PIPEB_PP_DIVISOR        0x61310
> > > +#define PIPEB_PP_STATUS         (dev_priv->info->mmio_offset + 0x61300)
> > > +#define PIPEB_PP_CONTROL        (dev_priv->info->mmio_offset + 0x61304)
> > > +#define PIPEB_PP_ON_DELAYS      (dev_priv->info->mmio_offset + 0x61308)
> > > +#define PIPEB_PP_OFF_DELAYS     (dev_priv->info->mmio_offset + 0x6130c)
> > > +#define PIPEB_PP_DIVISOR        (dev_priv->info->mmio_offset + 0x61310)
> > 
> > Blargh, another set of PP registers ... I'm confused.
> 
> The per pipe PP regs are for VLV in fact. Not sure they're used anywhere
> else. I suppose not (based  on the comment). I can drop the non-pipe PP
> regs from the conversion.
> 
> Although in fact PIPEA_PP == PP, so we could remove one of those sets
> completely. Not sure if it would cause confusion in the code using them.
> For now I'll leave the non-pipe PP regs be since this is going to need
> more work for VLV anyway.

Maybe just do the PP conversion in it's own patch and write an angry
commit message about this mess ;-)

[cut]

> > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > index 7f09041..599a1b3 100644
> > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > @@ -516,7 +516,7 @@ int intel_setup_gmbus(struct drm_device *dev)
> > >  	if (HAS_PCH_SPLIT(dev))
> > >  		dev_priv->gpio_mmio_base = PCH_GPIOA - GPIOA;
> > >  	else
> > > -		dev_priv->gpio_mmio_base = 0;
> > > +		dev_priv->gpio_mmio_base = dev_priv->info->mmio_offset;
> > 
> > Well, that works, too ;-) But I'd vote for an explicit IS_VLV case
> > here, since that stuff moved around with the PCH split already. And having
> > different magic ways to select the register offset doesn't look too good
> > imo.
> 
> If you want uniformity, we should just kill all these special offset
> handling thingys and convert everything over to the intel_device_info
> offset (even PCH vs. non-PCH if possible) ;)

Yeah, the gpio_mmio_base could be moved to intel_info, too. My comment
about consistency was more meant that for a given register we should stick
to just one mmio_offset, and not combine them in funny ways ;-) I think
your patch would actually have worked, since on PCH_SPLIT platforms
dev_priv->gpio_mmio_base has the right offset, and on vlv
info->mmio_offset has the right offset.

For all the other blocks (ports, planes, sprites, maybe PP) I think it's
better to keep the mmio_offset/base reg somewhere local in the struct. But
since gmbus/gpio exists only once, moving it sounds like a good idea.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch