[Intel-gfx] [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Oct 28 15:30:41 CET 2014
On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote:
> 2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at intel.com> (for the
> >> previous versions of the patch).
> >> Cc: Damien Lespiau <damien.lespiau at intel.com>
> >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list