All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 7/9] drm/i915/display: add phy, vbt and ddi indexes
Date: Mon, 23 Dec 2019 16:17:44 -0800	[thread overview]
Message-ID: <20191224001744.trefroqjqy5j3o5f@ldmartin-desk1> (raw)
In-Reply-To: <20191224001044.GW2877816@mdroper-desk1.amr.corp.intel.com>

On Mon, Dec 23, 2019 at 04:10:44PM -0800, Matt Roper wrote:
>On Mon, Dec 23, 2019 at 11:58:48AM -0800, 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
>> on 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.
>>
>> With this new table the approach will be: the port as defined by the
>> enum port is purely 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 (although the helper
>> functions can remain so we may differentiate between, e.g. Dekel and MG
>> phys).
>>
>> While at it, also give names to the ports so they can be easily
>> identified.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c  | 54 +++++++++----------
>>  drivers/gpu/drm/i915/display/intel_display.h  |  7 +++
>>  .../drm/i915/display/intel_display_types.h    |  5 ++
>>  3 files changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index ad85cf75c815..219f180fa395 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -16277,14 +16277,14 @@ static bool ddi_is_port_present(struct drm_i915_private *i915,
>>  static const struct intel_output tgl_output = {
>>  	.dsi_init = icl_dsi_init,
>>  	.ddi_ports = {
>> -		{ .port = PORT_A },
>> -		{ .port = PORT_B },
>> -		{ .port = PORT_D },
>> -		{ .port = PORT_E },
>> -		{ .port = PORT_F },
>> -		{ .port = PORT_G },
>> -		{ .port = PORT_H },
>> -		{ .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, },
>> +		{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
>> +		{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
>> +		{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
>> +		{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
>> +		{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
>> +		{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
>>  		{ .port = PORT_NONE }
>>  	}
>>  };
>> @@ -16293,12 +16293,12 @@ static const struct intel_output icl_output = {
>>  	.dsi_init = icl_dsi_init,
>>  	.is_port_present = icl_is_port_present,
>>  	.ddi_ports = {
>> -		{ .port = PORT_A },
>> -		{ .port = PORT_B },
>> -		{ .port = PORT_C },
>> -		{ .port = PORT_D },
>> -		{ .port = PORT_E },
>> -		{ .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_TC,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2, },
>> +		{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3, },
>> +		{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4, },
>> +		{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_TC,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5, },
>>  		{ .port = PORT_NONE }
>>  	}
>>  };
>> @@ -16306,10 +16306,10 @@ static const struct intel_output icl_output = {
>>  static const struct intel_output ehl_output = {
>>  	.dsi_init = icl_dsi_init,
>>  	.ddi_ports = {
>> -		{ .port = PORT_A },
>> -		{ .port = PORT_B },
>> -		{ .port = PORT_C },
>> -		{ .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 }
>>  	}
>>  };
>> @@ -16317,9 +16317,9 @@ static const struct intel_output ehl_output = {
>>  static const struct intel_output gen9lp_output = {
>>  	.dsi_init = vlv_dsi_init,
>>  	.ddi_ports = {
>> -		{ .port = PORT_A },
>> -		{ .port = PORT_B },
>> -		{ .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 }
>>  	},
>>  };
>> @@ -16327,12 +16327,12 @@ static const struct intel_output gen9lp_output = {
>>  static const struct intel_output ddi_output = {
>>  	.is_port_present = ddi_is_port_present,
>>  	.ddi_ports = {
>> -		{ .port = PORT_A },
>> -		{ .port = PORT_B },
>> -		{ .port = PORT_C },
>> -		{ .port = PORT_D },
>> -		{ .port = PORT_E },
>> -		{ .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 05d68bd393dc..db19e5eee248 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>> @@ -303,6 +303,13 @@ enum phy {
>>  	I915_MAX_PHYS
>>  };
>>
>> +enum phy_type {
>> +	PHY_TYPE_NONE = 0,
>> +
>> +	PHY_TYPE_COMBO,
>> +	PHY_TYPE_TC,
>> +};
>
>Why not go ahead and actually specify the true phy type in the enum?
>
>        enum phy_type {
>                PHY_TYPE_NONE = 0,
>
>                PHY_TYPE_COMBO,
>                PHY_TYPE_MG,
>                PHY_TYPE_DKL,
>                PHY_TYPE_FOO,
>                PHY_TYPE_BAR,
>        };
>
>and then once your series is complete the helpers can figure out whether
>something is "type c" with something like
>   type == MG || type == DKL || type == BAR

yeah, I thought about that but I couldn't find a compelling reason to
differentiate DKL and MG.  There's only a single place where it could be
useful, but I didn't convert enough of the driver to make sure it
actually is.

that's a good option indeed, I will think about it while doing the rest
of the conversion if people agree with the general idea on this series.

thanks
Lucas De Marchi

>
>
>Matt
>
>> +
>>  #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 4d2f4ee35812..23a885895803 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1377,7 +1377,12 @@ 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.24.0
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>VTT-OSGC Platform Enablement
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2019-12-24  0:17 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-23 19:58 [Intel-gfx] [PATCH 0/9] RFC: display/ddi: keep register indexes in a table Lucas De Marchi
2019-12-23 19:58 ` [Intel-gfx] [PATCH 1/9] drm/i915/display: nuke skl workaround for pre-production hw Lucas De Marchi
2019-12-23 19:58 ` [Intel-gfx] [PATCH 2/9] drm/i915/display: remove alias to dig_port Lucas De Marchi
2019-12-23 21:05   ` Matt Roper
2019-12-23 19:58 ` [Intel-gfx] [PATCH 3/9] drm/i915/display: prefer the more common dig_port name Lucas De Marchi
2019-12-23 21:16   ` Matt Roper
2019-12-23 21:25     ` Lucas De Marchi
2019-12-23 19:58 ` [Intel-gfx] [PATCH 4/9] drm/i915/display: start description-based ddi initialization Lucas De Marchi
2019-12-31  9:58   ` Jani Nikula
2020-01-02  7:19     ` Lucas De Marchi
2020-06-22 23:50       ` Lucas De Marchi
2020-07-03 13:22         ` Jani Nikula
2019-12-23 19:58 ` [Intel-gfx] [PATCH 5/9] drm/i915/display: move icl to description-based ddi init Lucas De Marchi
2019-12-24  0:00   ` Matt Roper
2019-12-31 10:14   ` Jani Nikula
2020-01-02  7:32     ` Lucas De Marchi
2019-12-23 19:58 ` [Intel-gfx] [PATCH 6/9] drm/i915/display: description-based initialization for remaining ddi platforms Lucas De Marchi
2019-12-31 10:25   ` Jani Nikula
2019-12-31 10:26     ` Jani Nikula
2020-01-02  7:23     ` Lucas De Marchi
2019-12-23 19:58 ` [Intel-gfx] [PATCH 7/9] drm/i915/display: add phy, vbt and ddi indexes Lucas De Marchi
2019-12-24  0:10   ` Matt Roper
2019-12-24  0:17     ` Lucas De Marchi [this message]
2019-12-31 10:33   ` Jani Nikula
2020-01-02  7:50     ` Lucas De Marchi
2019-12-23 19:58 ` [Intel-gfx] [PATCH 8/9] drm/i915/display: refer to vbt info as vbt_port_info Lucas De Marchi
2019-12-31 10:39   ` Jani Nikula
2019-12-23 19:58 ` [Intel-gfx] [PATCH 9/9] drm/i915/display: use port_info on intel_ddi_init Lucas De Marchi
2019-12-24  0:16   ` Matt Roper
2019-12-31 11:20     ` Jani Nikula
2020-06-23  1:11       ` Lucas De Marchi
2019-12-31 11:22   ` Jani Nikula
2019-12-23 20:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for RFC: display/ddi: keep register indexes in a table Patchwork
2019-12-23 20:57 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191224001744.trefroqjqy5j3o5f@ldmartin-desk1 \
    --to=lucas.demarchi@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=matthew.d.roper@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.