Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

[Daniel Vetter]

On Thu, Oct 13, 2016 at 10:54:51AM -0400, Alex Deucher wrote:
> On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey  wrote:
> > Add some additional comments to more explicitly describe the meaning and
> > usage of the three CRTC modeset detection booleans: mode_changed,
> > connectors_changed and active_changed.
> >
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Brian Starkey 
> > ---
> >
> > Hi Daniel,
> >
> > I guess I asked for this one :-), please just check my understanding
> > is correct.
> >
> 
> The whole thread was very enlightening for me with respect to those
> flags as well.  The patch looks good to me.
> Acked-by: Alex Deucher 

Applied to drm-misc, thanks.
-Daniel

> 
> > Thanks,
> > Brian
> >
> >  drivers/gpu/drm/drm_atomic_helper.c |9 +
> >  include/drm/drm_atomic.h|   11 ++-
> >  include/drm/drm_crtc.h  |5 +
> >  3 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 78ea735..fb4071a 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
> >   * removed from the crtc.
> >   * crtc_state->active_changed is set when crtc_state->active changes,
> >   * which is used for dpms.
> > + * See also: drm_atomic_crtc_needs_modeset()
> >   *
> >   * IMPORTANT:
> >   *
> > - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks 
> > if a
> > + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if 
> > a
> >   * plane update can't be done without a full modeset) _must_ call this 
> > function
> >   * afterwards after that change. It is permitted to call this function 
> > multiple
> >   * times for the same update, e.g. when the ->atomic_check functions 
> > depend upon
> > @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >
> > for_each_connector_in_state(state, connector, connector_state, i) {
> > /*
> > -* This only sets crtc->mode_changed for routing changes,
> > -* drivers must set crtc->mode_changed themselves when 
> > connector
> > -* properties need to be updated.
> > +* This only sets crtc->connectors_changed for routing 
> > changes,
> > +* drivers must set crtc->connectors_changed themselves when
> > +* connector properties need to be updated.
> >  */
> > ret = update_connector_routing(state, connector,
> >connector_state);
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index d9aff06..1ce255f 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct 
> > drm_atomic_state *state);
> >   *
> >   * To give drivers flexibility struct _crtc_state has 3 booleans to 
> > track
> >   * whether the state CRTC changed enough to need a full modeset cycle:
> > - * connectors_changed, mode_changed and active_change. This helper simply
> > + * connectors_changed, mode_changed and active_changed. This helper simply
> >   * combines these three to compute the overall need for a modeset for 
> > @state.
> > + *
> > + * The atomic helper code sets these booleans, but drivers can and should
> > + * change them appropriately to accurately represent whether a modeset is
> > + * really needed. In general, drivers should avoid full modesets whenever
> > + * possible.
> > + *
> > + * For example if the CRTC mode has changed, and the hardware is able to 
> > enact
> > + * the requested mode change without going through a full modeset, the 
> > driver
> > + * should clear mode_changed during its ->atomic_check.
> >   */
> >  static inline bool
> >  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index c4a3164..1f094d2 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
> >   * never return in a failure from the ->atomic_check callback. Userspace 
> > assumes
> >   * that a DPMS On will always succeed. In other words: @enable controls 
> > resource
> >   * assignment, @active controls the actual hardware state.
> > + *
> > + * The three booleans active_changed, connectors_changed and mode_changed 
> > are
> > + * intended to indicate whether a full modeset is needed, rather than 
> > strictly
> > + * describing what has changed in a commit.
> > + * See also: drm_atomic_crtc_needs_modeset()
> >   */
> >  struct drm_crtc_state {
> > struct drm_crtc *crtc;
> > --
> > 1.7.9.5
> >
> > 

Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

[Daniel Vetter]

On Thu, Oct 13, 2016 at 10:54:51AM -0400, Alex Deucher wrote:
> On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey  wrote:
> > Add some additional comments to more explicitly describe the meaning and
> > usage of the three CRTC modeset detection booleans: mode_changed,
> > connectors_changed and active_changed.
> >
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Brian Starkey 
> > ---
> >
> > Hi Daniel,
> >
> > I guess I asked for this one :-), please just check my understanding
> > is correct.
> >
> 
> The whole thread was very enlightening for me with respect to those
> flags as well.  The patch looks good to me.
> Acked-by: Alex Deucher 

Applied to drm-misc, thanks.
-Daniel

