| Message ID | 20200625001120.22810-5-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | display/ddi: keep register indexes in a table | expand |
On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Identify 3 possible cases in which the index numbers can be different > from the "port" and add them to the description-based ddi initialization > table. This can be used in place of additional functions mapping from > one to the other. Right now we already cover part of this by creating kind of > virtual phy numbering, but that comes with downsides: > > a) there's not really a "phy numbering" in the spec, this is purely a > software thing; hardware uses whatever they want thinking mapping from > one to the other arbitrarily is easy in software. > > b) currently the mapping occurs on "leaf" functions, making the decision > based on the platform for each of those functions > > With this new table the approach will be: the port, as defined by the > enum port, is merely a driver convention and won't be used anymore to > define the register offset or register bits. For that we have the other > 3 indexes, identified as being possibly different from the current usage > of register bits: ddi, vbt and phy. The phy type is also added here, > meant to replace the checks for combo vs tc. > > v2: Rebase and add RKL > I guess I'd like to see where the *_idx fields will lead before advocating for this. With them removed, Acked-by: Jani Nikula <jani.nikula@intel.com> But I'm also not saying you can't have them - until I see where this leads. ;) One comment inline below. > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++--------- > drivers/gpu/drm/i915/display/intel_display.h | 8 +++ > .../drm/i915/display/intel_display_types.h | 4 ++ > 3 files changed, 45 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index c234b50212b0..d591063502c5 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) > } > > static const struct intel_ddi_port_info rkl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI TC1", .port = PORT_D }, > - { .name = "DDI TC2", .port = PORT_E }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, > + /* TODO: use continguous namespace for port once driver is converted */ > + { .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, }, > + { .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info tgl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI TC1", .port = PORT_D }, > - { .name = "DDI TC2", .port = PORT_E }, > - { .name = "DDI TC3", .port = PORT_F }, > - { .name = "DDI TC4", .port = PORT_G }, > - { .name = "DDI TC5", .port = PORT_H }, > - { .name = "DDI TC6", .port = PORT_I }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, > + /* TODO: use continguous namespace for port once driver is converted */ > + { .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, }, > + { .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, }, > + { .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, }, > + { .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, }, > + { .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, }, > + { .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info ehl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI C", .port = PORT_C }, > - { .name = "DDI D", .port = PORT_D }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, > + { .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, }, > + { .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info icl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI TC1", .port = PORT_C }, > - { .name = "DDI TC2", .port = PORT_D }, > - { .name = "DDI TC3", .port = PORT_E }, > - { .name = "DDI TC4", .port = PORT_F }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,}, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,}, > + { .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,}, > + { .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,}, > + { .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,}, > + { .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,}, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info gen9lp_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI C", .port = PORT_C }, > + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, > + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, > + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info ddi_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI C", .port = PORT_C }, > - { .name = "DDI D", .port = PORT_D }, > - { .name = "DDI E", .port = PORT_E }, > - { .name = "DDI F", .port = PORT_F }, > + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, > + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, > + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, > + { .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 }, > + { .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 }, > + { .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 }, > { .port = PORT_NONE } > }; > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > index b7a6d56bac5f..22c999a54ff1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.h > +++ b/drivers/gpu/drm/i915/display/intel_display.h > @@ -311,6 +311,14 @@ enum phy { > I915_MAX_PHYS > }; > > +enum phy_type { > + PHY_TYPE_NONE = 0, > + > + PHY_TYPE_COMBO, > + PHY_TYPE_MG, > + PHY_TYPE_DKL, > +}; > + > #define phy_name(a) ((a) + 'A') > > enum phy_fia { > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 92cc7fc66bce..df587219c744 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder { > struct intel_ddi_port_info { > const char *name; > enum port port; > + s8 phy_type; Please make the type enum phy_type. > + u8 ddi_idx; > + u8 phy_idx; > + u8 vbt_idx; > }; > > static inline enum dpio_channel
On Wed, 01 Jul 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> Identify 3 possible cases in which the index numbers can be different >> from the "port" and add them to the description-based ddi initialization >> table. This can be used in place of additional functions mapping from >> one to the other. Right now we already cover part of this by creating kind of >> virtual phy numbering, but that comes with downsides: >> >> a) there's not really a "phy numbering" in the spec, this is purely a >> software thing; hardware uses whatever they want thinking mapping from >> one to the other arbitrarily is easy in software. >> >> b) currently the mapping occurs on "leaf" functions, making the decision >> based on the platform for each of those functions >> >> With this new table the approach will be: the port, as defined by the >> enum port, is merely a driver convention and won't be used anymore to >> define the register offset or register bits. For that we have the other >> 3 indexes, identified as being possibly different from the current usage >> of register bits: ddi, vbt and phy. The phy type is also added here, >> meant to replace the checks for combo vs tc. >> >> v2: Rebase and add RKL >> > > I guess I'd like to see where the *_idx fields will lead before > advocating for this. > > With them removed, Ahem, ddi_idx and vbt_idx - obviously phy_idx is used, and I approve of the use. Another comment inline below. > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > But I'm also not saying you can't have them - until I see where this > leads. ;) > > One comment inline below. > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++--------- >> drivers/gpu/drm/i915/display/intel_display.h | 8 +++ >> .../drm/i915/display/intel_display_types.h | 4 ++ >> 3 files changed, 45 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index c234b50212b0..d591063502c5 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) >> } >> >> static const struct intel_ddi_port_info rkl_ports[] = { >> - { .name = "DDI A", .port = PORT_A }, >> - { .name = "DDI B", .port = PORT_B }, >> - { .name = "DDI TC1", .port = PORT_D }, >> - { .name = "DDI TC2", .port = PORT_E }, >> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, >> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, >> + /* TODO: use continguous namespace for port once driver is converted */ >> + { .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, }, >> + { .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, }, >> { .port = PORT_NONE } >> }; >> >> static const struct intel_ddi_port_info tgl_ports[] = { >> - { .name = "DDI A", .port = PORT_A }, >> - { .name = "DDI B", .port = PORT_B }, >> - { .name = "DDI TC1", .port = PORT_D }, >> - { .name = "DDI TC2", .port = PORT_E }, >> - { .name = "DDI TC3", .port = PORT_F }, >> - { .name = "DDI TC4", .port = PORT_G }, >> - { .name = "DDI TC5", .port = PORT_H }, >> - { .name = "DDI TC6", .port = PORT_I }, >> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, >> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, >> + /* TODO: use continguous namespace for port once driver is converted */ >> + { .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, }, >> + { .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, }, >> + { .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, }, >> + { .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, }, >> + { .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, }, >> + { .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, }, >> { .port = PORT_NONE } >> }; >> >> static const struct intel_ddi_port_info ehl_ports[] = { >> - { .name = "DDI A", .port = PORT_A }, >> - { .name = "DDI B", .port = PORT_B }, >> - { .name = "DDI C", .port = PORT_C }, >> - { .name = "DDI D", .port = PORT_D }, >> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, >> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, >> + { .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, }, >> + { .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, }, >> { .port = PORT_NONE } >> }; >> >> static const struct intel_ddi_port_info icl_ports[] = { >> - { .name = "DDI A", .port = PORT_A }, >> - { .name = "DDI B", .port = PORT_B }, >> - { .name = "DDI TC1", .port = PORT_C }, >> - { .name = "DDI TC2", .port = PORT_D }, >> - { .name = "DDI TC3", .port = PORT_E }, >> - { .name = "DDI TC4", .port = PORT_F }, >> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,}, >> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,}, >> + { .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,}, >> + { .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,}, >> + { .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,}, >> + { .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,}, >> { .port = PORT_NONE } >> }; >> >> static const struct intel_ddi_port_info gen9lp_ports[] = { >> - { .name = "DDI A", .port = PORT_A }, >> - { .name = "DDI B", .port = PORT_B }, >> - { .name = "DDI C", .port = PORT_C }, >> + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, >> + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, >> + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, >> { .port = PORT_NONE } >> }; >> >> static const struct intel_ddi_port_info ddi_ports[] = { >> - { .name = "DDI A", .port = PORT_A }, >> - { .name = "DDI B", .port = PORT_B }, >> - { .name = "DDI C", .port = PORT_C }, >> - { .name = "DDI D", .port = PORT_D }, >> - { .name = "DDI E", .port = PORT_E }, >> - { .name = "DDI F", .port = PORT_F }, >> + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, >> + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, >> + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, >> + { .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 }, >> + { .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 }, >> + { .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 }, >> { .port = PORT_NONE } >> }; >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h >> index b7a6d56bac5f..22c999a54ff1 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.h >> +++ b/drivers/gpu/drm/i915/display/intel_display.h >> @@ -311,6 +311,14 @@ enum phy { >> I915_MAX_PHYS >> }; >> >> +enum phy_type { >> + PHY_TYPE_NONE = 0, >> + >> + PHY_TYPE_COMBO, >> + PHY_TYPE_MG, >> + PHY_TYPE_DKL, >> +}; >> + >> #define phy_name(a) ((a) + 'A') >> >> enum phy_fia { >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >> index 92cc7fc66bce..df587219c744 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder { >> struct intel_ddi_port_info { >> const char *name; >> enum port port; >> + s8 phy_type; > > Please make the type enum phy_type. > >> + u8 ddi_idx; >> + u8 phy_idx; I think we should retain enum phy as type for this too. I generally think this gives people a better grasp that you shouldn't convert it to some other generic integer nilly-willy. Also, if we need to change this later on, tooling (cocci, tagging tools, etc.) are more helpful with enums. BR, Jani. >> + u8 vbt_idx; >> }; >> >> static inline enum dpio_channel
On Wed, Jul 01, 2020 at 06:23:05PM +0300, Jani Nikula wrote: >On Wed, 01 Jul 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> Identify 3 possible cases in which the index numbers can be different >>> from the "port" and add them to the description-based ddi initialization >>> table. This can be used in place of additional functions mapping from >>> one to the other. Right now we already cover part of this by creating kind of >>> virtual phy numbering, but that comes with downsides: >>> >>> a) there's not really a "phy numbering" in the spec, this is purely a >>> software thing; hardware uses whatever they want thinking mapping from >>> one to the other arbitrarily is easy in software. >>> >>> b) currently the mapping occurs on "leaf" functions, making the decision >>> based on the platform for each of those functions >>> >>> With this new table the approach will be: the port, as defined by the >>> enum port, is merely a driver convention and won't be used anymore to >>> define the register offset or register bits. For that we have the other >>> 3 indexes, identified as being possibly different from the current usage >>> of register bits: ddi, vbt and phy. The phy type is also added here, >>> meant to replace the checks for combo vs tc. >>> >>> v2: Rebase and add RKL >>> >> >> I guess I'd like to see where the *_idx fields will lead before >> advocating for this. >> >> With them removed, > >Ahem, ddi_idx and vbt_idx - obviously phy_idx is used, and I approve of >the use. > >Another comment inline below. > >> >> Acked-by: Jani Nikula <jani.nikula@intel.com> >> >> But I'm also not saying you can't have them - until I see where this >> leads. ;) >> >> One comment inline below. >> >>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++--------- >>> drivers/gpu/drm/i915/display/intel_display.h | 8 +++ >>> .../drm/i915/display/intel_display_types.h | 4 ++ >>> 3 files changed, 45 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >>> index c234b50212b0..d591063502c5 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display.c >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c >>> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) >>> } >>> >>> static const struct intel_ddi_port_info rkl_ports[] = { >>> - { .name = "DDI A", .port = PORT_A }, >>> - { .name = "DDI B", .port = PORT_B }, >>> - { .name = "DDI TC1", .port = PORT_D }, >>> - { .name = "DDI TC2", .port = PORT_E }, >>> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, >>> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, >>> + /* TODO: use continguous namespace for port once driver is converted */ >>> + { .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, }, >>> + { .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, }, >>> { .port = PORT_NONE } >>> }; >>> >>> static const struct intel_ddi_port_info tgl_ports[] = { >>> - { .name = "DDI A", .port = PORT_A }, >>> - { .name = "DDI B", .port = PORT_B }, >>> - { .name = "DDI TC1", .port = PORT_D }, >>> - { .name = "DDI TC2", .port = PORT_E }, >>> - { .name = "DDI TC3", .port = PORT_F }, >>> - { .name = "DDI TC4", .port = PORT_G }, >>> - { .name = "DDI TC5", .port = PORT_H }, >>> - { .name = "DDI TC6", .port = PORT_I }, >>> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, >>> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, >>> + /* TODO: use continguous namespace for port once driver is converted */ >>> + { .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, }, >>> + { .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, }, >>> + { .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, }, >>> + { .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, }, >>> + { .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, }, >>> + { .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, }, >>> { .port = PORT_NONE } >>> }; >>> >>> static const struct intel_ddi_port_info ehl_ports[] = { >>> - { .name = "DDI A", .port = PORT_A }, >>> - { .name = "DDI B", .port = PORT_B }, >>> - { .name = "DDI C", .port = PORT_C }, >>> - { .name = "DDI D", .port = PORT_D }, >>> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, >>> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, >>> + { .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, }, >>> + { .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, }, >>> { .port = PORT_NONE } >>> }; >>> >>> static const struct intel_ddi_port_info icl_ports[] = { >>> - { .name = "DDI A", .port = PORT_A }, >>> - { .name = "DDI B", .port = PORT_B }, >>> - { .name = "DDI TC1", .port = PORT_C }, >>> - { .name = "DDI TC2", .port = PORT_D }, >>> - { .name = "DDI TC3", .port = PORT_E }, >>> - { .name = "DDI TC4", .port = PORT_F }, >>> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,}, >>> + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,}, >>> + { .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,}, >>> + { .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,}, >>> + { .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,}, >>> + { .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,}, >>> { .port = PORT_NONE } >>> }; >>> >>> static const struct intel_ddi_port_info gen9lp_ports[] = { >>> - { .name = "DDI A", .port = PORT_A }, >>> - { .name = "DDI B", .port = PORT_B }, >>> - { .name = "DDI C", .port = PORT_C }, >>> + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, >>> + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, >>> + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, >>> { .port = PORT_NONE } >>> }; >>> >>> static const struct intel_ddi_port_info ddi_ports[] = { >>> - { .name = "DDI A", .port = PORT_A }, >>> - { .name = "DDI B", .port = PORT_B }, >>> - { .name = "DDI C", .port = PORT_C }, >>> - { .name = "DDI D", .port = PORT_D }, >>> - { .name = "DDI E", .port = PORT_E }, >>> - { .name = "DDI F", .port = PORT_F }, >>> + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, >>> + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, >>> + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, >>> + { .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 }, >>> + { .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 }, >>> + { .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 }, >>> { .port = PORT_NONE } >>> }; >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h >>> index b7a6d56bac5f..22c999a54ff1 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display.h >>> +++ b/drivers/gpu/drm/i915/display/intel_display.h >>> @@ -311,6 +311,14 @@ enum phy { >>> I915_MAX_PHYS >>> }; >>> >>> +enum phy_type { >>> + PHY_TYPE_NONE = 0, >>> + >>> + PHY_TYPE_COMBO, >>> + PHY_TYPE_MG, >>> + PHY_TYPE_DKL, >>> +}; >>> + >>> #define phy_name(a) ((a) + 'A') >>> >>> enum phy_fia { >>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >>> index 92cc7fc66bce..df587219c744 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >>> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder { >>> struct intel_ddi_port_info { >>> const char *name; >>> enum port port; >>> + s8 phy_type; >> >> Please make the type enum phy_type. >> >>> + u8 ddi_idx; >>> + u8 phy_idx; > >I think we should retain enum phy as type for this too. I generally >think this gives people a better grasp that you shouldn't convert it to >some other generic integer nilly-willy. Also, if we need to change this >later on, tooling (cocci, tagging tools, etc.) are more helpful with >enums. for phy_type I agree, since we are supposed to really use they PHY_TYPE_* values. However I don't think we should really spell out the PHY_* in the code except in very few cases where checking for e.g. PHY_A in a "leaf function" is more convenient than restructuring the code. Also, if I do this here I'd increase the size of the struct in at least 3 bytes (considering I move the field up to remove the hole in the struct). This is used for one table per platform so I think we can't just ignore it. Lucas De Marchi > >BR, >Jani. > >>> + u8 vbt_idx; >>> }; >>> >>> static inline enum dpio_channel > >-- >Jani Nikula, Intel Open Source Graphics Center
On Wed, Jun 24, 2020 at 05:11:18PM -0700, Lucas De Marchi wrote: > Identify 3 possible cases in which the index numbers can be different > from the "port" and add them to the description-based ddi initialization > table. This can be used in place of additional functions mapping from > one to the other. Right now we already cover part of this by creating kind of > virtual phy numbering, but that comes with downsides: > > a) there's not really a "phy numbering" in the spec, this is purely a > software thing; hardware uses whatever they want thinking mapping from > one to the other arbitrarily is easy in software. > > b) currently the mapping occurs on "leaf" functions, making the decision > based on the platform for each of those functions > > With this new table the approach will be: the port, as defined by the > enum port, is merely a driver convention and won't be used anymore to > define the register offset or register bits. For that we have the other > 3 indexes, identified as being possibly different from the current usage > of register bits: ddi, vbt and phy. The phy type is also added here, > meant to replace the checks for combo vs tc. > > v2: Rebase and add RKL > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++--------- > drivers/gpu/drm/i915/display/intel_display.h | 8 +++ > .../drm/i915/display/intel_display_types.h | 4 ++ > 3 files changed, 45 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index c234b50212b0..d591063502c5 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) > } > > static const struct intel_ddi_port_info rkl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI TC1", .port = PORT_D }, > - { .name = "DDI TC2", .port = PORT_E }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, I'm thinking we won't need ddi_idx and instead 'port' should suffice. We can add the aliases with the TC names for tgl+ to unconfuse the current mess. In fact I already tried that in a local branch (while doing the hpd_pin cleanup) and it looks mostly fine to me. There are a few annoying parts, like power domains, where we still end up with port G-I names that don't exist anywhere in bspec (excetp in VBT). Not really sure about the other bare numbers you're using here either. Migth just make things less confusing if we don't have names for many things. So I think enums would be better. And I think this stuff should probably go into intel_ddi.c instead of intel_display.c. > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, > + /* TODO: use continguous namespace for port once driver is converted */ > + { .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, }, > + { .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info tgl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI TC1", .port = PORT_D }, > - { .name = "DDI TC2", .port = PORT_E }, > - { .name = "DDI TC3", .port = PORT_F }, > - { .name = "DDI TC4", .port = PORT_G }, > - { .name = "DDI TC5", .port = PORT_H }, > - { .name = "DDI TC6", .port = PORT_I }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, > + /* TODO: use continguous namespace for port once driver is converted */ > + { .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, }, > + { .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, }, > + { .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, }, > + { .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, }, > + { .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, }, > + { .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info ehl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI C", .port = PORT_C }, > - { .name = "DDI D", .port = PORT_D }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, > + { .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, }, > + { .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info icl_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI TC1", .port = PORT_C }, > - { .name = "DDI TC2", .port = PORT_D }, > - { .name = "DDI TC3", .port = PORT_E }, > - { .name = "DDI TC4", .port = PORT_F }, > + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,}, > + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,}, > + { .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,}, > + { .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,}, > + { .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,}, > + { .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,}, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info gen9lp_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI C", .port = PORT_C }, > + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, > + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, > + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, > { .port = PORT_NONE } > }; > > static const struct intel_ddi_port_info ddi_ports[] = { > - { .name = "DDI A", .port = PORT_A }, > - { .name = "DDI B", .port = PORT_B }, > - { .name = "DDI C", .port = PORT_C }, > - { .name = "DDI D", .port = PORT_D }, > - { .name = "DDI E", .port = PORT_E }, > - { .name = "DDI F", .port = PORT_F }, > + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, > + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, > + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, > + { .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 }, > + { .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 }, > + { .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 }, > { .port = PORT_NONE } > }; > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > index b7a6d56bac5f..22c999a54ff1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.h > +++ b/drivers/gpu/drm/i915/display/intel_display.h > @@ -311,6 +311,14 @@ enum phy { > I915_MAX_PHYS > }; > > +enum phy_type { > + PHY_TYPE_NONE = 0, > + > + PHY_TYPE_COMBO, > + PHY_TYPE_MG, > + PHY_TYPE_DKL, > +}; > + > #define phy_name(a) ((a) + 'A') > > enum phy_fia { > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 92cc7fc66bce..df587219c744 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder { > struct intel_ddi_port_info { > const char *name; > enum port port; > + s8 phy_type; > + u8 ddi_idx; > + u8 phy_idx; > + u8 vbt_idx; > }; > > static inline enum dpio_channel > -- > 2.26.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 01, 2020 at 10:24:07AM -0700, Lucas De Marchi wrote: > On Wed, Jul 01, 2020 at 08:04:30PM +0300, Ville Syrjälä wrote: > >On Wed, Jun 24, 2020 at 05:11:18PM -0700, Lucas De Marchi wrote: > >> Identify 3 possible cases in which the index numbers can be different > >> from the "port" and add them to the description-based ddi initialization > >> table. This can be used in place of additional functions mapping from > >> one to the other. Right now we already cover part of this by creating kind of > >> virtual phy numbering, but that comes with downsides: > >> > >> a) there's not really a "phy numbering" in the spec, this is purely a > >> software thing; hardware uses whatever they want thinking mapping from > >> one to the other arbitrarily is easy in software. > >> > >> b) currently the mapping occurs on "leaf" functions, making the decision > >> based on the platform for each of those functions > >> > >> With this new table the approach will be: the port, as defined by the > >> enum port, is merely a driver convention and won't be used anymore to > >> define the register offset or register bits. For that we have the other > >> 3 indexes, identified as being possibly different from the current usage > >> of register bits: ddi, vbt and phy. The phy type is also added here, > >> meant to replace the checks for combo vs tc. > >> > >> v2: Rebase and add RKL > >> > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++--------- > >> drivers/gpu/drm/i915/display/intel_display.h | 8 +++ > >> .../drm/i915/display/intel_display_types.h | 4 ++ > >> 3 files changed, 45 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >> index c234b50212b0..d591063502c5 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) > >> } > >> > >> static const struct intel_ddi_port_info rkl_ports[] = { > >> - { .name = "DDI A", .port = PORT_A }, > >> - { .name = "DDI B", .port = PORT_B }, > >> - { .name = "DDI TC1", .port = PORT_D }, > >> - { .name = "DDI TC2", .port = PORT_E }, > >> + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, > > > >I'm thinking we won't need ddi_idx and instead 'port' should suffice. > >We can add the aliases with the TC names for tgl+ to unconfuse the > >current mess. In fact I already tried that in a local branch (while > >doing the hpd_pin cleanup) and it looks mostly fine to me. There are > >a few annoying parts, like power domains, where we still end up with > >port G-I names that don't exist anywhere in bspec (excetp in VBT). > > I think we should stop trying that because it leads to the current mess > we put ourselves into. Hence my idea of "port should be just a software > thing, don't try to make it match the hardware". HW indexes (for register > address, bitfields and whatnot) are provided by the correspondent _idx. > Which index you use depends on what part of the hw you are talking to. > > See the TODO below of one case this would be true. Once the conversions > are finished we change them and then for every ddi+ platform, port is > just a number we can use to identify the entry in the table. Seems contrary to pretty much everything else in the driver so not great IMO.
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index c234b50212b0..d591063502c5 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) } static const struct intel_ddi_port_info rkl_ports[] = { - { .name = "DDI A", .port = PORT_A }, - { .name = "DDI B", .port = PORT_B }, - { .name = "DDI TC1", .port = PORT_D }, - { .name = "DDI TC2", .port = PORT_E }, + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, + /* TODO: use continguous namespace for port once driver is converted */ + { .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, }, + { .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, }, { .port = PORT_NONE } }; static const struct intel_ddi_port_info tgl_ports[] = { - { .name = "DDI A", .port = PORT_A }, - { .name = "DDI B", .port = PORT_B }, - { .name = "DDI TC1", .port = PORT_D }, - { .name = "DDI TC2", .port = PORT_E }, - { .name = "DDI TC3", .port = PORT_F }, - { .name = "DDI TC4", .port = PORT_G }, - { .name = "DDI TC5", .port = PORT_H }, - { .name = "DDI TC6", .port = PORT_I }, + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, + /* TODO: use continguous namespace for port once driver is converted */ + { .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, }, + { .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, }, + { .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, }, + { .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, }, + { .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, }, + { .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL, .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, }, { .port = PORT_NONE } }; static const struct intel_ddi_port_info ehl_ports[] = { - { .name = "DDI A", .port = PORT_A }, - { .name = "DDI B", .port = PORT_B }, - { .name = "DDI C", .port = PORT_C }, - { .name = "DDI D", .port = PORT_D }, + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, }, + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, }, + { .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, }, + { .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, }, { .port = PORT_NONE } }; static const struct intel_ddi_port_info icl_ports[] = { - { .name = "DDI A", .port = PORT_A }, - { .name = "DDI B", .port = PORT_B }, - { .name = "DDI TC1", .port = PORT_C }, - { .name = "DDI TC2", .port = PORT_D }, - { .name = "DDI TC3", .port = PORT_E }, - { .name = "DDI TC4", .port = PORT_F }, + { .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,}, + { .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,}, + { .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,}, + { .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,}, + { .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,}, + { .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG, .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,}, { .port = PORT_NONE } }; static const struct intel_ddi_port_info gen9lp_ports[] = { - { .name = "DDI A", .port = PORT_A }, - { .name = "DDI B", .port = PORT_B }, - { .name = "DDI C", .port = PORT_C }, + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, { .port = PORT_NONE } }; static const struct intel_ddi_port_info ddi_ports[] = { - { .name = "DDI A", .port = PORT_A }, - { .name = "DDI B", .port = PORT_B }, - { .name = "DDI C", .port = PORT_C }, - { .name = "DDI D", .port = PORT_D }, - { .name = "DDI E", .port = PORT_E }, - { .name = "DDI F", .port = PORT_F }, + { .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 }, + { .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 }, + { .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 }, + { .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 }, + { .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 }, + { .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 }, { .port = PORT_NONE } }; diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index b7a6d56bac5f..22c999a54ff1 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -311,6 +311,14 @@ enum phy { I915_MAX_PHYS }; +enum phy_type { + PHY_TYPE_NONE = 0, + + PHY_TYPE_COMBO, + PHY_TYPE_MG, + PHY_TYPE_DKL, +}; + #define phy_name(a) ((a) + 'A') enum phy_fia { diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 92cc7fc66bce..df587219c744 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder { struct intel_ddi_port_info { const char *name; enum port port; + s8 phy_type; + u8 ddi_idx; + u8 phy_idx; + u8 vbt_idx; }; static inline enum dpio_channel
Identify 3 possible cases in which the index numbers can be different from the "port" and add them to the description-based ddi initialization table. This can be used in place of additional functions mapping from one to the other. Right now we already cover part of this by creating kind of virtual phy numbering, but that comes with downsides: a) there's not really a "phy numbering" in the spec, this is purely a software thing; hardware uses whatever they want thinking mapping from one to the other arbitrarily is easy in software. b) currently the mapping occurs on "leaf" functions, making the decision based on the platform for each of those functions With this new table the approach will be: the port, as defined by the enum port, is merely a driver convention and won't be used anymore to define the register offset or register bits. For that we have the other 3 indexes, identified as being possibly different from the current usage of register bits: ddi, vbt and phy. The phy type is also added here, meant to replace the checks for combo vs tc. v2: Rebase and add RKL Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++--------- drivers/gpu/drm/i915/display/intel_display.h | 8 +++ .../drm/i915/display/intel_display_types.h | 4 ++ 3 files changed, 45 insertions(+), 31 deletions(-)