Re: [Intel-gfx] [PATCH] drm/atomic: Add connector atomic_check function.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2017-04-05 at 12:06 +0200, Daniel Vetter wrote:
> On Wed, Apr 05, 2017 at 10:41:24AM +0200, Maarten Lankhorst wrote:
> > The connector atomic check function may be called multiple times,
> > or not at all. It's mostly useful for implementing properties but if you
> > call check_modeset twice it can be used for other modeset related checks
> > as well.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c      | 25 +++++++++++++++++----
> >  include/drm/drm_modeset_helper_vtables.h | 37 ++++++++++++++++++++++++++++++++
> >  2 files changed, 58 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index c3994b4d5f32..d550e79e8347 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -459,10 +459,20 @@ mode_fixup(struct drm_atomic_state *state)
> >   *
> >   * Check the state object to see if the requested state is physically possible.
> >   * This does all the crtc and connector related computations for an atomic
> > - * update and adds any additional connectors needed for full modesets and calls
> > - * down into &drm_crtc_helper_funcs.mode_fixup and
> > - * &drm_encoder_helper_funcs.mode_fixup or
> > - * &drm_encoder_helper_funcs.atomic_check functions of the driver backend.
> > + * update and adds any additional connectors needed for full modesets. It calls
> > + * the various per-object callbacks in the follow order:
> > + *
> > + * 1. &drm_connector_helper_funcs.atomic_best_encoder for determining the new encoder.
> > + * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
> > + * 3. If it's determined a modeset is needed then all connectors on the affected crtc
> > + *    crtc are added.
Typo - 'crtc' typed twice.

update_output_state which is called before _helper_check_modeset() also
adds all affected connectors. Why is that not considered when you say
affected connectors are added in Step 3? Is that because
update_output_state() is in the legacy modeset path?


> > + * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > + * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
> > + *    This function is only called when the encoder will be part of a configured crtc,
> > + *    it must not be used for implementing connector property validation.
> > + *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
> > + *    instead.
> > + * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
> 
> Line too I think. And maybe insert empty lines between each item, to make
> them stand out more. Shouldn't affect rendering, but better to recheck.
> 
> >   *
> >   * &drm_crtc_state.mode_changed is set when the input mode is changed.
> >   * &drm_crtc_state.connectors_changed is set when a connector is added or
> > @@ -522,6 +532,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  		return ret;
> >  
> >  	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
> > +		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > +
> >  		/*
> >  		 * This only sets crtc->connectors_changed for routing changes,
> >  		 * drivers must set crtc->connectors_changed themselves when
> > @@ -539,6 +551,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  			    new_connector_state->link_status)
> >  				new_crtc_state->connectors_changed = true;
> >  		}
> > +
> > +		if (funcs->atomic_check)
> > +			ret = funcs->atomic_check(connector, new_connector_state);
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> >  	/*
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index b304950b402d..7b5dd909f189 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -860,6 +860,43 @@ struct drm_connector_helper_funcs {
> >  	 */
> >  	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
> >  						   struct drm_connector_state *connector_state);
> > +
> > +	/**
> > +	 * @atomic_check:
> > +	 *
> > +	 * Drivers should check connector properties in this
> > +	 * hook. This function is called from &drm_atomic_helper_check_modeset.
> > +	 *
> > +	 * This function may not be called at all on a modeset or connector
> > +	 * change, so it should only be used to validate properties.

I did not understand this part - "function may not be called at all on a
modeset or connector change". Why is that?

> > +	 * If it's required to validate all connectors from there, call
> > +	 * &drm_atomic_helper_check_modeset twice. This will result in
s/twice/again ? 
> > +	 * atomic_check possibly being called twice as well, which the callback

&drm_connector_helper_funcs.atomic_check?


I had to read this a couple of times to understand, but I guess, that's
mostly me trying to piece things together. 

-DK


> > +	 * should be made to handle correctly.
> > +	 *
> > +	 * This function is also allowed to inspect any other object's state and
> > +	 * can add more state objects to the atomic commit if needed. Care must
> > +	 * be taken though to ensure that state check and compute functions for
> > +	 * these added states are all called, and derived state in other objects
> > +	 * all updated. Again the recommendation is to just call check helpers
> > +	 * until a maximal configuration is reached.
> 
> I think it'd be good to highlight setting connectors_changed from this
> callback:
> 
> "If this callback determines that the property change requires a full
> modeset, it can set &drm_crtc_state->connectors_changed to steer the
> control flow in drm_atomic_helper_check_modeset()."
> 
> With those nits addressed, Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> 
> Would be good to get DK to r-b the patch too before merging, just for
> practice and coordination.
> 
> Cheers, Daniel
> 
> > +	 *
> > +	 * NOTE:
> > +	 *
> > +	 * This function is called in the check phase of an atomic update. The
> > +	 * driver is not allowed to change anything outside of the free-standing
> > +	 * state objects passed-in or assembled in the overall &drm_atomic_state
> > +	 * update tracking structure.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * 0 on success, -EINVAL if the state or the transition can't be
> > +	 * supported, -ENOMEM on memory allocation failure and -EDEADLK if an
> > +	 * attempt to obtain another state object ran into a &drm_modeset_lock
> > +	 * deadlock.
> > +	 */
> > +	int (*atomic_check)(struct drm_connector *connector,
> > +			    struct drm_connector_state *state);
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux