Message-ID: <CAKMK7uED5JQG3Kv1O1zdhrpQ=0S5+HSZVeb1enzeaG7JnVi_8g@mail.gmail.com>
Date:   Tue, 14 Feb 2017 20:38:40 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Chen Feng <puck.chen@...ilicon.com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Rongrong Zou <zourongrong@...il.com>,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@...aro.org> wrote:
> Currently, on the hikey board, we have the adv7511 bridge wired
> up to the kirin ade drm driver. Unfortunately, the kirin ade
> core cannot generate accurate byteclocks for all pixel clock
> values.
>
> Thus if a mode clock is selected that we cannot calculate a
> matching byteclock, the device will boot with a blank screen.
>
> Unfortunately, currently the only place we can properly check
> potential modes for this issue in the connector mode_valid
> helper. Again, hikey uses the adv7511 bridge, which is shared
> between a number of different devices, so its improper to put
> restrictions caused by the kirin drm driver in the adv7511
> logic.
>
> So this patch tries to correct for that, by adding some
> infrastructure so that the drm_crtc_helper_funcs can optionally
> implement a mode_valid check, so that the probe helpers can
> check to make sure there are not any restrictions at the crtc
> level as well.
>
> Cc: Daniel Vetter <daniel.vetter@...el.com>
> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
> Cc: Sean Paul <seanpaul@...omium.org>
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Rob Clark <robdclark@...il.com>
> Cc: Xinliang Liu <xinliang.liu@...aro.org>
> Cc: Xinliang Liu <z.liuxinliang@...ilicon.com>
> Cc: Rongrong Zou <zourongrong@...il.com>
> Cc: Xinwei Kong <kong.kongxinwei@...ilicon.com>
> Cc: Chen Feng <puck.chen@...ilicon.com>
> Cc: Archit Taneja <architt@...eaurora.org>
> Cc: dri-devel@...ts.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@...aro.org>

So I'm going to be super-annoying here and ask for a complete
solution. This here is defacto what ever driver already does (or has
too), but it doesn't really solve the overall issue of having entirely
separate validation paths for probe and atomic_check paths. I think if
we wan to solve this, we need to solve this properly, with a generic
solution. That would mean:
- still in helpers, to make it all opt-int
- covers crtc and encoders and bridges
- allows you to implement the current mode_valid in terms of the new
stuff (maybe as a default hook)
- allows you to implement the current assortment of mode_fixup and/or
atomic_check in terms of the new stuff, or at least to not have to
duplicate logic in there

Docs for all this, especially updating all the warnings on how to use
the existing hooks correctly.

I think just pushing this specific case into the helpers, without
rethinking the overall big picture, isn't gaining us all that much.
For just this I'd say just put it into drivers, until we have some
good clue about how the helpers should look like (maybe this time is
the time? ...).

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index cf8f012..a808348 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
>                 connector_status_connected;
>  }
>
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}
> +
>  /**
>   * drm_helper_probe_single_connector_modes - get complete set of display modes
>   * @connector: connector to probe
> @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 if (mode->status == MODE_OK && connector_funcs->mode_valid)
>                         mode->status = connector_funcs->mode_valid(connector,
>                                                                    mode);
> +
> +               mode->status = drm_connector_check_crtc_modes(connector, mode);
>         }
>
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 69c3974..53ca0e4 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
>         void (*commit)(struct drm_crtc *crtc);
>
>         /**
> +        * @mode_valid:
> +        *
> +        * Callback to validate a mode for a crtc, irrespective of the
> +        * specific display configuration.
> +        *
> +        * This callback is used by the probe helpers to filter the mode list
> +        * (which is usually derived from the EDID data block from the sink).
> +        * See e.g. drm_helper_probe_single_connector_modes().
> +        *
> +        * NOTE:
> +        *
> +        * This only filters the mode list supplied to userspace in the
> +        * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +        * ask the kernel to use them. It this case the atomic helpers or legacy
> +        * CRTC helpers will not call this function. Drivers therefore must
> +        * still fully validate any mode passed in in a modeset request.
> +        *
> +        * RETURNS:
> +        *
> +        * Either MODE_OK or one of the failure reasons in enum
> +        * &drm_mode_status.
> +        */
> +       enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +                                          struct drm_display_mode *mode);
> +
> +       /**
>          * @mode_fixup:
>          *
>          * This callback is used to validate a mode. The parameter mode is the
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Powered by blists - more mailing lists