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 at 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
