* [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality @ 2014-10-27 19:47 Paulo Zanoni 2014-10-27 19:47 ` [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum Paulo Zanoni 2014-10-28 7:49 ` [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Daniel Vetter 0 siblings, 2 replies; 10+ messages in thread From: Paulo Zanoni @ 2014-10-27 19:47 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, Paulo Zanoni 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; + } + + 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, -- 2.1.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum 2014-10-27 19:47 [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Paulo Zanoni @ 2014-10-27 19:47 ` Paulo Zanoni 2014-10-28 7:55 ` Daniel Vetter 2014-10-28 7:49 ` [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Daniel Vetter 1 sibling, 1 reply; 10+ messages in thread From: Paulo Zanoni @ 2014-10-27 19:47 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni From: Paulo Zanoni <paulo.r.zanoni@intel.com> Because I got annoyed that I had to document what values "int ddi_personality" is supposed to hold. A good side-effect of this change is that now the compilers can do some additional checks on our code, which may prevent some bugs in the future. A bad side-effect of this change is that now the compilers do some additional checks on our code and complain when a switch statement doesn't check for all possible values, so we need to add "default" cases to all those switches. Hopefully, this may help preventing confusions against DRM_MODE_CONNECTOR_* and DRM_MODE_ENCODER_*. I guess that just by looking at the patch, some people will think this change is not worth its benefits. In this case, I don't really mind dropping the patch. Also, there's probably still a few more places where we can s/int/enum intel_output_type/, but we can change that later, when we spot the places. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_drv.h | 31 ++++++++++++++++--------------- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 61ea8da..a79f83c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2971,6 +2971,8 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe, break; } break; + default: + break; } } drm_modeset_unlock_all(dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 16750c4..83b8771 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6269,6 +6269,8 @@ static int i9xx_crtc_mode_set(struct intel_crtc *crtc, case INTEL_OUTPUT_DSI: is_dsi = true; break; + default: + break; } num_connectors++; @@ -6597,6 +6599,8 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) if (enc_to_dig_port(&encoder->base)->port == PORT_A) has_cpu_edp = true; break; + default: + break; } } @@ -6901,6 +6905,8 @@ static void lpt_init_pch_refclk(struct drm_device *dev) case INTEL_OUTPUT_ANALOG: has_vga = true; break; + default: + break; } } @@ -6934,6 +6940,8 @@ static int ironlake_get_refclk(struct drm_crtc *crtc) case INTEL_OUTPUT_LVDS: is_lvds = true; break; + default: + break; } num_connectors++; } @@ -7188,6 +7196,8 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, case INTEL_OUTPUT_HDMI: is_sdvo = true; break; + default: + break; } num_connectors++; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index bf3267c..deb1bc6 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -94,18 +94,20 @@ /* these are outputs from the chip - integrated only external chips are via DVO or SDVO output */ -#define INTEL_OUTPUT_UNUSED 0 -#define INTEL_OUTPUT_ANALOG 1 -#define INTEL_OUTPUT_DVO 2 -#define INTEL_OUTPUT_SDVO 3 -#define INTEL_OUTPUT_LVDS 4 -#define INTEL_OUTPUT_TVOUT 5 -#define INTEL_OUTPUT_HDMI 6 -#define INTEL_OUTPUT_DISPLAYPORT 7 -#define INTEL_OUTPUT_EDP 8 -#define INTEL_OUTPUT_DSI 9 -#define INTEL_OUTPUT_UNKNOWN 10 -#define INTEL_OUTPUT_DP_MST 11 +enum intel_output_type { + INTEL_OUTPUT_UNUSED = 0, + INTEL_OUTPUT_ANALOG = 1, + INTEL_OUTPUT_DVO = 2, + INTEL_OUTPUT_SDVO = 3, + INTEL_OUTPUT_LVDS = 4, + INTEL_OUTPUT_TVOUT = 5, + INTEL_OUTPUT_HDMI = 6, + INTEL_OUTPUT_DISPLAYPORT = 7, + INTEL_OUTPUT_EDP = 8, + INTEL_OUTPUT_DSI = 9, + INTEL_OUTPUT_UNKNOWN = 10, + INTEL_OUTPUT_DP_MST = 11, +}; #define INTEL_DVO_CHIP_NONE 0 #define INTEL_DVO_CHIP_LVDS 1 @@ -136,7 +138,7 @@ struct intel_encoder { */ struct intel_crtc *new_crtc; - int type; + enum intel_output_type type; unsigned int cloneable; bool connectors_active; void (*hot_plug)(struct intel_encoder *); @@ -387,8 +389,7 @@ struct intel_crtc_config { bool dp_encoder_is_mst; int pbn; - /* This should be INTEL_OUTPUT_*. */ - int ddi_personality; + enum intel_output_type ddi_personality; }; struct intel_pipe_wm { -- 2.1.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum 2014-10-27 19:47 ` [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum Paulo Zanoni @ 2014-10-28 7:55 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-10-28 7:55 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni On Mon, Oct 27, 2014 at 05:47:52PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Because I got annoyed that I had to document what values "int > ddi_personality" is supposed to hold. > > A good side-effect of this change is that now the compilers can do > some additional checks on our code, which may prevent some bugs in the > future. A bad side-effect of this change is that now the compilers do > some additional checks on our code and complain when a switch > statement doesn't check for all possible values, so we need to add > "default" cases to all those switches. Hopefully, this may help > preventing confusions against DRM_MODE_CONNECTOR_* and > DRM_MODE_ENCODER_*. > > I guess that just by looking at the patch, some people will think this > change is not worth its benefits. In this case, I don't really mind > dropping the patch. > > Also, there's probably still a few more places where we can > s/int/enum intel_output_type/, but we can change that later, when we > spot the places. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> I like this, so went right ahead and applied it and resolved the little rebase conflict. -Daniel > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ > drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 31 ++++++++++++++++--------------- > 3 files changed, 28 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 61ea8da..a79f83c 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2971,6 +2971,8 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe, > break; > } > break; > + default: > + break; > } > } > drm_modeset_unlock_all(dev); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 16750c4..83b8771 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6269,6 +6269,8 @@ static int i9xx_crtc_mode_set(struct intel_crtc *crtc, > case INTEL_OUTPUT_DSI: > is_dsi = true; > break; > + default: > + break; > } > > num_connectors++; > @@ -6597,6 +6599,8 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) > if (enc_to_dig_port(&encoder->base)->port == PORT_A) > has_cpu_edp = true; > break; > + default: > + break; > } > } > > @@ -6901,6 +6905,8 @@ static void lpt_init_pch_refclk(struct drm_device *dev) > case INTEL_OUTPUT_ANALOG: > has_vga = true; > break; > + default: > + break; > } > } > > @@ -6934,6 +6940,8 @@ static int ironlake_get_refclk(struct drm_crtc *crtc) > case INTEL_OUTPUT_LVDS: > is_lvds = true; > break; > + default: > + break; > } > num_connectors++; > } > @@ -7188,6 +7196,8 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > case INTEL_OUTPUT_HDMI: > is_sdvo = true; > break; > + default: > + break; > } > > num_connectors++; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index bf3267c..deb1bc6 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -94,18 +94,20 @@ > > /* these are outputs from the chip - integrated only > external chips are via DVO or SDVO output */ > -#define INTEL_OUTPUT_UNUSED 0 > -#define INTEL_OUTPUT_ANALOG 1 > -#define INTEL_OUTPUT_DVO 2 > -#define INTEL_OUTPUT_SDVO 3 > -#define INTEL_OUTPUT_LVDS 4 > -#define INTEL_OUTPUT_TVOUT 5 > -#define INTEL_OUTPUT_HDMI 6 > -#define INTEL_OUTPUT_DISPLAYPORT 7 > -#define INTEL_OUTPUT_EDP 8 > -#define INTEL_OUTPUT_DSI 9 > -#define INTEL_OUTPUT_UNKNOWN 10 > -#define INTEL_OUTPUT_DP_MST 11 > +enum intel_output_type { > + INTEL_OUTPUT_UNUSED = 0, > + INTEL_OUTPUT_ANALOG = 1, > + INTEL_OUTPUT_DVO = 2, > + INTEL_OUTPUT_SDVO = 3, > + INTEL_OUTPUT_LVDS = 4, > + INTEL_OUTPUT_TVOUT = 5, > + INTEL_OUTPUT_HDMI = 6, > + INTEL_OUTPUT_DISPLAYPORT = 7, > + INTEL_OUTPUT_EDP = 8, > + INTEL_OUTPUT_DSI = 9, > + INTEL_OUTPUT_UNKNOWN = 10, > + INTEL_OUTPUT_DP_MST = 11, > +}; > > #define INTEL_DVO_CHIP_NONE 0 > #define INTEL_DVO_CHIP_LVDS 1 > @@ -136,7 +138,7 @@ struct intel_encoder { > */ > struct intel_crtc *new_crtc; > > - int type; > + enum intel_output_type type; > unsigned int cloneable; > bool connectors_active; > void (*hot_plug)(struct intel_encoder *); > @@ -387,8 +389,7 @@ struct intel_crtc_config { > bool dp_encoder_is_mst; > int pbn; > > - /* This should be INTEL_OUTPUT_*. */ > - int ddi_personality; > + enum intel_output_type ddi_personality; > }; > > struct intel_pipe_wm { > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-10-27 19:47 [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Paulo Zanoni 2014-10-27 19:47 ` [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum Paulo Zanoni @ 2014-10-28 7:49 ` Daniel Vetter 2014-10-28 10:46 ` Paulo Zanoni 2014-10-28 13:26 ` Paulo Zanoni 1 sibling, 2 replies; 10+ messages in thread From: Daniel Vetter @ 2014-10-28 7:49 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, intel-gfx, Paulo Zanoni 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 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-10-28 7:49 ` [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Daniel Vetter @ 2014-10-28 10:46 ` Paulo Zanoni 2014-10-28 11:20 ` Daniel Vetter 2014-10-28 13:26 ` Paulo Zanoni 1 sibling, 1 reply; 10+ messages in thread From: Paulo Zanoni @ 2014-10-28 10:46 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni 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 -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-10-28 10:46 ` Paulo Zanoni @ 2014-10-28 11:20 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-10-28 11:20 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni On Tue, Oct 28, 2014 at 08:46:21AM -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. > > 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-10-28 7:49 ` [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Daniel Vetter 2014-10-28 10:46 ` Paulo Zanoni @ 2014-10-28 13:26 ` Paulo Zanoni 2014-10-28 14:30 ` Ville Syrjälä 1 sibling, 1 reply; 10+ messages in thread From: Paulo Zanoni @ 2014-10-28 13:26 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni 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 -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-10-28 13:26 ` Paulo Zanoni @ 2014-10-28 14:30 ` Ville Syrjälä 2014-11-03 12:15 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Ville Syrjälä @ 2014-10-28 14:30 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni 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 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-10-28 14:30 ` Ville Syrjälä @ 2014-11-03 12:15 ` Daniel Vetter 2014-11-03 12:36 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2014-11-03 12:15 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality 2014-11-03 12:15 ` Daniel Vetter @ 2014-11-03 12:36 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-11-03 12:36 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-11-03 12:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-27 19:47 [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Paulo Zanoni 2014-10-27 19:47 ` [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum Paulo Zanoni 2014-10-28 7:55 ` Daniel Vetter 2014-10-28 7:49 ` [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality Daniel Vetter 2014-10-28 10:46 ` Paulo Zanoni 2014-10-28 11:20 ` Daniel Vetter 2014-10-28 13:26 ` Paulo Zanoni 2014-10-28 14:30 ` Ville Syrjälä 2014-11-03 12:15 ` Daniel Vetter 2014-11-03 12:36 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox