[Intel-gfx] [PATCH 3/3 v4] drm/i915: Set the digital port encoder personality during modeset
Daniel Vetter
daniel at ffwll.ch
Tue Jan 14 10:02:38 CET 2014
On Mon, Jan 13, 2014 at 06:03:01PM +0000, Damien Lespiau wrote:
> 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
> v3: Made by Paulo:
> - WARN in case "connectors > 1", since we have a patch that should
> catch this case earlier
> v4: Authorize unknown encoders with a "connected" connector to change
> personality (Damien)
>
> Testcase: igt/kms_setmode/clone-exclusive-crtc
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
> Tested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 102 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 99 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 74749c6..79776f8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1518,18 +1518,114 @@ 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 (WARN_ON(connectors > 1))
> + return false;
> +
> + 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;
> + }
> +
> + /*
> + * 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 (old_encoder_type != INTEL_OUTPUT_UNKNOWN &&
> + 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);
Nope, you can't change encoder->type in ->compute_config. Otherwise if we
run the atomic modeset stuff in the "check-only" mode this could wreak
utter havoc, and the goal is that userspace will fully enumerate all
configs using that. So fallout will happen. So we can only commit the
new encoder personality to anything outside of the staging state in
->mode_set.
There's two possible approaches that could work:
- Do a generic encoder personality thing which uses the linked up
connector in the new_ staging pointers to do all the heavy lifting in
the compute_config stage. Bit of work, especially since the sdvo output
doesn't need all that much state really. So feels a bit like overkill
just for ddi.
- Add a dynamic intel_ddi_staged_personality functions which walks the
new_ pointers to figure out which personality to use in
->compute_config. We'd do the same for sdvo then (which uses bitmasks
for that purpose). A bit fragile since then we need to carefully review
every place and decide whether to use this or the old encoder->type.
- Add a ddi_personality field to the pipe config which you set once in
ddi_compute_config and then use everywhere else. The generic pipe config
update code will take care of committing the value. encoder->type would
die.
Personally I'm leaning towards the last approach, but the others should
also work out. Another reason for this approach is that state
cross-checking (which we imo should have) naturally falls out by placing
the ddi personality into the pipe config - we only need to wire up a
little bit of code.
For the first approach we'd need to add a linked_connector out parameter
to encoder->get_config, which will add a bit of ugly code to the core
checker. And for the 2nd approach I don't really have a good idea how to
cross-check it.
Sorry that I didn't jump into this discussion earlier.
Cheers, Daniel
> + 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 at 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
More information about the Intel-gfx
mailing list