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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6652 bytes --]

On Thu, Apr 6, 2017 at 12:34 AM Maarten Lankhorst <
maarten.lankhorst@linux.intel.com> wrote:

> Op 06-04-17 om 03:27 schreef Pandiyan, Dhinakaran:
> > 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@linux.intel.com>
> >>> ---
> >>>  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?
> Yes. update_output_state is just a legacy function.
>
> When a crtc is cloned, a atomic update may only disable 1 connector and
> leave the other one enabled. In this case there's a modeset
> but all connectors are not checked yet.
>
> Similar for changing crtc_state->active, which may also result in a
> modeset with no connectors added yet.
> >
> >>> + * 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?
> I meant moving one connector to a different crtc, if there are 2
> connectors on one of the crtc the check function for the other one may not
> be called the first time around.
>
> I guess that falls under modeset, so that part may be killed.
>

With the "crtc crtc" typo fixed and this change, Reviewed-by: Dhinakaran
Pandiyan <dhinakaran.pandiyan@intel.com>
-DK

>>> +    * If it's required to validate all connectors from there, call
> >>> +    * &drm_atomic_helper_check_modeset twice. This will result in
> > s/twice/again ?
> Indeed.
> >>> +    * 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.
> Yes.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 10970 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx