Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On HSW+, one encoder (DDI) can have multiple connectors (HDMI and DP).
> If no connector is connected, we consider the encoder type to be
> INTEL_OUTPUT_UNKNOWN. The problem is that we allow user space to set
> modes on disconnected connectors, so when we try to set a mode on an
> INTEL_OUTPUT_UNKNOWN connector, we don't know what to do and end up
> printing a WARN. So on this patch, we introduce
> pipe_config->ddi_personality to help with that.
> 
> When the user space sets a mode on a connector, we check the connector
> type and match it against intel_encoder->type and set the DDI
> personality accordingly. Then, after the compute config stage is over,
> we properly assign the personality to intel_encoder->type.
> 
> This patch was previously called "drm/i915: Set the digital port
> encoder personality during modeset".
> 
> References: http://patchwork.freedesktop.org/patch/17838/
> Testcase: igt/kms_setmode/clone-exclusive-crtc
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
> Credits-to: Damien Lespiau <damien.lespiau@intel.com> (for the
> previous versions of the patch).
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 97 ++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  2 +
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++
>  3 files changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cb5367c..5cdc2f4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1557,18 +1557,109 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
>  	intel_dp_encoder_destroy(encoder);
>  }
>  
> +static bool intel_ddi_set_personality(struct intel_encoder *encoder,
> +				      struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct intel_connector *connector;
> +	int connectors = 0;
> +	enum port port = intel_ddi_get_encoder_port(encoder);
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +
> +		if (connector->new_encoder != encoder)
> +			continue;
> +
> +		connectors++;
> +		if (WARN_ON(connectors > 1))
> +			return false;
> +
> +		switch (connector->base.connector_type) {
> +		case DRM_MODE_CONNECTOR_HDMIA:
> +		case DRM_MODE_CONNECTOR_HDMIB:
> +			pipe_config->ddi_personality = INTEL_OUTPUT_HDMI;
> +			break;
> +		case DRM_MODE_CONNECTOR_DisplayPort:
> +			pipe_config->ddi_personality = INTEL_OUTPUT_DISPLAYPORT;
> +			break;
> +		case DRM_MODE_CONNECTOR_eDP:
> +			pipe_config->ddi_personality = INTEL_OUTPUT_EDP;
> +			break;
> +		default:
> +			WARN(1, "DRM connector type %d\n",
> +			     connector->base.connector_type);
> +			return false;
> +		}
> +
> +		if (encoder->type == pipe_config->ddi_personality)
> +			continue;
> +
> +		/* We expect eDP to always have encoder->type correctly set, so
> +		 * it shouldn't reach this point. */
> +		if (pipe_config->ddi_personality == INTEL_OUTPUT_EDP) {
> +			DRM_ERROR("DDI %c, type %s marked as eDP\n",
> +				   port_name(port),
> +				   intel_output_name(encoder->type));
> +			return false;
> +		}
> +
> +		/*
> +		 * We can't change the DDI type if we already have a connected
> +		 * device on this port. The first time a DDI is used though
> +		 * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
> +		 * connector to be connected (either trought the kernel command
> +		 * line or KMS) we need to comply.
> +		 */
> +		 if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
> +		     connector->base.status == connector_status_connected) {
> +			 DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
> +			 		port_name(port),
> +					intel_output_name(encoder->type),
> +					intel_output_name(pipe_config->ddi_personality));
> +			 return false;
> +		}

I think this part is better done with Ville's more general "do we have
both hdmi and dp on the same dig_port?" check. Care to review Ville's
patch instead? Thomas Wood is signed up for it on the review board but I
guess you can steal that task.

> +
> +		DRM_DEBUG_KMS("Setting DDI %c personality to %s\n",
> +			      port_name(port),
> +			      intel_output_name(pipe_config->ddi_personality));
> +	}
> +
> +	return true;
> +
> +}
> +
> +void intel_ddi_commit_personality(struct intel_crtc *crtc)
> +{
> +	struct intel_encoder *encoder = intel_ddi_get_crtc_encoder(&crtc->base);
> +
> +	switch (encoder->type) {
> +	case INTEL_OUTPUT_HDMI:
> +	case INTEL_OUTPUT_DISPLAYPORT:
> +	case INTEL_OUTPUT_EDP:
> +	case INTEL_OUTPUT_UNKNOWN:
> +		encoder->type = crtc->config.ddi_personality;
> +		break;
> +	case INTEL_OUTPUT_ANALOG:
> +		break;
> +	default:
> +		WARN(1, "Output type %s\n", intel_output_name(encoder->type));
> +		break;
> +	}
> +}
> +
>  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  				     struct intel_crtc_config *pipe_config)
>  {
> -	int type = encoder->type;
>  	int port = intel_ddi_get_encoder_port(encoder);
>  
> -	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> +	if (!intel_ddi_set_personality(encoder, pipe_config))
> +		return false;
>  
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> -	if (type == INTEL_OUTPUT_HDMI)
> +	if (pipe_config->ddi_personality == INTEL_OUTPUT_HDMI)
>  		return intel_hdmi_compute_config(encoder, pipe_config);
>  	else
>  		return intel_dp_compute_config(encoder, pipe_config);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5dbc88..16750c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7810,6 +7810,8 @@ static int haswell_crtc_mode_set(struct intel_crtc *crtc,
>  				 int x, int y,
>  				 struct drm_framebuffer *fb)
>  {
> +	intel_ddi_commit_personality(crtc);

This will conflict with Ander's in-flight patches to completely remove the
modeset callback. But I'm not really sure

Also I'm not sure whether we should keep updating encoder->type now that
we have ddi_personality - tracking the same state in two places usually
leads to bugs. Imo it's better to switch all existing encoder->type checks
in the ddi code over to check config->ddi_personality. We might need a
prep patch to also set the ddi_personality to FDI for the vga output on
hsw. Thinking about this, a separate enum might look pretty for this. Or
just match the bitfield enum of the register.

Also I think we should have state readout support for ddi_personality just
for paranoia.
-Daniel

> +
>  	if (!intel_ddi_pll_select(crtc))
>  		return -EINVAL;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5ab813c..bf3267c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -386,6 +386,9 @@ struct intel_crtc_config {
>  
>  	bool dp_encoder_is_mst;
>  	int pbn;
> +
> +	/* This should be INTEL_OUTPUT_*. */
> +	int ddi_personality;
>  };
>  
>  struct intel_pipe_wm {
> @@ -817,6 +820,7 @@ void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
>  void intel_ddi_clock_get(struct intel_encoder *encoder,
>  			 struct intel_crtc_config *pipe_config);
>  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> +void intel_ddi_commit_personality(struct intel_crtc *crtc);
>  
>  /* intel_frontbuffer.c */
>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> -- 
> 2.1.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx