diff mbox series

drm/i915: Populate pipe_offsets[] & co. accurately

Message ID 20190305192905.7140-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Populate pipe_offsets[] & co. accurately | expand

Commit Message

Ville Syrjälä March 5, 2019, 7:29 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@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.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c | 146 +++++++++++++++++++++++---------
 1 file changed, 104 insertions(+), 42 deletions(-)

Comments

Chris Wilson March 6, 2019, 9:31 a.m. UTC | #1 
Quoting Ville Syrjala (2019-03-05 19:29:05)
> From: Ville Syrjälä <ville.syrjala@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?

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pci.c | 146 +++++++++++++++++++++++---------
>  1 file changed, 104 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index c42c5ccf38fe..9e610e4bdd7d 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -35,7 +35,37 @@
>  #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
>  #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
>  
> -#define GEN_DEFAULT_PIPEOFFSETS \
> +#define I845_PIPE_OFFSETS \
> +       .pipe_offsets = { \
> +               [TRANSCODER_A] = PIPE_A_OFFSET, \
> +       }, \
> +       .trans_offsets = { \
> +               [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> +       }
> +
> +#define I9XX_PIPE_OFFSETS \
> +       .pipe_offsets = { \
> +               [TRANSCODER_A] = PIPE_A_OFFSET, \
> +               [TRANSCODER_B] = PIPE_B_OFFSET, \
> +       }, \
> +       .trans_offsets = { \
> +               [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> +               [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> +       }
> +
> +#define IVB_PIPE_OFFSETS \
> +       .pipe_offsets = { \
> +               [TRANSCODER_A] = PIPE_A_OFFSET, \
> +               [TRANSCODER_B] = PIPE_B_OFFSET, \
> +               [TRANSCODER_C] = PIPE_C_OFFSET, \
> +       }, \
> +       .trans_offsets = { \
> +               [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> +               [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> +               [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> +       }
> +
> +#define HSW_PIPE_OFFSETS \
>         .pipe_offsets = { \
>                 [TRANSCODER_A] = PIPE_A_OFFSET, \
>                 [TRANSCODER_B] = PIPE_B_OFFSET, \
> @@ -49,7 +79,7 @@
>                 [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
>         }
>  
> -#define GEN_CHV_PIPEOFFSETS \
> +#define CHV_PIPE_OFFSETS \
>         .pipe_offsets = { \
>                 [TRANSCODER_A] = PIPE_A_OFFSET, \
>                 [TRANSCODER_B] = PIPE_B_OFFSET, \
> @@ -61,11 +91,30 @@
>                 [TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
>         }
>  
> -#define CURSOR_OFFSETS \
> -       .cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
> +#define I845_CURSOR_OFFSETS \
> +       .cursor_offsets = { \
> +               [PIPE_A] = CURSOR_A_OFFSET, \
> +       }
> +
> +#define I9XX_CURSOR_OFFSETS \
> +       .cursor_offsets = { \
> +               [PIPE_A] = CURSOR_A_OFFSET, \
> +               [PIPE_B] = CURSOR_B_OFFSET, \
> +       }
> +
> +#define CHV_CURSOR_OFFSETS \
> +       .cursor_offsets = { \
> +               [PIPE_A] = CURSOR_A_OFFSET, \
> +               [PIPE_B] = CURSOR_B_OFFSET, \
> +               [PIPE_C] = CHV_CURSOR_C_OFFSET, \
> +       }
>  
>  #define IVB_CURSOR_OFFSETS \
> -       .cursor_offsets = { CURSOR_A_OFFSET, IVB_CURSOR_B_OFFSET, IVB_CURSOR_C_OFFSET }
> +       .cursor_offsets = { \
> +               [PIPE_A] = CURSOR_A_OFFSET, \
> +               [PIPE_B] = IVB_CURSOR_B_OFFSET, \
> +               [PIPE_C] = IVB_CURSOR_C_OFFSET, \
> +       }

Ok, those tables all match up with my understanding and rummaging.

>  static const struct intel_device_info intel_i865g_info = {
> -       GEN2_FEATURES,
> +       I845_FEATURES,

Hmm, 865g had dvo in addition to vga (unlike 845g) so did it have a
second pipe? Though memory says dual pipe was a luxury of mobile only.

>         PLATFORM(INTEL_I865G),
>  };
>  #define G75_FEATURES  \
> @@ -404,6 +465,7 @@ static const struct intel_device_info intel_valleyview_info = {

Grr.

>         .display.has_psr = 1, \
>         .display.has_dp_mst = 1, \
>         .has_rc6p = 0 /* RC6p removed-by HSW */, \
> +       HSW_PIPE_OFFSETS, \

Was thinking vlv using HSW? but then realised that diff was misleading.

And the device tables look to be using the correct feature sets.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä March 6, 2019, 2:52 p.m. UTC | #2 
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@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?

> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_pci.c | 146 +++++++++++++++++++++++---------
> >  1 file changed, 104 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index c42c5ccf38fe..9e610e4bdd7d 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -35,7 +35,37 @@
> >  #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
> >  #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
> >  
> > -#define GEN_DEFAULT_PIPEOFFSETS \
> > +#define I845_PIPE_OFFSETS \
> > +       .pipe_offsets = { \
> > +               [TRANSCODER_A] = PIPE_A_OFFSET, \
> > +       }, \
> > +       .trans_offsets = { \
> > +               [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > +       }
> > +
> > +#define I9XX_PIPE_OFFSETS \
> > +       .pipe_offsets = { \
> > +               [TRANSCODER_A] = PIPE_A_OFFSET, \
> > +               [TRANSCODER_B] = PIPE_B_OFFSET, \
> > +       }, \
> > +       .trans_offsets = { \
> > +               [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > +               [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > +       }
> > +
> > +#define IVB_PIPE_OFFSETS \
> > +       .pipe_offsets = { \
> > +               [TRANSCODER_A] = PIPE_A_OFFSET, \
> > +               [TRANSCODER_B] = PIPE_B_OFFSET, \
> > +               [TRANSCODER_C] = PIPE_C_OFFSET, \
> > +       }, \
> > +       .trans_offsets = { \
> > +               [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > +               [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > +               [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> > +       }
> > +
> > +#define HSW_PIPE_OFFSETS \
> >         .pipe_offsets = { \
> >                 [TRANSCODER_A] = PIPE_A_OFFSET, \
> >                 [TRANSCODER_B] = PIPE_B_OFFSET, \
> > @@ -49,7 +79,7 @@
> >                 [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> >         }
> >  
> > -#define GEN_CHV_PIPEOFFSETS \
> > +#define CHV_PIPE_OFFSETS \
> >         .pipe_offsets = { \
> >                 [TRANSCODER_A] = PIPE_A_OFFSET, \
> >                 [TRANSCODER_B] = PIPE_B_OFFSET, \
> > @@ -61,11 +91,30 @@
> >                 [TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
> >         }
> >  
> > -#define CURSOR_OFFSETS \
> > -       .cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
> > +#define I845_CURSOR_OFFSETS \
> > +       .cursor_offsets = { \
> > +               [PIPE_A] = CURSOR_A_OFFSET, \
> > +       }
> > +
> > +#define I9XX_CURSOR_OFFSETS \
> > +       .cursor_offsets = { \
> > +               [PIPE_A] = CURSOR_A_OFFSET, \
> > +               [PIPE_B] = CURSOR_B_OFFSET, \
> > +       }
> > +
> > +#define CHV_CURSOR_OFFSETS \
> > +       .cursor_offsets = { \
> > +               [PIPE_A] = CURSOR_A_OFFSET, \
> > +               [PIPE_B] = CURSOR_B_OFFSET, \
> > +               [PIPE_C] = CHV_CURSOR_C_OFFSET, \
> > +       }
> >  
> >  #define IVB_CURSOR_OFFSETS \
> > -       .cursor_offsets = { CURSOR_A_OFFSET, IVB_CURSOR_B_OFFSET, IVB_CURSOR_C_OFFSET }
> > +       .cursor_offsets = { \
> > +               [PIPE_A] = CURSOR_A_OFFSET, \
> > +               [PIPE_B] = IVB_CURSOR_B_OFFSET, \
> > +               [PIPE_C] = IVB_CURSOR_C_OFFSET, \
> > +       }
> 
> Ok, those tables all match up with my understanding and rummaging.
> 
> >  static const struct intel_device_info intel_i865g_info = {
> > -       GEN2_FEATURES,
> > +       I845_FEATURES,
> 
> Hmm, 865g had dvo in addition to vga (unlike 845g)

845g doesn't have dvo? The docs don't seem to quite agree with that.
I should probably hunt one down just to complete my gen2 collection :)

> so did it have a
> second pipe? Though memory says dual pipe was a luxury of mobile only.

Yeah, single pipe only I'm afraid.

> 
> >         PLATFORM(INTEL_I865G),
> >  };
> >  #define G75_FEATURES  \
> > @@ -404,6 +465,7 @@ static const struct intel_device_info intel_valleyview_info = {
> 
> Grr.
> 
> >         .display.has_psr = 1, \
> >         .display.has_dp_mst = 1, \
> >         .has_rc6p = 0 /* RC6p removed-by HSW */, \
> > +       HSW_PIPE_OFFSETS, \
> 
> Was thinking vlv using HSW? but then realised that diff was misleading.

I rather dislike these template macros for this exact reason.

> 
> And the device tables look to be using the correct feature sets.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks.
Chris Wilson March 6, 2019, 2:55 p.m. UTC | #3 
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@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?
-Chris
Ville Syrjälä March 6, 2019, 3:13 p.m. UTC | #4 
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@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...
Lucas De Marchi March 6, 2019, 5:42 p.m. UTC | #5 
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@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? :-/

Lucas De Marchi
>
>-- 
>Ville Syrjälä
>Intel
Ville Syrjälä March 6, 2019, 5:55 p.m. UTC | #6 
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@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.
So using that to determine if a transcoder is present is not going
to work.
Ville Syrjälä March 6, 2019, 5:58 p.m. UTC | #7 
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@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.
> So using that to determine if a transcoder is present is not going
> to work.

Make that "every platform excep chv".
Ville Syrjälä March 6, 2019, 6:01 p.m. UTC | #8 
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@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.
> So using that to determine if a transcoder is present is not going
> to work.

So far the HAS_EDP_TRANSCODER() thing is not actually a problem as
it's only called from the ddi code which guarantees that the transcoder
is in fact present. But the error capture code is now very much wrong.
Lucas De Marchi March 6, 2019, 6:55 p.m. UTC | #9 
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@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.

checking now that that probably broke chv though.

Lucas De Marchi

>So using that to determine if a transcoder is present is not going
>to work.
>
>-- 
>Ville Syrjälä
>Intel
Lucas De Marchi March 6, 2019, 6:58 p.m. UTC | #10 
On Wed, Mar 06, 2019 at 08:01:05PM +0200, Ville Syrjälä 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@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.
>> So using that to determine if a transcoder is present is not going
>> to work.
>
>So far the HAS_EDP_TRANSCODER() thing is not actually a problem as
>it's only called from the ddi code which guarantees that the transcoder
>is in fact present. But the error capture code is now very much wrong.

Commit 062de72bc0c73f4b9fa8532e5a98bf609ebd08da was done in preparation
for HAS_EDP_TRANSCODER to account for that.

Lucas De Marchi

>
>-- 
>Ville Syrjälä
>Intel
Ville Syrjälä March 6, 2019, 7 p.m. UTC | #11 
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@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.
Ville Syrjälä March 6, 2019, 8:25 p.m. UTC | #12 
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@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.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index c42c5ccf38fe..9e610e4bdd7d 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -35,7 +35,37 @@ 
 #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
 #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
 
-#define GEN_DEFAULT_PIPEOFFSETS \
+#define I845_PIPE_OFFSETS \
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET,	\
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+	}
+
+#define I9XX_PIPE_OFFSETS \
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET,	\
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+	}
+
+#define IVB_PIPE_OFFSETS \
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET,	\
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = PIPE_C_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+	}
+
+#define HSW_PIPE_OFFSETS \
 	.pipe_offsets = { \
 		[TRANSCODER_A] = PIPE_A_OFFSET,	\
 		[TRANSCODER_B] = PIPE_B_OFFSET, \
@@ -49,7 +79,7 @@ 
 		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
 	}
 
-#define GEN_CHV_PIPEOFFSETS \
+#define CHV_PIPE_OFFSETS \
 	.pipe_offsets = { \
 		[TRANSCODER_A] = PIPE_A_OFFSET, \
 		[TRANSCODER_B] = PIPE_B_OFFSET, \
@@ -61,11 +91,30 @@ 
 		[TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
 	}
 
-#define CURSOR_OFFSETS \
-	.cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
+#define I845_CURSOR_OFFSETS \
+	.cursor_offsets = { \
+		[PIPE_A] = CURSOR_A_OFFSET, \
+	}
+
+#define I9XX_CURSOR_OFFSETS \
+	.cursor_offsets = { \
+		[PIPE_A] = CURSOR_A_OFFSET, \
+		[PIPE_B] = CURSOR_B_OFFSET, \
+	}
+
+#define CHV_CURSOR_OFFSETS \
+	.cursor_offsets = { \
+		[PIPE_A] = CURSOR_A_OFFSET, \
+		[PIPE_B] = CURSOR_B_OFFSET, \
+		[PIPE_C] = CHV_CURSOR_C_OFFSET, \
+	}
 
 #define IVB_CURSOR_OFFSETS \
-	.cursor_offsets = { CURSOR_A_OFFSET, IVB_CURSOR_B_OFFSET, IVB_CURSOR_C_OFFSET }
+	.cursor_offsets = { \
+		[PIPE_A] = CURSOR_A_OFFSET, \
+		[PIPE_B] = IVB_CURSOR_B_OFFSET, \
+		[PIPE_C] = IVB_CURSOR_C_OFFSET, \
+	}
 
 #define BDW_COLORS \
 	.color = { .degamma_lut_size = 512, .gamma_lut_size = 512 }
@@ -85,7 +134,25 @@ 
 #define GEN_DEFAULT_PAGE_SIZES \
 	.page_sizes = I915_GTT_PAGE_SIZE_4K
 
-#define GEN2_FEATURES \
+#define I830_FEATURES \
+	GEN(2), \
+	.is_mobile = 1, \
+	.num_pipes = 2, \
+	.display.has_overlay = 1, \
+	.display.cursor_needs_physical = 1, \
+	.display.overlay_needs_physical = 1, \
+	.display.has_gmch = 1, \
+	.gpu_reset_clobbers_display = true, \
+	.hws_needs_physical = 1, \
+	.unfenced_needs_alignment = 1, \
+	.engine_mask = BIT(RCS0), \
+	.has_snoop = true, \
+	.has_coherent_ggtt = false, \
+	I9XX_PIPE_OFFSETS, \
+	I9XX_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
+
+#define I845_FEATURES \
 	GEN(2), \
 	.num_pipes = 1, \
 	.display.has_overlay = 1, \
@@ -97,34 +164,28 @@ 
 	.engine_mask = BIT(RCS0), \
 	.has_snoop = true, \
 	.has_coherent_ggtt = false, \
-	GEN_DEFAULT_PIPEOFFSETS, \
-	GEN_DEFAULT_PAGE_SIZES, \
-	CURSOR_OFFSETS
+	I845_PIPE_OFFSETS, \
+	I845_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
 
 static const struct intel_device_info intel_i830_info = {
-	GEN2_FEATURES,
+	I830_FEATURES,
 	PLATFORM(INTEL_I830),
-	.is_mobile = 1,
-	.display.cursor_needs_physical = 1,
-	.num_pipes = 2, /* legal, last one wins */
 };
 
 static const struct intel_device_info intel_i845g_info = {
-	GEN2_FEATURES,
+	I845_FEATURES,
 	PLATFORM(INTEL_I845G),
 };
 
 static const struct intel_device_info intel_i85x_info = {
-	GEN2_FEATURES,
+	I830_FEATURES,
 	PLATFORM(INTEL_I85X),
-	.is_mobile = 1,
-	.num_pipes = 2, /* legal, last one wins */
-	.display.cursor_needs_physical = 1,
 	.display.has_fbc = 1,
 };
 
 static const struct intel_device_info intel_i865g_info = {
-	GEN2_FEATURES,
+	I845_FEATURES,
 	PLATFORM(INTEL_I865G),
 };
 
@@ -136,9 +197,9 @@  static const struct intel_device_info intel_i865g_info = {
 	.engine_mask = BIT(RCS0), \
 	.has_snoop = true, \
 	.has_coherent_ggtt = true, \
-	GEN_DEFAULT_PIPEOFFSETS, \
-	GEN_DEFAULT_PAGE_SIZES, \
-	CURSOR_OFFSETS
+	I9XX_PIPE_OFFSETS, \
+	I9XX_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
 
 static const struct intel_device_info intel_i915g_info = {
 	GEN3_FEATURES,
@@ -213,9 +274,9 @@  static const struct intel_device_info intel_pineview_info = {
 	.engine_mask = BIT(RCS0), \
 	.has_snoop = true, \
 	.has_coherent_ggtt = true, \
-	GEN_DEFAULT_PIPEOFFSETS, \
-	GEN_DEFAULT_PAGE_SIZES, \
-	CURSOR_OFFSETS
+	I9XX_PIPE_OFFSETS, \
+	I9XX_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
 
 static const struct intel_device_info intel_i965g_info = {
 	GEN4_FEATURES,
@@ -262,9 +323,9 @@  static const struct intel_device_info intel_gm45_info = {
 	.has_coherent_ggtt = true, \
 	/* ilk does support rc6, but we do not implement [power] contexts */ \
 	.has_rc6 = 0, \
-	GEN_DEFAULT_PIPEOFFSETS, \
-	GEN_DEFAULT_PAGE_SIZES, \
-	CURSOR_OFFSETS
+	I9XX_PIPE_OFFSETS, \
+	I9XX_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
 
 static const struct intel_device_info intel_ironlake_d_info = {
 	GEN5_FEATURES,
@@ -289,9 +350,9 @@  static const struct intel_device_info intel_ironlake_m_info = {
 	.has_rc6 = 1, \
 	.has_rc6p = 1, \
 	.ppgtt = INTEL_PPGTT_ALIASING, \
-	GEN_DEFAULT_PIPEOFFSETS, \
-	GEN_DEFAULT_PAGE_SIZES, \
-	CURSOR_OFFSETS
+	I9XX_PIPE_OFFSETS, \
+	I9XX_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
 
 #define SNB_D_PLATFORM \
 	GEN6_FEATURES, \
@@ -334,9 +395,9 @@  static const struct intel_device_info intel_sandybridge_m_gt2_info = {
 	.has_rc6 = 1, \
 	.has_rc6p = 1, \
 	.ppgtt = INTEL_PPGTT_FULL, \
-	GEN_DEFAULT_PIPEOFFSETS, \
-	GEN_DEFAULT_PAGE_SIZES, \
-	IVB_CURSOR_OFFSETS
+	IVB_PIPE_OFFSETS, \
+	IVB_CURSOR_OFFSETS, \
+	GEN_DEFAULT_PAGE_SIZES
 
 #define IVB_D_PLATFORM \
 	GEN7_FEATURES, \
@@ -391,9 +452,9 @@  static const struct intel_device_info intel_valleyview_info = {
 	.has_coherent_ggtt = false,
 	.engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0),
 	.display_mmio_offset = VLV_DISPLAY_BASE,
+	I9XX_PIPE_OFFSETS,
+	I9XX_CURSOR_OFFSETS,
 	GEN_DEFAULT_PAGE_SIZES,
-	GEN_DEFAULT_PIPEOFFSETS,
-	CURSOR_OFFSETS
 };
 
 #define G75_FEATURES  \
@@ -404,6 +465,7 @@  static const struct intel_device_info intel_valleyview_info = {
 	.display.has_psr = 1, \
 	.display.has_dp_mst = 1, \
 	.has_rc6p = 0 /* RC6p removed-by HSW */, \
+	HSW_PIPE_OFFSETS, \
 	.has_runtime_pm = 1
 
 #define HSW_PLATFORM \
@@ -483,10 +545,10 @@  static const struct intel_device_info intel_cherryview_info = {
 	.has_snoop = true,
 	.has_coherent_ggtt = false,
 	.display_mmio_offset = VLV_DISPLAY_BASE,
-	GEN_DEFAULT_PAGE_SIZES,
-	GEN_CHV_PIPEOFFSETS,
-	CURSOR_OFFSETS,
+	CHV_PIPE_OFFSETS,
+	CHV_CURSOR_OFFSETS,
 	CHV_COLORS,
+	GEN_DEFAULT_PAGE_SIZES,
 };
 
 #define GEN9_DEFAULT_PAGE_SIZES \
@@ -559,10 +621,10 @@  static const struct intel_device_info intel_skylake_gt4_info = {
 	.has_snoop = true, \
 	.has_coherent_ggtt = false, \
 	.display.has_ipc = 1, \
-	GEN9_DEFAULT_PAGE_SIZES, \
-	GEN_DEFAULT_PIPEOFFSETS, \
+	HSW_PIPE_OFFSETS, \
 	IVB_CURSOR_OFFSETS, \
-	BDW_COLORS
+	BDW_COLORS, \
+	GEN9_DEFAULT_PAGE_SIZES
 
 static const struct intel_device_info intel_broxton_info = {
 	GEN9_LP_FEATURES,