Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Daniel Vetter <daniel@ffwll.ch>]

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