> 
> > Thanks,
> > Brian
> >
> >  drivers/gpu/drm/drm_atomic_helper.c |9 +
> >  include/drm/drm_atomic.h|   11 ++-
> >  include/drm/drm_crtc.h  |5 +
> >  3 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 78ea735..fb4071a 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
> >   * removed from the crtc.
> >   * crtc_state->active_changed is set when crtc_state->active changes,
> >   * which is used for dpms.
> > + * See also: drm_atomic_crtc_needs_modeset()
> >   *
> >   * IMPORTANT:
> >   *
> > - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks 
> > if a
> > + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if 
> > a
> >   * plane update can't be done without a full modeset) _must_ call this 
> > function
> >   * afterwards after that change. It is permitted to call this function 
> > multiple
> >   * times for the same update, e.g. when the ->atomic_check functions 
> > depend upon
> > @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >
> > for_each_connector_in_state(state, connector, connector_state, i) {
> > /*
> > -* This only sets crtc->mode_changed for routing changes,
> > -* drivers must set crtc->mode_changed themselves when 
> > connector
> > -* properties need to be updated.
> > +* This only sets crtc->connectors_changed for routing 
> > changes,
> > +* drivers must set crtc->connectors_changed themselves when
> > +* connector properties need to be updated.
> >  */
> > ret = update_connector_routing(state, connector,
> >connector_state);
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index d9aff06..1ce255f 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct 
> > drm_atomic_state *state);
> >   *
> >   * To give drivers flexibility struct _crtc_state has 3 booleans to 
> > track
> >   * whether the state CRTC changed enough to need a full modeset cycle:
> > - * connectors_changed, mode_changed and active_change. This helper simply
> > + * connectors_changed, mode_changed and active_changed. This helper simply
> >   * combines these three to compute the overall need for a modeset for 
> > @state.
> > + *
> > + * The atomic helper code sets these booleans, but drivers can and should
> > + * change them appropriately to accurately represent whether a modeset is
> > + * really needed. In general, drivers should avoid full modesets whenever
> > + * possible.
> > + *
> > + * For example if the CRTC mode has changed, and the hardware is able to 
> > enact
> > + * the requested mode change without going through a full modeset, the 
> > driver
> > + * should clear mode_changed during its ->atomic_check.
> >   */
> >  static inline bool
> >  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index c4a3164..1f094d2 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
> >   * never return in a failure from the ->atomic_check callback. Userspace 
> > assumes
> >   * that a DPMS On will always succeed. In other words: @enable controls 
> > resource
> >   * assignment, @active controls the actual hardware state.
> > + *
> > + * The three booleans active_changed, connectors_changed and mode_changed 
> > are
> > + * intended to indicate whether a full modeset is needed, rather than 
> > strictly
> > + * describing what has changed in a commit.
> > + * See also: drm_atomic_crtc_needs_modeset()
> >   */
> >  struct drm_crtc_state {
> > struct drm_crtc *crtc;
> > --
> > 1.7.9.5
> >
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > 

Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

[Alex Deucher]

On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey  wrote:
> Add some additional comments to more explicitly describe the meaning and
> usage of the three CRTC modeset detection booleans: mode_changed,
> connectors_changed and active_changed.
>
> Suggested-by: Daniel Vetter 
> Signed-off-by: Brian Starkey 
> ---
>
> Hi Daniel,
>
> I guess I asked for this one :-), please just check my understanding
> is correct.
>

The whole thread was very enlightening for me with respect to those
flags as well.  The patch looks good to me.
Acked-by: Alex Deucher 

