lkml.org 
[lkml]   [2017]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@synopsys.com>
    > Cc: Carlos Palminha <palminha@synopsys.com>
    > Cc: Alexey Brodkin <abrodkin@synopsys.com>
    > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
    > Cc: Dave Airlie <airlied@linux.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

    \
     
     \ /
      Last update: 2017-05-02 10:49    [W:2.887 / U:0.520 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site