* [PATCH] drm/atomic: Add connector atomic_check function. @ 2017-04-05 8:41 Maarten Lankhorst 2017-04-05 10:00 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-04-05 10:06 ` [PATCH] " Daniel Vetter 0 siblings, 2 replies; 7+ messages in thread From: Maarten Lankhorst @ 2017-04-05 8:41 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx 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. * * &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); }; /** -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/atomic: Add connector atomic_check function. 2017-04-05 8:41 [PATCH] drm/atomic: Add connector atomic_check function Maarten Lankhorst @ 2017-04-05 10:00 ` Patchwork 2017-04-05 10:06 ` [PATCH] " Daniel Vetter 1 sibling, 0 replies; 7+ messages in thread From: Patchwork @ 2017-04-05 10:00 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/atomic: Add connector atomic_check function. URL : https://patchwork.freedesktop.org/series/22507/ State : success == Summary == Series 22507v1 drm/atomic: Add connector atomic_check function. https://patchwork.freedesktop.org/api/1.0/series/22507/revisions/1/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#100007 Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 430s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 429s fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 572s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 512s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 536s fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 487s fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 482s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 410s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 506s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 426s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 493s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 460s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 455s fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time: 568s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 460s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 570s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 460s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 486s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 434s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 531s fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time: 410s bdea8c440d1dba4f8f8145cf86852011f396282f drm-tip: 2017y-04m-05d-09h-08m-21s UTC integration manifest 9fcc128 drm/atomic: Add connector atomic_check function. == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4405/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic: Add connector atomic_check function. 2017-04-05 8:41 [PATCH] drm/atomic: Add connector atomic_check function Maarten Lankhorst 2017-04-05 10:00 ` ✓ Fi.CI.BAT: success for " Patchwork @ 2017-04-05 10:06 ` Daniel Vetter 2017-04-05 10:22 ` Maarten Lankhorst 2017-04-06 1:27 ` Pandiyan, Dhinakaran 1 sibling, 2 replies; 7+ messages in thread From: Daniel Vetter @ 2017-04-05 10:06 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic: Add connector atomic_check function. 2017-04-05 10:06 ` [PATCH] " Daniel Vetter @ 2017-04-05 10:22 ` Maarten Lankhorst 2017-04-06 1:27 ` Pandiyan, Dhinakaran 1 sibling, 0 replies; 7+ messages in thread From: Maarten Lankhorst @ 2017-04-05 10:22 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: Intel Graphics Development, 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? _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic: Add connector atomic_check function. 2017-04-05 10:06 ` [PATCH] " Daniel Vetter 2017-04-05 10:22 ` Maarten Lankhorst @ 2017-04-06 1:27 ` Pandiyan, Dhinakaran 2017-04-06 7:33 ` [Intel-gfx] " Maarten Lankhorst 1 sibling, 1 reply; 7+ messages in thread From: Pandiyan, Dhinakaran @ 2017-04-06 1:27 UTC (permalink / raw) To: daniel; +Cc: intel-gfx, dri-devel 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Add connector atomic_check function. 2017-04-06 1:27 ` Pandiyan, Dhinakaran @ 2017-04-06 7:33 ` Maarten Lankhorst 2017-04-06 8:36 ` Dhinakaran Pandiyan 0 siblings, 1 reply; 7+ messages in thread From: Maarten Lankhorst @ 2017-04-06 7:33 UTC (permalink / raw) To: Pandiyan, Dhinakaran, daniel; +Cc: intel-gfx, 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic: Add connector atomic_check function. 2017-04-06 7:33 ` [Intel-gfx] " Maarten Lankhorst @ 2017-04-06 8:36 ` Dhinakaran Pandiyan 0 siblings, 0 replies; 7+ messages in thread From: Dhinakaran Pandiyan @ 2017-04-06 8:36 UTC (permalink / raw) To: Maarten Lankhorst, Pandiyan, Dhinakaran, daniel; +Cc: intel-gfx, dri-devel [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-06 8:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-05 8:41 [PATCH] drm/atomic: Add connector atomic_check function Maarten Lankhorst 2017-04-05 10:00 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-04-05 10:06 ` [PATCH] " Daniel Vetter 2017-04-05 10:22 ` Maarten Lankhorst 2017-04-06 1:27 ` Pandiyan, Dhinakaran 2017-04-06 7:33 ` [Intel-gfx] " Maarten Lankhorst 2017-04-06 8:36 ` Dhinakaran Pandiyan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.