On Fri, May 12, 2017 at 11:24:12AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Wednesday 10 May 2017 10:03:37 Daniel Vetter wrote:
> > On Tue, May 09, 2017 at 06:00:08PM +0100, Jose Abreu wrote:
> > > This adds a new callback to crtc, encoder and bridge helper functions
> > > called mode_valid(). This callback shall be implemented if the
> > > corresponding component has some sort of restriction in the modes
> > > that can be displayed. A NULL callback implicates that the component
> > > can display all the modes.
> > >
> > > We also change the description of connector->mode_valid() callback
> > > so that it matches the existing behaviour: It is never called in
> > > atomic check phase.
> > >
> > > Only the callbacks were implemented to simplify review process,
> > > following patches will make use of them.
> > >
> > > Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>
> > > Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
> > > Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Cc: Dave Airlie <airlied@xxxxxxxx>
> > > Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > > Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> > > ---
> > >
> > > Changes v1->v2:
> > > - Change description of connector->mode_valid() (Daniel)
> > >
> > > include/drm/drm_bridge.h | 20 ++++++++++++++
> > > include/drm/drm_modeset_helper_vtables.h | 45 +++++++++++++++++++++++++++
> > > 2 files changed, 65 insertions(+)
> > >
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index fdd82fc..00c6c36 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -59,6 +59,26 @@ struct drm_bridge_funcs {
> > > void (*detach)(struct drm_bridge *bridge);
> > >
> > > /**
> > > + * @mode_valid:
> > > + *
> > > + * This callback is used to check if a specific mode is valid in this
> > > + * bridge. This should be implemented if the bridge has some sort of
> > > + * restriction in the modes it can display. For example, a given
> bridge
> > > + * 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 modes to only the ones that
> > > + * can be displayed.
> > > + *
> > > + * This is called at mode probe and at atomic check phase.
> > > + *
> > > + * RETURNS:
> > > + *
> > > + * drm_mode_status Enum
> > > + */
> > > + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
> > > + const struct drm_display_mode
> *mode);
> > > +
> > > + /**
> > > * @mode_fixup:
> > > *
> > > * This callback is used to validate and adjust a mode. The paramater
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > b/include/drm/drm_modeset_helper_vtables.h index c01c328..eec2c70 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -106,6 +106,26 @@ struct drm_crtc_helper_funcs {
> > > void (*commit)(struct drm_crtc *crtc);
> > >
> > > /**
> > > + * @mode_valid:
> > > + *
> > > + * This callback is used to check if a specific mode is valid in this
> > > + * crtc. This 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 modes to only the ones that
> > > + * can be displayed.
> > > + *
> > > + * This is called at mode probe and at atomic check phase.
> > > + *
> > > + * RETURNS:
> > > + *
> > > + * drm_mode_status Enum
> > > + */
> > > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> > > + const struct drm_display_mode
> *mode);
> > > +
> > > + /**
> > > * @mode_fixup:
> > > *
> > > * This callback is used to validate a mode. The parameter mode is the
> > > @@ -457,6 +477,26 @@ struct drm_encoder_helper_funcs {
> > > void (*dpms)(struct drm_encoder *encoder, int mode);
> > >
> > > /**
> > > + * @mode_valid:
> > > + *
> > > + * This callback is used to check if a specific mode is valid in this
> > > + * encoder. This should be implemented if the encoder has some sort
> > > + * of restriction in the modes it can display. For example, a given
> > > + * encoder 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 modes to only the ones that
> > > + * can be displayed.
> > > + *
> > > + * This is called at mode probe and at atomic check phase.
> > > + *
> > > + * RETURNS:
> > > + *
> > > + * drm_mode_status Enum
> > > + */
> > > + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
> > > + const struct drm_display_mode
> *mode);
> > > +
> > > + /**
> > > * @mode_fixup:
> > > *
> > > * This callback is used to validate and adjust a mode. The parameter
> > > @@ -795,6 +835,11 @@ struct drm_connector_helper_funcs {
> > > * (which is usually derived from the EDID data block from the sink).
> > > * See e.g. drm_helper_probe_single_connector_modes().
> > > *
> > > + * This callback is never called in atomic check phase so that
> userspace
> > > + * can override kernel sink checks in case of broken EDID with wrong
> > > + * limits from the sink. You can use the remaining mode_valid()
> > > + * callbacks to validate the mode against your video path.
> > > + *
> > > * NOTE:
> > > *
> > > * This only filters the mode list supplied to userspace in the
> >
> > Kerneldoc review seems to still be missing. One case that needs to be
> > updated is this note here. But there's a pile of other places where we
> > reference one of the mode_valid or mode_fixup functions, and they should
> > all be updated.
> >
> > Also, it'd be good to explain what to put into mode_valid and what to put
> > into mode_fixup, for objects which have both. I can help with this, but I
> > think it'd be good if you make a first round, since that might catch some
> > interactions we've missed.
>
> I was going to mention that. Interactions between mode_valid and mode_fixup
> are not defined clearly. Additionally, even though it might be a bit out of
> scope for this patch series, I think we should also define what mode_fixup is
> allowed to fix and what it should reject straight away.
>
> Thinking about it, do we really need two separate operations ? As I understand
> it, mode_fixup is mostly (only ? - this is where we need documentation) used
> to fixup the pixel clock frequency as clock generators usually have
> limitations in their dividers. We assume that the sync won't care too much,
> and happily feed it with a mode that is slightly different from what userspace
> requested. Given that mode_valid should accepts mode for which the exact pixel
> clock frequency can't bee achieved, and that the atomic commit will fixup that
> frequency anyway, can't we apply the same processing to modes enumerated by
> the connector, and merge the mode_valid and mode_fixup operations ?
They are fairly similar, except mode_fixup also has the adjusted mode, and
needs to fill that one out. With atomic there's also the complication
that drivers with not-so-simple checks use atomic_check, and we really
can't fake the entire atomic states. So even if we could merge mode_fixup
and mode_valid somehow, we'll be left with atomic_check.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
