Message ID | 1491381684-5267-1-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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. > + * 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. > + * If it's required to validate all connectors from there, call > + * &drm_atomic_helper_check_modeset twice. This will result in > + * atomic_check possibly being called twice as well, which the callback > + * 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@ffwll.ch> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 05-04-17 om 12:06 schreef Daniel Vetter: > 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. >> + * 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. >> + * If it's required to validate all connectors from there, call >> + * &drm_atomic_helper_check_modeset twice. This will result in >> + * atomic_check possibly being called twice as well, which the callback >> + * 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@ffwll.ch> > > Would be good to get DK to r-b the patch too before merging, just for > practice and coordination. > > Cheers, Daniel Could you review?
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? > > + * 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@ffwll.ch> > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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. >>> + * 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.
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 >
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. + * 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. * * &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. + * If it's required to validate all connectors from there, call + * &drm_atomic_helper_check_modeset twice. This will result in + * atomic_check possibly being called twice as well, which the callback + * 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. + * + * 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); }; /**
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(-)