On Mon, Nov 19, 2018 at 02:06:31PM -0800, Lucas De Marchi wrote:
> On Mon, Nov 19, 2018 at 08:43:27PM +0200, Imre Deak wrote:
> > On Mon, Nov 19, 2018 at 05:51:31PM +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 19, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> > > > Add a comment to the pipe and transcoder enum definitions about our
> > > > assumption in the code that pipe==transcoder for PIPE_A-C /
> > > > TRANSCODER_A-C. This means we have to keep the values for these
> > > > pipe/transcoder enums fixed.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
> > > > 1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index 43eb4ebbcc35..cbb5d79d6a4c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > @@ -43,6 +43,10 @@ enum i915_gpio {
> > > > GPIOM,
> > > > };
> > > >
> > > > +/*
> > > > + * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
> > > > + * these pipes.
> > > > + */
> > >
> > > I suspect the A-C part is going to bitrot. Also I'm sure we
> > > make more assupmtions about these values (PIPE_A == 0,
> > > PIPE_N+1 == n+1, etc.). We should probably try to spell it
> > > all out here.
> >
> > Ok, can add instead:
> >
> > /*
> > * Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
> > * rest have consecutive values and match the enum values of transcoders
> > * with a 1:1 transcoder->pipe mapping.
>
> 1:1 transcoder <-> pipe mapping, so it doesn't look like it's a pointer :)
You can only map from transcoder to pipe not the other way around (a pipe
can always be connected to any transcoder). But will add spaces around
'->'.
>
> > */
> >
> > >
> > > > enum pipe {
> > > > INVALID_PIPE = -1,
> > > >
> > > > @@ -56,6 +60,10 @@ enum pipe {
> > > >
> > > > #define pipe_name(p) ((p) + 'A')
> > > >
> > > > +/*
> > > > + * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
> > > > + * these transcoders.
> > > > + */
> > >
> > > Same issue with the A-C part perhaps.
> >
> > and here:
> >
> > >
> > > > enum transcoder {
> >
> > /*
> > * The following transcoders have a 1:1 transcoder->pipe mapping, keep
> > * their values fixed: the code assumes that TRANSCODER_A=0, the rest
> > * have consecutive values and match the enum values of the pipes they map
> > * to.
> > */
> >
> >
> > > > TRANSCODER_A = 0,
> > > > TRANSCODER_B,
> > > > TRANSCODER_C,
>
> could we additionally do like below?
>
> TRANSCODER_A = PIPE_A,
> TRANSCODER_B = PIPE_B,
> TRANSCODER_C = PIPE_C,
Good idea, will change it.
> Or at least a BUILD_BUG_ON(TRANSCODER_A != PIPE_A || TRANSCODER_B != PIPE_B || TRANSCODER_C != PIPE_C)
This could get stale.
>
> Lucas De Marchi
>
> >
> > /*
> > * The following transcoders can map to any pipe, their enum value
> > * doesn't need to stay fixed.
> > */
> >
> > > > TRANSCODER_EDP,
> > > > TRANSCODER_DSI_0,
> > > > TRANSCODER_DSI_1,
> >
> >
> > > > --
> > > > 2.13.2
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
