2013/11/29 Damien Lespiau <damien.lespiau@xxxxxxxxx>:
> 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 wasne
> 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.
Needs bugzilla references.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 85 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c8382f5..3ed2e3c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1442,11 +1442,96 @@ 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;
> +
> + 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;
I'd add a check for 2 connectors on the same encoder here.
> +
> + 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:
> + new_encoder_type = INTEL_OUTPUT_EDP;
I don't see a reason for this assignment, since we're "continue"ing here.
> + continue;
> + default:
> + 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",
I think some people would probably complain about the broken string here.
> + port_name(port),
> + intel_output_name(new_encoder_type));
> + return false;
> + }
> +
> + if (connector->base.status == connector_status_connected) {
> + DRM_ERROR("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));
I guess this means: "your user-space is just insane". I'm not sure if
we want to DRM_ERROR here. Maybe just DRM_DEBUG_KMS, otherwise we'll
get bug reports that we can't fix in the Kernel.
> + 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));
> +
> + connector->new_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;
>
You'll still hit the warn here, since "type" was assigned before you
called intel_ddi_ensure_encoder_type.
I took your patch, applied it, fixed the things I pointed above,
tested, and also patched IGT to fix the remaining issue. I'll send the
new version implementing the changes I suggested.
> WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
