RFC: i915 arch changes to better support new chipsets

RFC: i915 arch changes to better support new chipsets

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1676 bytes --]

I've been talking with some people offline about potential changes to
our source structure to make it easier to add support for new chipsets,
prevent breaking chipsets one doesn't intend to affect, and generally
make the code more readable and maintainable.

I've got some new chipset code ready that's motivating some of this
work, because it moved all the display regs around, added a new unit in
low MMIO space, and generally affects gen7 paths with gen5 like
behavior.

The question is, how far should we go?  At the very least, I'd like to
propose the following:
  intel_display.c -> pch_display.c, gmch_display.c, potentially with
    additional sub-files for specific weirdness and power/drps
    functionality
  i915_irq.c -> per chipset irq files (i9xx, ironlake, ivb at the very
    least, with some additional duplication)
  new, range specific i915_read/write routines, e.g. i915_read_gt,
    i915_read_display to make forcewake and register block moves easier
    to handle

I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
beast.  Our goal to be to make it easy to add new definitions while
also making it easy to not accidentally use old an incorrect
definitions on a new platform.

Then obviously within those files there's lots of room for improvement,
for example in i9xx mode setting we still have some pretty massive
functions that need to be split (I have patches to do that).

Thoughts?  It may also make sense to split some of our port specific
files where they differ enough from previous platforms.  E.g. g4x DP vs
ironlake+...

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Daniel Vetter]

On Tue, Mar 20, 2012 at 11:13:57AM -0700, Jesse Barnes wrote:
> I've been talking with some people offline about potential changes to
> our source structure to make it easier to add support for new chipsets,
> prevent breaking chipsets one doesn't intend to affect, and generally
> make the code more readable and maintainable.
> 
> I've got some new chipset code ready that's motivating some of this
> work, because it moved all the display regs around, added a new unit in
> low MMIO space, and generally affects gen7 paths with gen5 like
> behavior.
> 
> The question is, how far should we go?  At the very least, I'd like to
> propose the following:
>   intel_display.c -> pch_display.c, gmch_display.c, potentially with
>     additional sub-files for specific weirdness and power/drps
>     functionality

Yeah, that seems pretty sensible. I also think that splitting out some of
the less crtc_modeset related display functionality would be good, e.g.
pageflip, cursor stuff (like we've done for planes already). One big
monsters is the init code, which for added hilarity seems to be the place
to do all kinds of once-per-poweron workarounds. I don't have a clear idea
what we should do there.

>   i915_irq.c -> per chipset irq files (i9xx, ironlake, ivb at the very
>     least, with some additional duplication)

I'm not too sure about that one - we have i915_irq.c, i915_dma.c and
i915_drv.c with rather ill-defined use-cases for all kinds of things. And
tons of dri1 legacy stuff splattered all accross. We might want to pull
out a few functionalities out of these first before splitting it up
according to chipsets (e.g. pulling out the error handler and gpu reset
crap, which is currently splattered over a few files). After that, not
much might be left and it might make sense to just leave the irq stuff
together.

>   new, range specific i915_read/write routines, e.g. i915_read_gt,
>     i915_read_display to make forcewake and register block moves easier
>     to handle

I'm not convinced whether this is a great idea if applied all over the
code. For specific cases where a block moves it's imo better to just add a
mmio base for that (like we're doing with the ring ctrl regs, which move
around quite a bit over the various rings and generations). Obviously
adding a small helper is good, but imo we should name it a bit more
specific (if possible).

> I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
> beast.  Our goal to be to make it easy to add new definitions while
> also making it easy to not accidentally use old an incorrect
> definitions on a new platform.

Close your eyes and just keep on adding gunk. Imo i915_reg.h is pretty
much a write-once file, and cscope can still keep up with the definitions.
So not a pain point for me.

> Then obviously within those files there's lots of room for improvement,
> for example in i9xx mode setting we still have some pretty massive
> functions that need to be split (I have patches to do that).
> 
> Thoughts?  It may also make sense to split some of our port specific
> files where they differ enough from previous platforms.  E.g. g4x DP vs
> ironlake+...

I've just talked about this a bit with Eugeni in the context of Haswell,
and I think we might want to hold of for that code until we move output
stuff all over the place.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: RFC: i915 arch changes to better support new chipsets

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 2622 bytes --]

