[Intel-gfx] [PATCH 2/2] drm/i915: Set the digital port encoder personality during modeset

[Paulo Zanoni]

2013/11/29 Damien Lespiau <damien.lespiau at intel.com>:
> The ->detect() vfunc of connectors is usually responsible for setting the
> encoder type on intel_digital_ports when a hotplug event happens.
>
> However we sometimes want to force a modeset on a specific connector:
>   - the user can ask the SETCRTC ioctl to use a connector that wasne
>     marked as connected (because we never received a hotplug event for it).
>     This can be also used in tests to do a modeset without the need of a
>     plugged-in monitor.
>   - the command line video= option can be used to force modesets, eg.:
>     video=HDMI-A-1:1024x768e
>
> So, before we try to do anything with the DDI encoder as part of a modeset,
> we need to ensure that the personality of the encoder matches the selected
> connector.

Needs bugzilla references.


>
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 85 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c8382f5..3ed2e3c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1442,11 +1442,96 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
>         intel_dp_encoder_destroy(encoder);
>  }
>
> +/*
> + * The ->detect() vfunc of connectors is usually responsible for setting the
> + * encoder type on intel_digital_ports when a hotplug event happens.
> + *
> + * However we sometimes want to force a modeset on a specific connector:
> + *   - the user can ask the SETCRTC ioctl to use a connector that isn't marked
> + *     as connected (because we never received a hotplug event for it).
> + *     This can be also used in tests to do a modeset without the need of a
> + *     plugged-in monitor.
> + *   - the command line video= option can be used to force modesets, eg.:
> + *     video=HDMI-A-1:1024x768e
> + *
> + * So, before we try to do anything with the DDI encoder as part of a modeset,
> + * we need to ensure that the personality of the encoder matches the selected
> + * connector.
> + */
> +static bool
> +intel_ddi_ensure_encoder_type(struct intel_encoder *encoder)
> +{
> +       struct drm_device *dev = encoder->base.dev;
> +       struct intel_connector *connector;
> +
> +       list_for_each_entry(connector, &dev->mode_config.connector_list,
> +                           base.head) {
> +               int connector_type, old_encoder_type, new_encoder_type;
> +               int port;
> +
> +               if (connector->new_encoder != encoder)
> +                       continue;

I'd add a check for 2 connectors on the same encoder here.


> +
> +               connector_type = connector->base.connector_type;
> +               old_encoder_type = encoder->type;
> +               switch (connector_type) {
> +               case DRM_MODE_CONNECTOR_HDMIA:
> +               case DRM_MODE_CONNECTOR_HDMIB:
> +                       new_encoder_type = INTEL_OUTPUT_HDMI;
> +                       break;
> +               case DRM_MODE_CONNECTOR_DisplayPort:
> +                       new_encoder_type = INTEL_OUTPUT_DISPLAYPORT;
> +                       break;
> +               case DRM_MODE_CONNECTOR_eDP:
> +                       new_encoder_type = INTEL_OUTPUT_EDP;

I don't see a reason for this assignment, since we're "continue"ing here.


> +                       continue;
> +               default:
> +                       continue;
> +               }
> +
> +               if (old_encoder_type == new_encoder_type)
> +                       continue;
> +
> +               port = intel_ddi_get_encoder_port(encoder);
> +
> +               if (old_encoder_type == INTEL_OUTPUT_EDP) {
> +                       DRM_ERROR("Can't change DDI %c personality to %s, "
> +                                 "it's an eDP DDI\n",

I think some people would probably complain about the broken string here.


> +                                 port_name(port),
> +                                 intel_output_name(new_encoder_type));
> +                       return false;
> +               }
> +
> +               if (connector->base.status == connector_status_connected) {
> +                       DRM_ERROR("Can't change DDI %c personality to %s, "
> +                                 "it has a connected %s device\n",
> +                                 port_name(port),
> +                                 intel_output_name(new_encoder_type),
> +                                 intel_output_name(old_encoder_type));

I guess this means: "your user-space is just insane". I'm not sure if
we want to DRM_ERROR here. Maybe just DRM_DEBUG_KMS, otherwise we'll
get bug reports that we can't fix in the Kernel.


> +                       return false;
> +               }
> +
> +               DRM_DEBUG_KMS("Changing DDI %c from %s to %s\n",
> +                             port_name(port),
> +                             intel_output_name(old_encoder_type),
> +                             intel_output_name(new_encoder_type));
> +
> +               connector->new_encoder->type = new_encoder_type;
> +       }
> +
> +       return true;
> +}
> +
>  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);
> +       int ret;
> +
> +       ret = intel_ddi_ensure_encoder_type(encoder);
> +       if (!ret)
> +               return false;
>

You'll still hit the warn here, since "type" was assigned before you
called intel_ddi_ensure_encoder_type.


I took your patch, applied it, fixed the things I pointed above,
tested, and also patched IGT to fix the remaining issue. I'll send the
new version implementing the changes I suggested.


>         WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni