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. Ville's patch solves a different problem. I just reviewed it, but we still need the check above. The code above is in case, for example, there's a DP connector actually connected (but without a mode set), and then the user tries to set a mode on the HDMI connector of this encoder. > >> + >> + 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 -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
