On Wed, Jul 01, 2020 at 05:20:17PM +0300, Jani Nikula wrote:
On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote:Start adding per-platform relevant data into a table that we use for initialization. Intention is to keep the different indexes we need (e.g. phy, vbt, ddi, etc) and any other differences for each platform in these tables so we don't have to keep converting back and forth between them. For now, just add the naked table with name. Subsequent patches will start piping this in via intel_ddi_init(). v2: do not try to generalize the checks for port presence nor dsi initialization. Instead focus on getting the ddi table created for all platforms using DDI and keep their differences in the original functionI like this.drm/i915/display: description-based initialization for remaining ddi platformsStray line?
yep, happens a lot to me when squashing changes
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> --- drivers/gpu/drm/i915/display/intel_display.c | 141 ++++++++++++------ .../drm/i915/display/intel_display_types.h | 5 + 2 files changed, 99 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index effd6b65f270..c234b50212b0 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -16805,6 +16805,83 @@ static void intel_pps_init(struct drm_i915_private *dev_priv) intel_pps_unlock_regs_wa(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 }, + { .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 }, + { .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 }, + { .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 }, + { .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 }, + { .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 }, + { .port = PORT_NONE } +};These make me wonder about a potential future restructuring of moving the output setup stuff to a separate file. No need to do that here, just throwing ideas around.
yeah, I think it would make sense to later let the tables live alone in a single .c file. Another possibility that reduces conflicts is to have each platform in its own .c file and either #include the .c or declare the table as extern.
+ +/* + * Use a description-based approach for platforms that can be supported with a + * static table + * + * @disable_mask: any port that should not be enabled due to being disabled by + * any reason + */ +static void setup_ddi_outputs_desc(struct drm_i915_private *i915, + const struct intel_ddi_port_info *ports, + unsigned long disable_mask) +{ + const struct intel_ddi_port_info *port_info; + + for (port_info = ports; + port_info->port != PORT_NONE; port_info++) { + if (test_bit(port_info->port, &disable_mask)) + continue; + + intel_ddi_init(i915, port_info->port); + } +} + static void intel_setup_outputs(struct drm_i915_private *dev_priv) { struct intel_encoder *encoder; @@ -16816,46 +16893,21 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) return; if (IS_ROCKETLAKE(dev_priv)) { - intel_ddi_init(dev_priv, PORT_A); - intel_ddi_init(dev_priv, PORT_B); - intel_ddi_init(dev_priv, PORT_D); /* DDI TC1 */ - intel_ddi_init(dev_priv, PORT_E); /* DDI TC2 */ + setup_ddi_outputs_desc(dev_priv, rkl_ports, 0); } else if (INTEL_GEN(dev_priv) >= 12) { - intel_ddi_init(dev_priv, PORT_A); - intel_ddi_init(dev_priv, PORT_B); - intel_ddi_init(dev_priv, PORT_D); - intel_ddi_init(dev_priv, PORT_E); - intel_ddi_init(dev_priv, PORT_F); - intel_ddi_init(dev_priv, PORT_G); - intel_ddi_init(dev_priv, PORT_H); - intel_ddi_init(dev_priv, PORT_I); + setup_ddi_outputs_desc(dev_priv, tgl_ports, 0); icl_dsi_init(dev_priv); } else if (IS_ELKHARTLAKE(dev_priv)) { - intel_ddi_init(dev_priv, PORT_A); - intel_ddi_init(dev_priv, PORT_B); - intel_ddi_init(dev_priv, PORT_C); - intel_ddi_init(dev_priv, PORT_D); + setup_ddi_outputs_desc(dev_priv, ehl_ports, 0); icl_dsi_init(dev_priv); } else if (IS_GEN(dev_priv, 11)) { - intel_ddi_init(dev_priv, PORT_A); - intel_ddi_init(dev_priv, PORT_B); - intel_ddi_init(dev_priv, PORT_C); - intel_ddi_init(dev_priv, PORT_D); - intel_ddi_init(dev_priv, PORT_E); - intel_ddi_init(dev_priv, PORT_F); + setup_ddi_outputs_desc(dev_priv, icl_ports, 0); icl_dsi_init(dev_priv); } else if (IS_GEN9_LP(dev_priv)) { - /* - * FIXME: Broxton doesn't support port detection via the - * DDI_BUF_CTL_A or SFUSE_STRAP registers, find another way to - * detect the ports. - */ - intel_ddi_init(dev_priv, PORT_A); - intel_ddi_init(dev_priv, PORT_B); - intel_ddi_init(dev_priv, PORT_C); - + setup_ddi_outputs_desc(dev_priv, gen9lp_ports, 0); vlv_dsi_init(dev_priv); } else if (HAS_DDI(dev_priv)) { + unsigned long disable_mask = 0; int found; if (intel_ddi_crt_present(dev_priv)) @@ -16869,28 +16921,23 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) */ found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED; /* WaIgnoreDDIAStrap: skl */ - if (found || IS_GEN9_BC(dev_priv)) - intel_ddi_init(dev_priv, PORT_A); + if (!found && !IS_GEN9_BC(dev_priv)) + disable_mask |= BIT(PORT_A); /* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP * register */ found = intel_de_read(dev_priv, SFUSE_STRAP); - if (found & SFUSE_STRAP_DDIB_DETECTED) - intel_ddi_init(dev_priv, PORT_B); - if (found & SFUSE_STRAP_DDIC_DETECTED) - intel_ddi_init(dev_priv, PORT_C); - if (found & SFUSE_STRAP_DDID_DETECTED) - intel_ddi_init(dev_priv, PORT_D); - if (found & SFUSE_STRAP_DDIF_DETECTED) - intel_ddi_init(dev_priv, PORT_F); - /* - * On SKL we don't have a way to detect DDI-E so we rely on VBT. - */ - if (IS_GEN9_BC(dev_priv) && - intel_bios_is_port_present(dev_priv, PORT_E)) - intel_ddi_init(dev_priv, PORT_E); + if (!(found & SFUSE_STRAP_DDIB_DETECTED)) + disable_mask |= BIT(PORT_B); + if (!(found & SFUSE_STRAP_DDIC_DETECTED)) + disable_mask |= BIT(PORT_C); + if (!(found & SFUSE_STRAP_DDID_DETECTED)) + disable_mask |= BIT(PORT_D); + if (!(found & SFUSE_STRAP_DDIF_DETECTED)) + disable_mask |= BIT(PORT_F); + setup_ddi_outputs_desc(dev_priv, ddi_ports, disable_mask); } else if (HAS_PCH_SPLIT(dev_priv)) { int found; diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 4b0aaa3081c9..92cc7fc66bce 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1433,6 +1433,11 @@ struct intel_dp_mst_encoder { struct intel_connector *connector; }; +struct intel_ddi_port_info {Just thinking out loud, should we have a "struct port" or "struct intel_port" instead. Maybe, maybe not. *shrug*
maybe in future... I think that would mean converting the previous platforms as well. I'm willing to go as far as when ddi started to be used. When those are done we can think about the rest.
Anyway the patch is Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
thanks Lucas De Marchi
+ const char *name; + enum port port; +}; + static inline enum dpio_channel vlv_dport_to_channel(struct intel_digital_port *dport) {-- Jani Nikula, Intel Open Source Graphics Center
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