> Thanks,
> Brian
>
>  drivers/gpu/drm/drm_atomic_helper.c |9 +
>  include/drm/drm_atomic.h|   11 ++-
>  include/drm/drm_crtc.h  |5 +
>  3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 78ea735..fb4071a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
>   * removed from the crtc.
>   * crtc_state->active_changed is set when crtc_state->active changes,
>   * which is used for dpms.
> + * See also: drm_atomic_crtc_needs_modeset()
>   *
>   * IMPORTANT:
>   *
> - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks 
> if a
> + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
>   * plane update can't be done without a full modeset) _must_ call this 
> function
>   * afterwards after that change. It is permitted to call this function 
> multiple
>   * times for the same update, e.g. when the ->atomic_check functions depend 
> upon
> @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>
> for_each_connector_in_state(state, connector, connector_state, i) {
> /*
> -* This only sets crtc->mode_changed for routing changes,
> -* drivers must set crtc->mode_changed themselves when 
> connector
> -* properties need to be updated.
> +* This only sets crtc->connectors_changed for routing 
> changes,
> +* drivers must set crtc->connectors_changed themselves when
> +* connector properties need to be updated.
>  */
> ret = update_connector_routing(state, connector,
>connector_state);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d9aff06..1ce255f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct 
> drm_atomic_state *state);
>   *
>   * To give drivers flexibility struct _crtc_state has 3 booleans to track
>   * whether the state CRTC changed enough to need a full modeset cycle:
> - * connectors_changed, mode_changed and active_change. This helper simply
> + * connectors_changed, mode_changed and active_changed. This helper simply
>   * combines these three to compute the overall need for a modeset for @state.
> + *
> + * The atomic helper code sets these booleans, but drivers can and should
> + * change them appropriately to accurately represent whether a modeset is
> + * really needed. In general, drivers should avoid full modesets whenever
> + * possible.
> + *
> + * For example if the CRTC mode has changed, and the hardware is able to 
> enact
> + * the requested mode change without going through a full modeset, the driver
> + * should clear mode_changed during its ->atomic_check.
>   */
>  static inline bool
>  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c4a3164..1f094d2 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
>   * never return in a failure from the ->atomic_check callback. Userspace 
> assumes
>   * that a DPMS On will always succeed. In other words: @enable controls 
> resource
>   * assignment, @active controls the actual hardware state.
> + *
> + * The three booleans active_changed, connectors_changed and mode_changed are
> + * intended to indicate whether a full modeset is needed, rather than 
> strictly
> + * describing what has changed in a commit.
> + * See also: drm_atomic_crtc_needs_modeset()
>   */
>  struct drm_crtc_state {
> struct drm_crtc *crtc;
> --
> 1.7.9.5
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

[Alex Deucher]

On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey  wrote:
> Add some additional comments to more explicitly describe the meaning and
> usage of the three CRTC modeset detection booleans: mode_changed,
> connectors_changed and active_changed.
>
> Suggested-by: Daniel Vetter 
> Signed-off-by: Brian Starkey 
> ---
>
> Hi Daniel,
>
> I guess I asked for this one :-), please just check my understanding
> is correct.
>

The whole thread was very enlightening for me with respect to those
flags as well.  The patch looks good to me.
Acked-by: Alex Deucher 

> Thanks,
> Brian
>
>  drivers/gpu/drm/drm_atomic_helper.c |9 +
>  include/drm/drm_atomic.h|   11 ++-
>  include/drm/drm_crtc.h  |5 +
>  3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 78ea735..fb4071a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
>   * removed from the crtc.
>   * crtc_state->active_changed is set when crtc_state->active changes,
>   * which is used for dpms.
> + * See also: drm_atomic_crtc_needs_modeset()
>   *
>   * IMPORTANT:
>   *
> - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks 
> if a
> + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
>   * plane update can't be done without a full modeset) _must_ call this 
> function
>   * afterwards after that change. It is permitted to call this function 
> multiple
>   * times for the same update, e.g. when the ->atomic_check functions depend 
> upon
> @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>
> for_each_connector_in_state(state, connector, connector_state, i) {
> /*
> -* This only sets crtc->mode_changed for routing changes,
> -* drivers must set crtc->mode_changed themselves when 
> connector
> -* properties need to be updated.
> +* This only sets crtc->connectors_changed for routing 
> changes,
> +* drivers must set crtc->connectors_changed themselves when
> +* connector properties need to be updated.
>  */
> ret = update_connector_routing(state, connector,
>connector_state);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d9aff06..1ce255f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct 
> drm_atomic_state *state);
>   *
>   * To give drivers flexibility struct _crtc_state has 3 booleans to track
>   * whether the state CRTC changed enough to need a full modeset cycle:
> - * connectors_changed, mode_changed and active_change. This helper simply
> + * connectors_changed, mode_changed and active_changed. This helper simply
>   * combines these three to compute the overall need for a modeset for @state.
> + *
> + * The atomic helper code sets these booleans, but drivers can and should
> + * change them appropriately to accurately represent whether a modeset is
> + * really needed. In general, drivers should avoid full modesets whenever
> + * possible.
> + *
> + * For example if the CRTC mode has changed, and the hardware is able to 
> enact
> + * the requested mode change without going through a full modeset, the driver
> + * should clear mode_changed during its ->atomic_check.
>   */
>  static inline bool
>  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c4a3164..1f094d2 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
>   * never return in a failure from the ->atomic_check callback. Userspace 
> assumes
>   * that a DPMS On will always succeed. In other words: @enable controls 
> resource
>   * assignment, @active controls the actual hardware state.
> + *
> + * The three booleans active_changed, connectors_changed and mode_changed are
> + * intended to indicate whether a full modeset is needed, rather than 
> strictly
> + * describing what has changed in a commit.
> + * See also: drm_atomic_crtc_needs_modeset()
>   */
>  struct drm_crtc_state {
> struct drm_crtc *crtc;
> --
> 1.7.9.5
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel