[Intel-gfx] [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality
Daniel Vetter
daniel at ffwll.ch
Tue Oct 28 08:49:15 CET 2014
On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at 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 at intel.com> (for the
> previous versions of the patch).
> Cc: Damien Lespiau <damien.lespiau at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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
More information about the Intel-gfx
mailing list