[Intel-gfx] [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality
Daniel Vetter
daniel at ffwll.ch
Tue Oct 28 12:20:46 CET 2014
On Tue, Oct 28, 2014 at 08:46:21AM -0200, Paulo Zanoni wrote:
> 2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> > 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.
>
> Which patch? Please tell me the email subject, or patchwork link.
drm/i915: Reject modeset when the same digital port is used more than once
> >
> >> +
> >> + 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.
> >
>
> The problem is that intel_display.c also has checks for encoder
> types... I'm not 100% sure, but I think we just can't go with just
> ddi_personality.
On a quick check I've spotted just one that would apply to hsw/bdw, and
that can be replaced with a check to config->has_dp_encoder. We've come a
long way with properly tracking all the insane details in the pipe config,
at least on modern platforms ;-) vlv/gmch platforms are a different beast.
>
>
> > Also I think we should have state readout support for ddi_personality just
> > for paranoia.
>
> And how exactly would you implement that? Doesn't make too much sense for me...
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2688bc940879..3ee83464bb83 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1511,11 +1511,15 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
case TRANS_DDI_MODE_SELECT_HDMI:
pipe_config->has_hdmi_sink = true;
case TRANS_DDI_MODE_SELECT_DVI:
+ pipe_config->ddi_personality = DDI_HDMI;
+ break;
case TRANS_DDI_MODE_SELECT_FDI:
+ pipe_config->ddi_personality = DDI_FDI;
break;
case TRANS_DDI_MODE_SELECT_DP_SST:
case TRANS_DDI_MODE_SELECT_DP_MST:
pipe_config->has_dp_encoder = true;
+ pipe_config->ddi_personality = DDI_DP;
intel_dp_get_m_n(intel_crtc, pipe_config);
break;
default:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ee982f5412d6..481a05622f24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10322,6 +10322,7 @@ intel_pipe_config_compare(struct drm_device *dev,
PIPE_CONF_CHECK_I(double_wide);
PIPE_CONF_CHECK_X(ddi_pll_sel);
+ PIPE_CONF_CHECK_I(ddi_personality);
PIPE_CONF_CHECK_I(shared_dpll);
PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list