Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

I thought about this some more, I think what we need to fix this mess
properly is:
- mode_valid helper callbacks for crtc, encoder, bridges, with the
same interface as for connectors.
- calling all these new mode_valid hooks from the probe helpers, but
with the restriction that we only reject a mode if all possible
crtc/encoders combos reject it. We need to filter by
possible_encoders/crtcs for these checks. Bridges have a fixed routing
to their encoder, so those are easy.
- add calls to mode_valid in the atomic helpers, right before we call
mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
- convert drivers to move code from mode_fixup into mode_valid
wherever possible, to make sure we can share as much of the check
logic between probe and atomic comit code.
- update docs for all the hooks, plus update the overview sections accordingly.

I think this should give us a good approximation of nirvana. For I
long time I thought we could get away without adding mode_valid
everywhere, but in the probe paths we really can't fake the
adjusted_mode (and other atomic state), so adding dedicated hooks
which are called from both places is probably the only option.
-Daniel

On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@xxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Xinliang Liu <xinliang.liu@xxxxxxxxxx>
> Cc: Xinliang Liu <z.liuxinliang@xxxxxxxxxxxxx>
> Cc: Rongrong Zou <zourongrong@xxxxxxxxx>
> Cc: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
> Cc: Chen Feng <puck.chen@xxxxxxxxxxxxx>
> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



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