[Intel-gfx] [PATCH] drm/i915: Populate pipe_offsets[] & co. accurately

[Ville Syrjälä]

On Wed, Mar 06, 2019 at 09:00:01PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2019 at 10:55:01AM -0800, Lucas De Marchi wrote:
> > On Wed, Mar 06, 2019 at 07:55:33PM +0200, Ville Syrjälä wrote:
> > >On Wed, Mar 06, 2019 at 09:42:57AM -0800, Lucas De Marchi wrote:
> > >> On Wed, Mar 06, 2019 at 05:13:39PM +0200, Ville Syrjälä wrote:
> > >> >On Wed, Mar 06, 2019 at 02:55:58PM +0000, Chris Wilson wrote:
> > >> >> Quoting Ville Syrjälä (2019-03-06 14:52:11)
> > >> >> > On Wed, Mar 06, 2019 at 09:31:48AM +0000, Chris Wilson wrote:
> > >> >> > > Quoting Ville Syrjala (2019-03-05 19:29:05)
> > >> >> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >> >> > > >
> > >> >> > > > At some point people have started to assume that
> > >> >> > > > pipe_offsets[] & co. are only populated for pipes and whatnot
> > >> >> > > > that actually exist. That is in fact not currently true, but
> > >> >> > > > we can easily make it so.
> > >> >> > >
> > >> >> > > Any benefits of knock on effect?
> > >> >> >
> > >> >> > What kind of knock on effect we're thinking?
> > >> >>
> > >> >> Just wondering why people are eager to make the assumption that
> > >> >> non-existent pipes are not set. I presume its to make code neater.
> > >> >>
> > >> >> i.e. why cater to their whims at all?
> > >> >
> > >> >Yeah, I guess this was done just to avoid having convoluted platform
> > >> >checks all over. I've not checked the code to see if there are
> > >> >more places where we could simplify the existing code by adopting
> > >> >this approach.
> > >> >
> > >> >However now that you forced me to think a bit I realize that this
> > >> >may break in the presence of fused off pipes. Not quite sure how
> > >> >the registers for such fused off blocks would behave. If we aren't
> > >> >allowed to touch those registers we'd need to move this stuff
> > >> >into the runtime info. That feels a bit wasteful, so as an
> > >> >alternative we could just add one or two bitmasks instead.
> > >> >
> > >> >Cc:ing Lucas who seems to the main offender here...
> > >>
> > >> humn.. is this about the EDP transcoder? Missing some context here.
> > >> Did I miss any platform not setting trans_offsets for TRANSCODER_EDP,
> > >> even if it has? glancing over the possible mistake.... chv? :-/
> > >
> > >Currently .trans_offsets[TRANSCODER_EDP] != 0 on _every_ platform.
> > 
> > the commit was made to _allow_ platforms not to have the edp transcoder
> > so we don't need to keep adding platform checks when the needs arrise.
> 
> Most of the platforms don't have EDP transcoder.
> 
> > 
> > checking now that that probably broke chv though.
> 
> No, chv is fine. All pre-hsw platforms are busted though.

I pushed the patch so at least we should be closer to the old state
of things now.

Oh, and I just realized that BXT DSI is now also potentially broken.
The error capture code is going to attempt to read registers which
do not exist on those transcoders. Not sure if that's going to succeed
or not.

In fact, wasn't there something about the clock having to be on
before we can even access the DSI registers? Hmm, yes there was
(look for for bxt_dsi_pll_is_enabled()), but I'm not sure if the
transcoder registers were affected or not.

-- 
Ville Syrjälä
Intel