On Mon, May 21, 2018 at 05:25:52PM -0700, Paulo Zanoni wrote:
> Do like the other functions and check for the ISR bits. We have plans
> to add a few more checks in this code in the next patches, that's why
> it's a little more verbose than it could be.
>
> Cc: Animesh Manna <animesh.manna@xxxxxxxxx>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 5 ++++
> drivers/gpu/drm/i915/intel_dp.c | 57 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 49a72320e794..24308d4435f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7055,6 +7055,7 @@ enum {
> #define GEN11_TC3_HOTPLUG (1 << 18)
> #define GEN11_TC2_HOTPLUG (1 << 17)
> #define GEN11_TC1_HOTPLUG (1 << 16)
> +#define GEN11_TC_HOTPLUG(tc_port) (1 << ((tc_port) + 16))
> #define GEN11_DE_TC_HOTPLUG_MASK (GEN11_TC4_HOTPLUG | \
> GEN11_TC3_HOTPLUG | \
> GEN11_TC2_HOTPLUG | \
> @@ -7063,6 +7064,7 @@ enum {
> #define GEN11_TBT3_HOTPLUG (1 << 2)
> #define GEN11_TBT2_HOTPLUG (1 << 1)
> #define GEN11_TBT1_HOTPLUG (1 << 0)
> +#define GEN11_TBT_HOTPLUG(tc_port) (1 << (tc_port))
> #define GEN11_DE_TBT_HOTPLUG_MASK (GEN11_TBT4_HOTPLUG | \
> GEN11_TBT3_HOTPLUG | \
> GEN11_TBT2_HOTPLUG | \
> @@ -7486,6 +7488,9 @@ enum {
> #define ICP_GMBUS (1 << 23)
> #define ICP_DDIB_HOTPLUG (1 << 17)
> #define ICP_DDIA_HOTPLUG (1 << 16)
> +#define ICP_TC_HOTPLUG(tc_port) (1 << ((tc_port) + 24))
> +#define ICP_DDI_HOTPLUG(port) (1 << ((port) + 16))
> +
>
> #define ICP_SDE_DDI_MASK (ICP_DDIB_HOTPLUG | \
> ICP_DDIA_HOTPLUG)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 102070940095..b477124717e7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4722,6 +4722,61 @@ static bool bxt_digital_port_connected(struct intel_encoder *encoder)
> return I915_READ(GEN8_DE_PORT_ISR) & bit;
> }
>
> +static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
> + struct intel_digital_port *intel_dig_port)
> +{
> + enum port port = intel_dig_port->base.port;
> +
> + return I915_READ(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(port);
> +}
> +
> +static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
> + struct intel_digital_port *intel_dig_port)
> +{
> + enum port port = intel_dig_port->base.port;
> + enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> + u32 legacy_bit = ICP_TC_HOTPLUG(tc_port);
> + u32 typec_bit = GEN11_TC_HOTPLUG(tc_port);
> + u32 tbt_bit = GEN11_TBT_HOTPLUG(tc_port);
> + bool is_legacy = false, is_typec = false, is_tbt = false;
> + u32 cpu_isr;
why *cpu*_isr? hpd_isr, isr or val would be better IMO
> +
> + if (I915_READ(ICP_SDE_ISR) & legacy_bit)
> + is_legacy = true;
> +
> + cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
> + if (cpu_isr & typec_bit)
> + is_typec = true;
> + if (cpu_isr & tbt_bit)
> + is_tbt = true;
> +
> + WARN_ON(is_legacy + is_typec + is_tbt > 1);
> + if (!is_legacy && !is_typec && !is_tbt)
> + return false;
> +
> + return true;
you know you could "return is_legacy + is_typec + is_tbt;", right? you
are already doing it above, it may make sense to remove the extra
branch. Or not.
Feel free to disagree and push.
Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Lucas De Marchi
> +}
> +
> +static bool icl_digital_port_connected(struct intel_encoder *encoder)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> +
> + switch (encoder->hpd_pin) {
> + case HPD_PORT_A:
> + case HPD_PORT_B:
> + return icl_combo_port_connected(dev_priv, dig_port);
> + case HPD_PORT_C:
> + case HPD_PORT_D:
> + case HPD_PORT_E:
> + case HPD_PORT_F:
> + return icl_tc_port_connected(dev_priv, dig_port);
> + default:
> + MISSING_CASE(encoder->hpd_pin);
> + return false;
> + }
> +}
> +
> /*
> * intel_digital_port_connected - is the specified port connected?
> * @encoder: intel_encoder
> @@ -4749,6 +4804,8 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
> return bdw_digital_port_connected(encoder);
> else if (IS_GEN9_LP(dev_priv))
> return bxt_digital_port_connected(encoder);
> + else if (IS_ICELAKE(dev_priv))
> + return icl_digital_port_connected(encoder);
> else
> return spt_digital_port_connected(encoder);
> }
> --
> 2.14.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
