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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