On Tue, 20 Mar 2012 19:43:04 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> >   new, range specific i915_read/write routines, e.g. i915_read_gt,
> >     i915_read_display to make forcewake and register block moves easier
> >     to handle
> 
> I'm not convinced whether this is a great idea if applied all over the
> code. For specific cases where a block moves it's imo better to just add a
> mmio base for that (like we're doing with the ring ctrl regs, which move
> around quite a bit over the various rings and generations). Obviously
> adding a small helper is good, but imo we should name it a bit more
> specific (if possible).

So the problem with VLV is that it has a CedarTrail like display (so
some new registers, lots of moved bits, etc) but moved to 0x180000.

Not everything has moved, just enough to be painful.  For example, the
PLLs and some interrupt regs are still in the low range, but pipe &
display plane regs have moved.  So it's not as simple as doing a single
offset and applying it everywhere.  We need to only apply it for
certain regs, which means touching a bunch of read/write accesses that
don't already use a wrapper.  And for the ones that use a wrapper, like
PIPECONF, we'd need a dev_priv->display_mmio_offset or something?

I'm not happy with any solution here, but definitely don't want to
upstream my current hack (a new IS_DISPLAYREG() check in read/write
that adds the offset if needed).

> > I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
> > beast.  Our goal to be to make it easy to add new definitions while
> > also making it easy to not accidentally use old an incorrect
> > definitions on a new platform.
> 
> Close your eyes and just keep on adding gunk. Imo i915_reg.h is pretty
> much a write-once file, and cscope can still keep up with the definitions.
> So not a pain point for me.
> 
> > Then obviously within those files there's lots of room for improvement,
> > for example in i9xx mode setting we still have some pretty massive
> > functions that need to be split (I have patches to do that).
> > 
> > Thoughts?  It may also make sense to split some of our port specific
> > files where they differ enough from previous platforms.  E.g. g4x DP vs
> > ironlake+...
> 
> I've just talked about this a bit with Eugeni in the context of Haswell,
> and I think we might want to hold of for that code until we move output
> stuff all over the place.

Yeah I don't want to make HSW any harder than necessary; we can put off
splits there.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 1625 bytes --]

On Tue, Mar 20, 2012 at 17:13, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> > > Thoughts?  It may also make sense to split some of our port specific
> > > files where they differ enough from previous platforms.  E.g. g4x DP vs
> > > ironlake+...
> >
> > I've just talked about this a bit with Eugeni in the context of Haswell,
> > and I think we might want to hold of for that code until we move output
> > stuff all over the place.
>
> Yeah I don't want to make HSW any harder than necessary; we can put off
> splits there.
>

(I suspect those 2 paragraphs above are enough for a new phoronix article
on HSW already :)).

Yeah, I've been through the same issues as Jesse, and we were talking with
Daniel about this. Our intel_display.c is huge and unnecessarily complex,
and we need to split it at some point if we want to keep our mental sanity.
At least the split of it into intel_display.c and intel_pch_display.c would
simplify the things a lot; and maybe we could add intel_display_gen6.c,
intel_display_gen7.c, intel_display_pch_cpt.c and intel_display_pch_lpt.c
and move those hardware-specific stuff there for simplifying out tasks in
the future.

I was also thinking on adding a intel_workarounds.c for handling all the
W/As we have in one place, and having some support for calling them via
callbacks to run specific WAs during the modeset, after suspend/resume
cycle and so on.

For i915_regs.h, I don't think there is a need to split is for now. It is a
nice way to have all the registers in just one place, and at least I never
had a problem with it so far.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 2132 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Chris Wilson]

On Wed, 21 Mar 2012 08:42:37 -0300, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> I was also thinking on adding a intel_workarounds.c for handling all the
> W/As we have in one place, and having some support for calling them via
> callbacks to run specific WAs during the modeset, after suspend/resume
> cycle and so on.

Too much midlayer. The recipe for modesetting for a particular chipset
goes in e.g. intel_display_pch_ibx.c and that recipe includes the w/a
with the calls to common code in intel_display_pch.c and beyond. (It
should ideally be a straight lift out of the bspec of the modesetting
sequence.) Not the other way around. Otherwise you just have the same
tangle of code just split over many different files and the coupling
masked through complex assemblages of vtables.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: RFC: i915 arch changes to better support new chipsets

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 977 bytes --]

On Tue, 20 Mar 2012 13:13:47 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
> > > beast.  Our goal to be to make it easy to add new definitions while
> > > also making it easy to not accidentally use old an incorrect
> > > definitions on a new platform.
> > 
> > Close your eyes and just keep on adding gunk. Imo i915_reg.h is pretty
> > much a write-once file, and cscope can still keep up with the definitions.
> > So not a pain point for me.

