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

Submitted by Imre Deak on Nov. 19, 2018, 2:41 p.m.

Details

Message ID 20181119144109.12994-3-imre.deak@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 4 3 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak Nov. 19, 2018, 2:41 p.m.
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(+)

Patch hide | download patch | download mbox

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.
+ */
 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.
+ */
 enum transcoder {
 	TRANSCODER_A = 0,
 	TRANSCODER_B,

Comments

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.

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

>  enum transcoder {
>  	TRANSCODER_A = 0,
>  	TRANSCODER_B,
> -- 
> 2.13.2
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
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@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.
>  */

lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > >     TRANSCODER_EDP,
> > >     TRANSCODER_DSI_0,
> > >     TRANSCODER_DSI_1,
> 
> 
> > > -- 
> > > 2.13.2
> > 
> > -- 
> > Ville Syrjälä
> > Intel
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@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.

1:1 transcoder <-> pipe mapping, so it doesn't look like it's a pointer :)

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


Or at least a BUILD_BUG_ON(TRANSCODER_A != PIPE_A || TRANSCODER_B != PIPE_B || TRANSCODER_C != PIPE_C)

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