[Intel-gfx] [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality
Paulo Zanoni
przanoni at gmail.com
Tue Oct 28 11:46:21 CET 2014
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.
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
More information about the Intel-gfx
mailing list