2013/12/3 Daniel Vetter <daniel@xxxxxxxx>: > On Tue, Dec 03, 2013 at 03:36:20PM -0200, Paulo Zanoni wrote: >> From: 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 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@xxxxxxxxx> >> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> --- >> 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. See "[PATCH] drm/i915: don't set modes for 2 connectors on the same encoder": http://lists.freedesktop.org/archives/intel-gfx/2013-November/036771.html . The implementation is based on something you posted to pastebin when we were discussing this problem a few months ago. Even with that patch applied, I think we should keep this current path as-is since it runs before the code that checks for 2 encoders. > > 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
