| Message ID | 20180522002558.29262-20-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 21, 2018 at 05:25:53PM -0700, Paulo Zanoni wrote: > The type is detected based on the interrupt ISR bit. Once detected, > it's not supposed to be changed, so we have some sanity checks for > that. > > Cc: Animesh Manna <animesh.manna@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_display.h | 7 +++++++ > drivers/gpu/drm/i915/intel_dp.c | 36 +++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h > index c88185ed7594..fcedc600706b 100644 > --- a/drivers/gpu/drm/i915/intel_display.h > +++ b/drivers/gpu/drm/i915/intel_display.h > @@ -137,6 +137,13 @@ enum tc_port { > I915_MAX_TC_PORTS > }; > > +enum tc_port_type { > + TC_PORT_UNKNOWN = 0, > + TC_PORT_TYPEC, > + TC_PORT_TBT, > + TC_PORT_LEGACY, > +}; > + > enum dpio_channel { > DPIO_CH0, > DPIO_CH1 > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b477124717e7..f3d5b9eed625 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4730,6 +4730,38 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv, > return I915_READ(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(port); > } > > +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > + struct intel_digital_port *intel_dig_port, > + bool is_legacy, bool is_typec, bool is_tbt) > +{ > + enum port port = intel_dig_port->base.port; > + enum tc_port_type old_type = intel_dig_port->tc_type; > + const char *type_str; > + > + WARN_ON(is_legacy + is_typec + is_tbt != 1); > + > + if (is_legacy) { > + intel_dig_port->tc_type = TC_PORT_LEGACY; > + type_str = "legacy"; > + } else if (is_typec) { > + intel_dig_port->tc_type = TC_PORT_TYPEC; > + type_str = "typec"; > + } else if (is_tbt) { > + intel_dig_port->tc_type = TC_PORT_TBT; > + type_str = "tbt"; > + } else { > + return; > + } > + > + /* Types are not supposed to be changed at runtime. */ > + WARN_ON(old_type != TC_PORT_UNKNOWN && > + old_type != intel_dig_port->tc_type); > + > + if (old_type != intel_dig_port->tc_type) > + DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port), > + type_str); > +} > + > static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > struct intel_digital_port *intel_dig_port) > { > @@ -4750,10 +4782,12 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > 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; > > + icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec, > + is_tbt); I really don't like the new chain of functions this patch here starts. for all other platforms the function is_port_connect returns true or false immediately. For this TC/TBT design this start a new chain that not only check if it is connected but it also updates the status... and all in a chain of function calls.... I didn't check the code now actually. I just remember for one rebase change that I did a while ago and saw these patches. Unfortunately I had no better idea on when exactly call the current status when I looked. Probably a totally separated function that is called outside right always along with is_port_connected update_tc_port() is_port_connected() just to keep is_port_connect as simple as it is on any other platform and this new meaning of status update in a separated block. Thanks, Rodrigo. > + > return true; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index a54232c270e1..8602f2e17d86 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1169,6 +1169,7 @@ struct intel_digital_port { > bool release_cl2_override; > uint8_t max_lanes; > enum intel_display_power_domain ddi_io_power_domain; > + enum tc_port_type tc_type; > > void (*write_infoframe)(struct drm_encoder *encoder, > const struct intel_crtc_state *crtc_state, > -- > 2.14.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2018-06-14 às 12:59 -0700, Rodrigo Vivi escreveu: > On Mon, May 21, 2018 at 05:25:53PM -0700, Paulo Zanoni wrote: > > The type is detected based on the interrupt ISR bit. Once detected, > > it's not supposed to be changed, so we have some sanity checks for > > that. > > > > Cc: Animesh Manna <animesh.manna@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.h | 7 +++++++ > > drivers/gpu/drm/i915/intel_dp.c | 36 > > +++++++++++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 3 files changed, 43 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h > > b/drivers/gpu/drm/i915/intel_display.h > > index c88185ed7594..fcedc600706b 100644 > > --- a/drivers/gpu/drm/i915/intel_display.h > > +++ b/drivers/gpu/drm/i915/intel_display.h > > @@ -137,6 +137,13 @@ enum tc_port { > > I915_MAX_TC_PORTS > > }; > > > > +enum tc_port_type { > > + TC_PORT_UNKNOWN = 0, > > + TC_PORT_TYPEC, > > + TC_PORT_TBT, > > + TC_PORT_LEGACY, > > +}; > > + > > enum dpio_channel { > > DPIO_CH0, > > DPIO_CH1 > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index b477124717e7..f3d5b9eed625 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4730,6 +4730,38 @@ static bool icl_combo_port_connected(struct > > drm_i915_private *dev_priv, > > return I915_READ(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(port); > > } > > > > +static void icl_update_tc_port_type(struct drm_i915_private > > *dev_priv, > > + struct intel_digital_port > > *intel_dig_port, > > + bool is_legacy, bool is_typec, > > bool is_tbt) > > +{ > > + enum port port = intel_dig_port->base.port; > > + enum tc_port_type old_type = intel_dig_port->tc_type; > > + const char *type_str; > > + > > + WARN_ON(is_legacy + is_typec + is_tbt != 1); > > + > > + if (is_legacy) { > > + intel_dig_port->tc_type = TC_PORT_LEGACY; > > + type_str = "legacy"; > > + } else if (is_typec) { > > + intel_dig_port->tc_type = TC_PORT_TYPEC; > > + type_str = "typec"; > > + } else if (is_tbt) { > > + intel_dig_port->tc_type = TC_PORT_TBT; > > + type_str = "tbt"; > > + } else { > > + return; > > + } > > + > > + /* Types are not supposed to be changed at runtime. */ > > + WARN_ON(old_type != TC_PORT_UNKNOWN && > > + old_type != intel_dig_port->tc_type); > > + > > + if (old_type != intel_dig_port->tc_type) > > + DRM_DEBUG_KMS("Port %c has TC type %s\n", > > port_name(port), > > + type_str); > > +} > > + > > static bool icl_tc_port_connected(struct drm_i915_private > > *dev_priv, > > struct intel_digital_port > > *intel_dig_port) > > { > > @@ -4750,10 +4782,12 @@ static bool icl_tc_port_connected(struct > > drm_i915_private *dev_priv, > > 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; > > > > + icl_update_tc_port_type(dev_priv, intel_dig_port, > > is_legacy, is_typec, > > + is_tbt); > > I really don't like the new chain of functions this patch here > starts. I don't like it either, but the hardware changed in a way that is different from every previous platform. > > for all other platforms the function is_port_connect returns true or > false > immediately. For this TC/TBT design this start a new chain that not > only > check if it is connected but it also updates the status... and all in > a > chain of function calls.... We have to figure out the port type (in case it's tc) during the hotplug anyway since that's part of the hotplug sequence, for the DFLEXDP* dance. If we keep passing it as a local variable and opt to do later, we'll have to re-read the ISR bits anyway and we'll have to do it right after. I don't see how that would actually help anything: it could only increase the risk of de-sync when functions get moved around. > > I didn't check the code now actually. I just remember for one rebase > change that I did a while ago and saw these patches. Unfortunately I > had no better idea on when exactly call the current status when I > looked. > > Probably a totally separated function that is called outside right > always > along with is_port_connected > > update_tc_port() > is_port_connected() This would need to be in the inverse order and it would pretty much just re-read the ISR bits again. Doable, but wouldn't solve the main problem you complain about: is_connected() would still do a lot more than just reading the ISR bits, in fact it would look exactly the same as right now except it wouldn't set dig_port->tc_type. > > just to keep is_port_connect as simple as it is on any other platform > and this new meaning of status update in a separated block. This is not possible due to the DFLEXDP* dance. I think you're mad at the DFLEXDP* dance and is unleashing your rage at poor dig_port- >tc_type. Please read the rest of the series and tell me what you think should really be done here. Open review comments like "this is bad but I haven't read the other patches in order to formulate a proper suggestion" don't help very much. Thanks, Paulo > > Thanks, > Rodrigo. > > > + > > return true; > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index a54232c270e1..8602f2e17d86 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1169,6 +1169,7 @@ struct intel_digital_port { > > bool release_cl2_override; > > uint8_t max_lanes; > > enum intel_display_power_domain ddi_io_power_domain; > > + enum tc_port_type tc_type; > > > > void (*write_infoframe)(struct drm_encoder *encoder, > > const struct intel_crtc_state > > *crtc_state, > > -- > > 2.14.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h index c88185ed7594..fcedc600706b 100644 --- a/drivers/gpu/drm/i915/intel_display.h +++ b/drivers/gpu/drm/i915/intel_display.h @@ -137,6 +137,13 @@ enum tc_port { I915_MAX_TC_PORTS }; +enum tc_port_type { + TC_PORT_UNKNOWN = 0, + TC_PORT_TYPEC, + TC_PORT_TBT, + TC_PORT_LEGACY, +}; + enum dpio_channel { DPIO_CH0, DPIO_CH1 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b477124717e7..f3d5b9eed625 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4730,6 +4730,38 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv, return I915_READ(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(port); } +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, + struct intel_digital_port *intel_dig_port, + bool is_legacy, bool is_typec, bool is_tbt) +{ + enum port port = intel_dig_port->base.port; + enum tc_port_type old_type = intel_dig_port->tc_type; + const char *type_str; + + WARN_ON(is_legacy + is_typec + is_tbt != 1); + + if (is_legacy) { + intel_dig_port->tc_type = TC_PORT_LEGACY; + type_str = "legacy"; + } else if (is_typec) { + intel_dig_port->tc_type = TC_PORT_TYPEC; + type_str = "typec"; + } else if (is_tbt) { + intel_dig_port->tc_type = TC_PORT_TBT; + type_str = "tbt"; + } else { + return; + } + + /* Types are not supposed to be changed at runtime. */ + WARN_ON(old_type != TC_PORT_UNKNOWN && + old_type != intel_dig_port->tc_type); + + if (old_type != intel_dig_port->tc_type) + DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port), + type_str); +} + static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, struct intel_digital_port *intel_dig_port) { @@ -4750,10 +4782,12 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, 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; + icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec, + is_tbt); + return true; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a54232c270e1..8602f2e17d86 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1169,6 +1169,7 @@ struct intel_digital_port { bool release_cl2_override; uint8_t max_lanes; enum intel_display_power_domain ddi_io_power_domain; + enum tc_port_type tc_type; void (*write_infoframe)(struct drm_encoder *encoder, const struct intel_crtc_state *crtc_state,