* [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.