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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx