Re: [PATCH 3/5] drm/i915: lspcon detection

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, May 31, 2016 at 02:55:44PM +0530, Shashank Sharma wrote:
> lspcon is a tricky device to detect.
> When in LS mode:
> 	It should be detected as a HDMI device, by reading EDID, on
> 	I2C over Aux chanel
> 
> When in PCON mode:
> 	It should be detected as a DP device by reading DPCD over the
> 	Aux channel.
> 
> This patch accommodates these specific requirements of lspcon detection
> by adding appropriate changes in I915 drivers HDMI/DP detection sequence.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c    | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c     | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c   | 14 ++++++++++----
>  drivers/gpu/drm/i915/intel_lspcon.c |  6 ++++++
>  5 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c454744..811c829 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  {
>  	int type = encoder->type;
>  	int port = intel_ddi_get_encoder_port(encoder);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>  
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> +	/*
> +	* A digital port with active lspcon device, should be detected
> +	* as HDMI when in LS mode and as DP when in PCON mode.
> +	*/
> +	if (is_lspcon_active(dig_port)) {
> +		struct intel_lspcon *lspcon = &dig_port->lspcon;
> +
> +		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
> +			type = INTEL_OUTPUT_HDMI;
> +		else
> +			type = INTEL_OUTPUT_DISPLAYPORT;
> +	}

Argh. I really wanted to kill this dual role DDI encoder mess (see [1])

I realize that with LSPCON we probably don't want separate encoders for
the two roles. Or maybe we do? At least we don't want two connectors,
which probably means two encoders might become messy.

In any case I don't think we want to be frobbing around with the
encoder->type anymore. Maybe I need to rethink my approach to DDI
encoders, and try to come up with something sane. For LSPCON at least
we want to keep track of the current mode in pipe_config most likely.
For LSPCON it's maybe a bit easier since the mode won't affect the
DDC stuff and whanot, so from the probe side there is just one role
except perhaps w.r.t. DPCD. For regular DDI stuff I'm still thinking
two encoder might be the most sensible apporach since we have totally
different paths for probe as well.

Probably the biggest kink in all of this is hpd handling. What, if
anything, should hpd_pulse do for LSPCON? And if something, should
it only do it in PCON mode? How is HPD routed anyway with LSPCON?

[1] https://patchwork.freedesktop.org/series/1596/

> +
>  	if (type == INTEL_OUTPUT_HDMI)
>  		return intel_hdmi_compute_config(encoder, pipe_config);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aa9c59e..39ce16e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> +	/*
> +	* LSPCON is a DP->HDMI converter which should be detected as
> +	* HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS
> +	* mode, do not try to read DPCD, but detect as HDMI.
> +	*/
> +	if (is_lspcon_active(intel_dig_port)) {
> +		struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> +
> +		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
> +			return lspcon_ls_mode_detect(connector, force);
> +	}
> +
>  	if (intel_dp->is_mst) {
>  		/* MST devices are disconnected from a monitor POV */
>  		intel_dp_unset_edid(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 205a463..fa77886 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *pipe_config);
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> +enum drm_connector_status
> +intel_hdmi_detect(struct drm_connector *connector, bool force);
>  
>  /* intel_lvds.c */
>  void intel_lvds_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 6b52c6a..79184e2 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +	struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi);
> +	struct i2c_adapter *adapter;
>  	struct edid *edid = NULL;
>  	bool connected = false;
>  
>  	if (force) {
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>  
> -		edid = drm_get_edid(connector,
> -				    intel_gmbus_get_adapter(dev_priv,
> -				    intel_hdmi->ddc_bus));
> +		if (is_lspcon_active(dig_port))
> +			adapter = &dig_port->lspcon.aux->ddc;
> +		else
> +			adapter = intel_gmbus_get_adapter(dev_priv,
> +				intel_hdmi->ddc_bus);
> +
> +		edid = drm_get_edid(connector, adapter);

I'm not a fan of this. This is even taking the wrong power domain
now, as does intel_hdmi_detect(). Probably LSPCON should just have
a dedicated codepath for this stuff.

If we ever have a board design with a DP++ port without a GMBUS
connection, then we might have rethink things because then we'd have to
use AUX also for HDMI DDC. But so far I'm not aware of such board
designs existing, so might as well keep LSPCON separate.

>  
>  		intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
>  
> @@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  	return connected;
>  }
>  
> -static enum drm_connector_status
> +enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
>  	enum drm_connector_status status;
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index dd50491..75b5028 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port)
>  	return dig_port->lspcon.active;
>  }
>  
> +enum drm_connector_status
> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force)
> +{
> +	return intel_hdmi_detect(connector, force);
> +}
> +
>  enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>  {
>  	enum drm_lspcon_mode current_mode;
> -- 
> 1.9.1

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