Oh I forgot the most important thing here: how many f*cking places do
we need to add PCI IDs??!!

I'd really really like to see i915_reg.h shared to libdrm and used
directly by intel-gpu-tools and Mesa if at all possible, whether we
split it or not.  The IS_* and HAS_* macros should be in there as well,
with the PCI IDs, then we can just add this stuff in one place...

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1503 bytes --]

On Wed, 21 Mar 2012 13:41:21 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 20 Mar 2012 13:13:47 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
> > > > beast.  Our goal to be to make it easy to add new definitions while
> > > > also making it easy to not accidentally use old an incorrect
> > > > definitions on a new platform.
> > > 
> > > Close your eyes and just keep on adding gunk. Imo i915_reg.h is pretty
> > > much a write-once file, and cscope can still keep up with the definitions.
> > > So not a pain point for me.
> 
> Oh I forgot the most important thing here: how many f*cking places do
> we need to add PCI IDs??!!
> 
> I'd really really like to see i915_reg.h shared to libdrm and used
> directly by intel-gpu-tools and Mesa if at all possible, whether we
> split it or not.  The IS_* and HAS_* macros should be in there as well,
> with the PCI IDs, then we can just add this stuff in one place...

So, I was thinking about this, but the problem I see is that if we
settle on the PCI ID/gen-number identifier being in libdrm, then some
distro pulls a new libdrm with hsw bits, and ships the rest of old
userland that happily initializes and spits ivb packets at it.

I guess we could have the gen-number stuff be a union of
IS_IVB()/IS_HSW()/IS_VLV(), and switch chipset probing to using each of
those instead of just gen >= 4.

Does this sound sane?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1931 bytes --]

On Wed, 28 Mar 2012 12:35:53 -0700
Eric Anholt <eric@anholt.net> wrote:

> On Wed, 21 Mar 2012 13:41:21 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Tue, 20 Mar 2012 13:13:47 -0700
> > Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > > I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
> > > > > beast.  Our goal to be to make it easy to add new definitions while
> > > > > also making it easy to not accidentally use old an incorrect
> > > > > definitions on a new platform.
> > > > 
> > > > Close your eyes and just keep on adding gunk. Imo i915_reg.h is pretty
> > > > much a write-once file, and cscope can still keep up with the definitions.
> > > > So not a pain point for me.
> > 
> > Oh I forgot the most important thing here: how many f*cking places do
> > we need to add PCI IDs??!!
> > 
> > I'd really really like to see i915_reg.h shared to libdrm and used
> > directly by intel-gpu-tools and Mesa if at all possible, whether we
> > split it or not.  The IS_* and HAS_* macros should be in there as well,
> > with the PCI IDs, then we can just add this stuff in one place...
> 
> So, I was thinking about this, but the problem I see is that if we
> settle on the PCI ID/gen-number identifier being in libdrm, then some
> distro pulls a new libdrm with hsw bits, and ships the rest of old
> userland that happily initializes and spits ivb packets at it.
> 
> I guess we could have the gen-number stuff be a union of
> IS_IVB()/IS_HSW()/IS_VLV(), and switch chipset probing to using each of
> those instead of just gen >= 4.
> 
> Does this sound sane?

Yeah that might be better anyway, at least for the kernel where the
IS_GEN stuff is getting more and more overloaded and I'd like to move
away from it in some places.

So if you're ok with it, that sounds like a good approach.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 1112 bytes --]

On Wed, Mar 28, 2012 at 16:46, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> > I guess we could have the gen-number stuff be a union of
> > IS_IVB()/IS_HSW()/IS_VLV(), and switch chipset probing to using each of
> > those instead of just gen >= 4.
> >
> > Does this sound sane?
>
> Yeah that might be better anyway, at least for the kernel where the
> IS_GEN stuff is getting more and more overloaded and I'd like to move
> away from it in some places.
>
> So if you're ok with it, that sounds like a good approach.
>

I think that we could move away from the IS_GEN checks in most places
actually, and not just in some of them, by using the feature checks instead.

My latest branch reports gives `grep IS_GEN * | wc -l` = 112; and if we
look for recent chipsets, we have `grep IS_GEN[67] *` = 47. And most of
those checks have sub-checks as well for specific chip features or names.
So if we drop the IS_GEN macros, and just use the specific feature or GPU
name checks instead we should improve both the readability and decrease the
code complexity I think.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1555 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1411 bytes --]

