| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170502084807.k7ioo7a5j33xtnpj@phenom.ffwll.local>
Date: Tue, 2 May 2017 10:48:07 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Jose Abreu <Jose.Abreu@...opsys.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
Ville Syrjälä
<ville.syrjala@...ux.intel.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Dave Airlie <airlied@...ux.ie>
Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback
On Wed, Apr 26, 2017 at 11:48:34AM +0100, Jose Abreu wrote:
> Some crtc's may have restrictions in the mode they can display. In
> this patch a new callback (crtc->mode_valid()) is introduced that
> is called at the same stage of connector->mode_valid() callback.
>
> This shall be implemented if the crtc has some sort of restriction
> so that we don't probe modes that will fail in the commit() stage.
> For example: A given crtc may be responsible to set a clock value.
> If the clock can not produce all the values for the available
> modes then this callback can be used to restrict the number of
> probbed modes to only the ones that can be displayed.
>
> If the crtc does not implement the callback then the behaviour will
> remain the same. Also, for a given set of crtcs that can be bound to
> the connector, if at least one can display the mode then the mode
> will be probbed.
>
> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
> Cc: Carlos Palminha <palminha@...opsys.com>
> Cc: Alexey Brodkin <abrodkin@...opsys.com>
> Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Cc: Dave Airlie <airlied@...ux.ie>
Not sure this is useful, since you still have to duplicate the exact same
check into your ->mode_fixup hook. That seems to make things even more
confusing.
Also this doesn't update the various kerneldoc comments. For the existing
hooks. Since this topic causes so much confusion, I don't think a
half-solution will help, but has some good potential to make things worse.
-Daniel
> ---
> drivers/gpu/drm/drm_probe_helper.c | 44 ++++++++++++++++++++++++++++++++
> include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++
> 2 files changed, 70 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..61eac30 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -80,6 +80,46 @@
> return MODE_OK;
> }
>
> +static enum drm_mode_status drm_mode_validate_connector_crtc(
> + struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
> + enum drm_mode_status mode_status = MODE_ERROR;
> + struct drm_device *dev = connector->dev;
> + struct drm_encoder *encoder;
> + struct drm_crtc *crtc;
> + bool callback_found = false;
> + int i;
> +
> + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> + encoder = drm_encoder_find(dev, connector->encoder_ids[i]);
> +
> + if (!encoder)
> + continue;
> +
> + drm_for_each_crtc(crtc, dev) {
> + crtc_funcs = crtc->helper_private;
> +
> + if (!drm_encoder_crtc_ok(encoder, crtc))
> + continue;
> + if (!crtc_funcs || !crtc_funcs->mode_valid)
> + continue;
> +
> + /* MODE_OK=0 and default mode_status=MODE_ERROR=-1
> + * so if at least one crtc accepts the mode we get
> + * MODE_OK */
> + mode_status &= crtc_funcs->mode_valid(crtc, mode);
> + callback_found |= true;
> + }
> + }
> +
> + /* We can reach here without calling mode_valid if there is no
> + * registered crtc with this callback, lets return MODE_OK in this
> + * case */
> + return callback_found ? mode_status : MODE_OK;
> +}
> +
> static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> {
> struct drm_cmdline_mode *cmdline_mode;
> @@ -431,6 +471,10 @@ 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);
> +
> + if (mode->status == MODE_OK)
> + mode->status = drm_mode_validate_connector_crtc(
> + connector, mode);
> }
>
> prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index c01c328..59fffba 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs {
> void (*commit)(struct drm_crtc *crtc);
>
> /**
> + * @mode_valid:
> + *
> + * This callback should be implemented if the crtc has some sort of
> + * restriction in the modes it can display. For example, a given crtc
> + * may be responsible to set a clock value. If the clock can not
> + * produce all the values for the available modes then this callback
> + * can be used to restrict the number of probbed modes to only the ones
> + * that can be displayed.
> + *
> + * This is directly called at the same stage of connector->mode_valid
&drm_connector_helper_funcs.mode_valid to make a proper link. Please also
check the output of make htmldocs and make sure it looks pretty.
> + * callback.
> + *
> + * NOTE:
> + *
> + * For a given set of crtc's in a drm_device, if at least one does not
> + * have the mode_valid callback, or, at least one returns MODE_OK then
> + * the mode will be probbed.
> + *
> + * RETURNS:
> + *
> + * drm_mode_status Enum
> + */
> + 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
> --
> 1.9.1
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists