| Message ID | 20181119144109.12994-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values | expand |
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
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,
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(+)