Re: [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Daniel Vetter]

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