[Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

Fri Oct 23 00:01:30 UTC 2020

On Fri, Oct 23, 2020 at 02:56:21AM +0300, Ville Syrjälä wrote:
>On Thu, Oct 08, 2020 at 01:52:30AM -0700, Lucas De Marchi wrote:
>> On Thu, Oct 08, 2020 at 11:40:28AM +0300, Ville Syrjälä wrote:
>> >On Wed, Oct 07, 2020 at 03:51:11PM -0700, Lucas De Marchi wrote:
>> >> On Tue, Oct 06, 2020 at 05:33:34PM +0300, Ville Syrjälä wrote:
>> >> >From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >> >
>> >> >Just like with the DDIs tgl+ renamed the AUX CHs to reflect
>> >> >the type of the DDI. Let's add the aliasing enum values for
>> >> >the type-C AUX CHs.
>> >> >
>> >> >Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >> >---
>> >> > drivers/gpu/drm/i915/display/intel_display.h |  8 +++
>> >> > drivers/gpu/drm/i915/display/intel_dp.c      | 53 ++++++++++++++++++--
>> >> > 2 files changed, 58 insertions(+), 3 deletions(-)
>> >> >
>> >> >diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>> >> >index a39be3c9e0cf..cba876721ea0 100644
>> >> >--- a/drivers/gpu/drm/i915/display/intel_display.h
>> >> >+++ b/drivers/gpu/drm/i915/display/intel_display.h
>> >> >@@ -290,6 +290,14 @@ enum aux_ch {
>> >> > 	AUX_CH_G,
>> >> > 	AUX_CH_H,
>> >> > 	AUX_CH_I,
>> >> >+
>> >> >+	/* tgl+ */
>> >> >+	AUX_CH_USBC1 = AUX_CH_D,
>> >> >+	AUX_CH_USBC2,
>> >> >+	AUX_CH_USBC3,
>> >> >+	AUX_CH_USBC4,
>> >> >+	AUX_CH_USBC5,
>> >> >+	AUX_CH_USBC6,
>> >> > };
>> >> >
>> >> > #define aux_ch_name(a) ((a) + 'A')
>> >> >diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> >index 239016dcd544..a73c354c920e 100644
>> >> >--- a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> >+++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> >@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
>> >> > 	case AUX_CH_D:
>> >> > 	case AUX_CH_E:
>> >> > 	case AUX_CH_F:
>> >> >-	case AUX_CH_G:
>> >> > 		return DP_AUX_CH_CTL(aux_ch);
>> >> > 	default:
>> >> > 		MISSING_CASE(aux_ch);
>> >> >@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
>> >> > 	case AUX_CH_D:
>> >> > 	case AUX_CH_E:
>> >> > 	case AUX_CH_F:
>> >> >-	case AUX_CH_G:
>> >> >+		return DP_AUX_CH_DATA(aux_ch, index);
>> >> >+	default:
>> >> >+		MISSING_CASE(aux_ch);
>> >> >+		return DP_AUX_CH_DATA(AUX_CH_A, index);
>> >> >+	}
>> >> >+}
>> >> >+
>> >> >+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
>> >> >+{
>> >> >+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> >> >+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> >> >+	enum aux_ch aux_ch = dig_port->aux_ch;
>> >> >+
>> >> >+	switch (aux_ch) {
>> >> >+	case AUX_CH_A:
>> >> >+	case AUX_CH_B:
>> >> >+	case AUX_CH_C:
>> >> >+	case AUX_CH_USBC1:
>> >> >+	case AUX_CH_USBC2:
>> >> >+	case AUX_CH_USBC3:
>> >> >+	case AUX_CH_USBC4:
>> >> >+	case AUX_CH_USBC5:
>> >> >+	case AUX_CH_USBC6:
>> >> >+		return DP_AUX_CH_CTL(aux_ch);
>> >> >+	default:
>> >> >+		MISSING_CASE(aux_ch);
>> >> >+		return DP_AUX_CH_CTL(AUX_CH_A);
>> >> >+	}
>> >> >+}
>> >> >+
>> >> >+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
>> >> >+{
>> >> >+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> >> >+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> >> >+	enum aux_ch aux_ch = dig_port->aux_ch;
>> >> >+
>> >> >+	switch (aux_ch) {
>> >> >+	case AUX_CH_A:
>> >> >+	case AUX_CH_B:
>> >> >+	case AUX_CH_C:
>> >> >+	case AUX_CH_USBC1:
>> >> >+	case AUX_CH_USBC2:
>> >> >+	case AUX_CH_USBC3:
>> >> >+	case AUX_CH_USBC4:
>> >> >+	case AUX_CH_USBC5:
>> >> >+	case AUX_CH_USBC6:
>> >> > 		return DP_AUX_CH_DATA(aux_ch, index);
>> >> > 	default:
>> >> > 		MISSING_CASE(aux_ch);
>> >> >@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
>> >> > 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> >> > 	struct intel_encoder *encoder = &dig_port->base;
>> >> >
>> >> >-	if (INTEL_GEN(dev_priv) >= 9) {
>> >> >+	if (INTEL_GEN(dev_priv) >= 12) {
>> >> >+		intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;
>> >>
>> >> why is this even a function pointer rather than just the reg? AFAICS it
>> >> only depends on dig_port->aux_ch that is initialized in intel_ddi_init()
>> >
>> >Just for consistency with .aux_ch_data_reg() I guess. Can't remember
>> >a more specific reason at least.
>>
>> even that may be overkill since all the users just use index to
>> do `+ index * 4`
>
>The code used to do that but we got rid of it when the i915_reg
>stuff was introduced to discourage people from doing hand rolled
>arithmetic on register offsets. I think the tradeoff has been
>generally worth it because I can't remeber the last time someone
>messed up the register offsets. Before type safety it was a
>somewhat regular occurance.

ok. Just so patchwork gets it now:

Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi

>
>-- 
>Ville Syrjälä
>Intel
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx