On Tue, Oct 28, 2014 at 08:46:21AM -0200, Paulo Zanoni wrote: > 2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@xxxxxxxx>: > > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >> > >> 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@xxxxxxxxx> (for the > >> previous versions of the patch). > >> Cc: Damien Lespiau <damien.lespiau@xxxxxxxxx> > >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >> --- > >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
