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. > + > + 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