On Wed, 28 Mar 2012 16:59:15 -0300, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> On Wed, Mar 28, 2012 at 16:46, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:
> 
> > > I guess we could have the gen-number stuff be a union of
> > > IS_IVB()/IS_HSW()/IS_VLV(), and switch chipset probing to using each of
> > > those instead of just gen >= 4.
> > >
> > > Does this sound sane?
> >
> > Yeah that might be better anyway, at least for the kernel where the
> > IS_GEN stuff is getting more and more overloaded and I'd like to move
> > away from it in some places.
> >
> > So if you're ok with it, that sounds like a good approach.
> >
> 
> I think that we could move away from the IS_GEN checks in most places
> actually, and not just in some of them, by using the feature checks instead.
> 
> My latest branch reports gives `grep IS_GEN * | wc -l` = 112; and if we
> look for recent chipsets, we have `grep IS_GEN[67] *` = 47. And most of
> those checks have sub-checks as well for specific chip features or names.
> So if we drop the IS_GEN macros, and just use the specific feature or GPU
> name checks instead we should improve both the readability and decrease the
> code complexity I think.

A grep for IS_GEN will be significantly under-counting the number of
places that gen numbers are used, given that Mesa uses intel->gen, and
the 2d driver tends to use INTEL_INFO(intel)->gen.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RFC: i915 arch changes to better support new chipsets

[Daniel Vetter]

On Wed, Mar 28, 2012 at 01:29:26PM -0700, Eric Anholt wrote:
> On Wed, 28 Mar 2012 16:59:15 -0300, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> > On Wed, Mar 28, 2012 at 16:46, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:
> > 
> > > > I guess we could have the gen-number stuff be a union of
> > > > IS_IVB()/IS_HSW()/IS_VLV(), and switch chipset probing to using each of
> > > > those instead of just gen >= 4.
> > > >
> > > > Does this sound sane?
> > >
> > > Yeah that might be better anyway, at least for the kernel where the
> > > IS_GEN stuff is getting more and more overloaded and I'd like to move
> > > away from it in some places.
> > >
> > > So if you're ok with it, that sounds like a good approach.
> > >
> > 
> > I think that we could move away from the IS_GEN checks in most places
> > actually, and not just in some of them, by using the feature checks instead.
> > 
> > My latest branch reports gives `grep IS_GEN * | wc -l` = 112; and if we
> > look for recent chipsets, we have `grep IS_GEN[67] *` = 47. And most of
> > those checks have sub-checks as well for specific chip features or names.
> > So if we drop the IS_GEN macros, and just use the specific feature or GPU
> > name checks instead we should improve both the readability and decrease the
> > code complexity I think.
> 
> A grep for IS_GEN will be significantly under-counting the number of
> places that gen numbers are used, given that Mesa uses intel->gen, and
> the 2d driver tends to use INTEL_INFO(intel)->gen.

Actually the kernel uses INTEL_INFO(dev)->gen, too, mostly where we want
to case switch on the gen number. And that happens at a fair amount of
places.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: RFC: i915 arch changes to better support new chipsets

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --]

On Wed, Mar 28, 2012 at 17:37, Daniel Vetter <daniel@ffwll.ch> wrote:

> > > My latest branch reports gives `grep IS_GEN * | wc -l` = 112; and if we
> > > look for recent chipsets, we have `grep IS_GEN[67] *` = 47. And most of
> > > those checks have sub-checks as well for specific chip features or
> names.
> > > So if we drop the IS_GEN macros, and just use the specific feature or
> GPU
> > > name checks instead we should improve both the readability and
> decrease the
> > > code complexity I think.
> >
> > A grep for IS_GEN will be significantly under-counting the number of
> > places that gen numbers are used, given that Mesa uses intel->gen, and
> > the 2d driver tends to use INTEL_INFO(intel)->gen.
>
> Actually the kernel uses INTEL_INFO(dev)->gen, too, mostly where we want
> to case switch on the gen number. And that happens at a fair amount of
> places.
>

Indeed, you are right, my grep skills failed on me here.

After looking on the way gen info is used in Mesa, yeah, I think it is not
really a solution to abandon it just yet. I came to think on the possible
feature checks to replace them, but I don't think it would be worth the
effort just to replace all the Gen checks with a different switch just for
the sake of doing it.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1780 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx