| Message ID | 1414439272-1885-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@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@intel.com> (for the > previous versions of the patch). > Cc: Damien Lespiau <damien.lespiau@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 >
2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@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@intel.com> (for the >> previous versions of the patch). >> Cc: Damien Lespiau <damien.lespiau@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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_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. > 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... > -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
2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@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@intel.com> (for the >> previous versions of the patch). >> Cc: Damien Lespiau <damien.lespiau@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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. 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
On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote: > 2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@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@intel.com> (for the > >> previous versions of the patch). > >> Cc: Damien Lespiau <damien.lespiau@intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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. > > 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. Should we even care about that issue? Forcing a HDMI mode on a port even when there's a DP monitor plugged in does make it easier to test things without having to yank out the cables all the time. Also your patch wouldn't fix that problem for non-ddi platforms. I would suggest that we should allow this behaviour unless there's some risk of causing problems to the sink. Another reason for rejecting it would be if aux would stop working, but I don't think that should be the case since we can do both gmbus and aux during probing anyway. > > > > >> + > >> + 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 28, 2014 at 04:30:41PM +0200, Ville Syrjälä wrote: > On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote: > > >> + /* > > >> + * 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. My comment was _only_ about the check quoted above, the other part of the loop is imo the entire point of adding ddi_personality. > Should we even care about that issue? Forcing a HDMI mode on a port even > when there's a DP monitor plugged in does make it easier to test things > without having to yank out the cables all the time. Also your patch > wouldn't fix that problem for non-ddi platforms. > > I would suggest that we should allow this behaviour unless there's some > risk of causing problems to the sink. Another reason for rejecting it > would be if aux would stop working, but I don't think that should be the > case since we can do both gmbus and aux during probing anyway. This is useful for injection arbitrary modeset configs, something Thomas Wood is slowly chipping away on. And I think we definitely want that for increased automated test coverage. -Daniel
On Mon, Nov 03, 2014 at 01:15:24PM +0100, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 04:30:41PM +0200, Ville Syrjälä wrote: > > On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote: > > > >> + /* > > > >> + * 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. > > My comment was _only_ about the check quoted above, the other part of the > loop is imo the entire point of adding ddi_personality. Ok, misunderstood what this does - I've thought it's just checking that we don't use both hdmi and DP on the same ddi encoder. Imo that's all we need to check really. -Daniel
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; + } + + 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); + 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,