Re: [PATCH 2/2] drm/i915: Set the digital port encoder personality during modeset

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 03, 2013 at 03:36:20PM -0200, Paulo Zanoni wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> The ->detect() vfunc of connectors is usually responsible for setting the
> encoder type on intel_digital_ports when a hotplug event happens.
> 
> However we sometimes want to force a modeset on a specific connector:
>   - the user can ask the SETCRTC ioctl to use a connector that isn't
>     marked as connected (because we never received a hotplug event for it).
>     This can be also used in tests to do a modeset without the need of a
>     plugged-in monitor.
>   - the command line video= option can be used to force modesets, eg.:
>     video=HDMI-A-1:1024x768e
> 
> So, before we try to do anything with the DDI encoder as part of a modeset,
> we need to ensure that the personality of the encoder matches the selected
> connector.
> 
> v2: Made by Paulo:
>   - Return false if we have more than one connector.
>   - WARN in case it's not eDP/DP/HDMI
>   - Don't break error strings in pieces
>   - Change the "status == connected" message from DRM_ERROR to
>     DRM_DEBUG_KMS
>   - Don't store+use the old encoder->type at compute_config
>   - Add bugzilla reference
>   - Add testcase reference
> 
> Testcase: igt/kms_setmode/clone-exclusive-crtc
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 96 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c8382f5..879fbe8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1442,18 +1442,108 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
>  	intel_dp_encoder_destroy(encoder);
>  }
>  
> +/*
> + * The ->detect() vfunc of connectors is usually responsible for setting the
> + * encoder type on intel_digital_ports when a hotplug event happens.
> + *
> + * However we sometimes want to force a modeset on a specific connector:
> + *   - the user can ask the SETCRTC ioctl to use a connector that isn't marked
> + *     as connected (because we never received a hotplug event for it).
> + *     This can be also used in tests to do a modeset without the need of a
> + *     plugged-in monitor.
> + *   - the command line video= option can be used to force modesets, eg.:
> + *     video=HDMI-A-1:1024x768e
> + *
> + * So, before we try to do anything with the DDI encoder as part of a modeset,
> + * we need to ensure that the personality of the encoder matches the selected
> + * connector.
> + */
> +static bool
> +intel_ddi_ensure_encoder_type(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct intel_connector *connector;
> +	int connectors = 0;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +		int connector_type, old_encoder_type, new_encoder_type;
> +		int port;
> +
> +		if (connector->new_encoder != encoder)
> +			continue;
> +
> +		connectors++;
> +		if (connectors > 1) {
> +			DRM_DEBUG_KMS("Can't set modes for 2 connectors on the same DDI encoder\n");
> +			return false;
> +		}

This part should be moved into it's own patch and also moved into the
modeset core - sdvo encoders suffer from the same issue. Even better, if
we put that in the common code we can reject modeset attempts even before
we've started to kill the old config, which is always what we want.
Otherwise we'll never get to the great promised land where modesetting
configurations can be checked non-destructively with the atomic ioctl.

With change I'd simply call the function ddi_set_encoder_type or
something.

Cheers, Daniel
> +
> +		connector_type = connector->base.connector_type;
> +		old_encoder_type = encoder->type;
> +		switch (connector_type) {
> +		case DRM_MODE_CONNECTOR_HDMIA:
> +		case DRM_MODE_CONNECTOR_HDMIB:
> +			new_encoder_type = INTEL_OUTPUT_HDMI;
> +			break;
> +		case DRM_MODE_CONNECTOR_DisplayPort:
> +			new_encoder_type = INTEL_OUTPUT_DISPLAYPORT;
> +			break;
> +		case DRM_MODE_CONNECTOR_eDP:
> +			continue;
> +		default:
> +			WARN(1, "DRM connector type %d\n", connector_type);
> +			continue;
> +		}
> +
> +		if (old_encoder_type == new_encoder_type)
> +			continue;
> +
> +		port = intel_ddi_get_encoder_port(encoder);
> +
> +		if (old_encoder_type == INTEL_OUTPUT_EDP) {
> +			DRM_ERROR("Can't change DDI %c personality to %s, it's an eDP DDI\n",
> +				  port_name(port),
> +				  intel_output_name(new_encoder_type));
> +			return false;
> +		}
> +
> +		if (connector->base.status == connector_status_connected) {
> +			DRM_DEBUG_KMS("Can't change DDI %c personality to %s, it has a connected %s device\n",
> +				      port_name(port),
> +				      intel_output_name(new_encoder_type),
> +				      intel_output_name(old_encoder_type));
> +			return false;
> +		}
> +
> +		DRM_DEBUG_KMS("Changing DDI %c from %s to %s\n",
> +			      port_name(port),
> +			      intel_output_name(old_encoder_type),
> +			      intel_output_name(new_encoder_type));
> +
> +		encoder->type = new_encoder_type;
> +	}
> +
> +	return true;
> +}
> +
>  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);
> +	int ret;
> +
> +	ret = intel_ddi_ensure_encoder_type(encoder);
> +	if (!ret)
> +		return false;
>  
> -	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> +	WARN(encoder->type == INTEL_OUTPUT_UNKNOWN,
> +	     "compute_config() on unknown output!\n");
>  
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> -	if (type == INTEL_OUTPUT_HDMI)
> +	if (encoder->type == INTEL_OUTPUT_HDMI)
>  		return intel_hdmi_compute_config(encoder, pipe_config);
>  	else
>  		return intel_dp_compute_config(encoder, pipe_config);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch