Re: [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder

[Imre Deak <imre.deak@intel.com>]

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@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  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.
 */

> 
> >  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,

/*
 * 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx