Re-spin of Archit's original RFC. I've added a modeset lock to the private object, making it operate in a way more consistent with other objects, and avoiding the need for the locking dance in the driver.
Possibly we could split up mixer state from smp/hwpipe state into separate private objects. In that case, the purpose would be mainly to split the locking, so having the modeset lock per object in drm core works out nicely for that.
The remainder of the patchset is Archit's original patchset, with the now redundant locking removed, and rebased onto msm-next.
From original cover-letter:
It's been recommended that we use drm_private_objs embedded in drm_atomic_state to hold shared resources instead of subclassing drm_atomic_state.
This will also help us in getting one step closer to using the atomic commit helpers instead of the msm_atomic_commit() funcs in msm_atomic.c
Archit Taneja (3): drm/msm/mdp5: Add global state as a private atomic object drm/msm/mdp5: Use the new private_obj state drm/msm: Don't subclass drm_atomic_state anymore
Rob Clark (1): drm/atomic: integrate modeset lock with private objects
drivers/gpu/drm/drm_atomic.c | 9 ++- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 101 ++++++++++++++++++++--------- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 26 ++++---- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++-- drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 20 +++--- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 17 +++-- drivers/gpu/drm/msm/msm_atomic.c | 31 --------- drivers/gpu/drm/msm/msm_drv.c | 3 - drivers/gpu/drm/msm/msm_kms.h | 14 ---- include/drm/drm_atomic.h | 5 ++ 10 files changed, 126 insertions(+), 112 deletions(-)
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
+ drm_modeset_lock_init(&obj->lock); + obj->state = state; obj->funcs = funcs; } @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state); + drm_modeset_lock_fini(&obj->lock); } EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) { - int index, num_objs, i; + int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state; @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
+ ret = drm_modeset_lock(&obj->lock, state->acquire_ctx); + if (ret) + return ERR_PTR(ret); + num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs { * &drm_modeset_lock is required to duplicate and update this object's state. */ struct drm_private_obj { + /** + * @lock: Modeset lock to protect the state object. + */ + struct drm_modeset_lock lock; + /** * @state: Current atomic state for this driver private object. */
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
- drm_modeset_lock_init(&obj->lock);
- obj->state = state; obj->funcs = funcs;
} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
- drm_modeset_lock_fini(&obj->lock);
} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
- int index, num_objs, i;
- int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;
@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
- ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
- if (ret)
return ERR_PTR(ret);- num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
- /**
* @lock: Modeset lock to protect the state object.*/- struct drm_modeset_lock lock;
- /**
*/
- @state: Current atomic state for this driver private object.
-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
drm_modeset_lock_init(&obj->lock);obj->state = state; obj->funcs = funcs;} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
drm_modeset_lock_fini(&obj->lock);} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
int index, num_objs, i;
int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);if (ret)return ERR_PTR(ret);num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
/*** @lock: Modeset lock to protect the state object.*/struct drm_modeset_lock lock;/** * @state: Current atomic state for this driver private object. */-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
drm_modeset_lock_init(&obj->lock);obj->state = state; obj->funcs = funcs;} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
drm_modeset_lock_fini(&obj->lock);} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
int index, num_objs, i;
int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);if (ret)return ERR_PTR(ret);num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
/*** @lock: Modeset lock to protect the state object.*/struct drm_modeset_lock lock;/** * @state: Current atomic state for this driver private object. */-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
BR, -R
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
drm_modeset_lock_init(&obj->lock);obj->state = state; obj->funcs = funcs;} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
drm_modeset_lock_fini(&obj->lock);} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
int index, num_objs, i;
int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);if (ret)return ERR_PTR(ret);num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
/*** @lock: Modeset lock to protect the state object.*/struct drm_modeset_lock lock;/** * @state: Current atomic state for this driver private object. */-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
BR, -R
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
drm_modeset_lock_init(&obj->lock);obj->state = state; obj->funcs = funcs;} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
drm_modeset_lock_fini(&obj->lock);} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
int index, num_objs, i;
int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);if (ret)return ERR_PTR(ret);num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
/*** @lock: Modeset lock to protect the state object.*/struct drm_modeset_lock lock;/** * @state: Current atomic state for this driver private object. */-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
I'm not sure why that is strictly true (that you need to add it to the state if for read-only), since you'd be guarding it with your own driver read-lock you can just priv->foo_state->bar.
Since it is read-only access, there is no roll-back to worry about for test-only or failed atomic_check()s..
BR, -R
BR, -R
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
drm_modeset_lock_init(&obj->lock);obj->state = state; obj->funcs = funcs;} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
drm_modeset_lock_fini(&obj->lock);} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
int index, num_objs, i;
int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);if (ret)return ERR_PTR(ret);num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
/*** @lock: Modeset lock to protect the state object.*/struct drm_modeset_lock lock;/** * @state: Current atomic state for this driver private object. */-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote: > Follow the same pattern of locking as with other state objects. This > avoids boilerplate in the driver.
I'm not sure we really want to do this. What if the driver wants a custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
I'm not sure why that is strictly true (that you need to add it to the state if for read-only), since you'd be guarding it with your own driver read-lock you can just priv->foo_state->bar.
Since it is read-only access, there is no roll-back to worry about for test-only or failed atomic_check()s..
That would be super ugly. We want to access the information the same way whether it has been modified or not.
BR, -R
BR, -R
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
> > Signed-off-by: Rob Clark robdclark@gmail.com > --- > drivers/gpu/drm/drm_atomic.c | 9 ++++++++- > include/drm/drm_atomic.h | 5 +++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index fc8c4da409ff..004e621ab307 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, > { > memset(obj, 0, sizeof(*obj)); > > + drm_modeset_lock_init(&obj->lock); > + > obj->state = state; > obj->funcs = funcs; > } > @@ -1093,6 +1095,7 @@ void > drm_atomic_private_obj_fini(struct drm_private_obj *obj) > { > obj->funcs->atomic_destroy_state(obj, obj->state); > + drm_modeset_lock_fini(&obj->lock); > } > EXPORT_SYMBOL(drm_atomic_private_obj_fini); > > @@ -1113,7 +1116,7 @@ struct drm_private_state * > drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > struct drm_private_obj *obj) > { > - int index, num_objs, i; > + int index, num_objs, i, ret; > size_t size; > struct __drm_private_objs_state *arr; > struct drm_private_state *obj_state; > @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > if (obj == state->private_objs[i].ptr) > return state->private_objs[i].state; > > + ret = drm_modeset_lock(&obj->lock, state->acquire_ctx); > + if (ret) > + return ERR_PTR(ret); > + > num_objs = state->num_private_objs + 1; > size = sizeof(*state->private_objs) * num_objs; > arr = krealloc(state->private_objs, size, GFP_KERNEL); > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 09076a625637..9ae53b73c9d2 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -218,6 +218,11 @@ struct drm_private_state_funcs { > * &drm_modeset_lock is required to duplicate and update this object's state. > */ > struct drm_private_obj { > + /** > + * @lock: Modeset lock to protect the state object. > + */ > + struct drm_modeset_lock lock; > + > /** > * @state: Current atomic state for this driver private object. > */ > -- > 2.14.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote: > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote: >> Follow the same pattern of locking as with other state objects. This >> avoids boilerplate in the driver. > > I'm not sure we really want to do this. What if the driver wants a > custom locking scheme for this state?
That seems like something we want to discourage, ie. all the more reason for this patch.
There is no reason drivers could not split their global state into multiple private objs's, each with their own lock, for more fine grained locking. That is basically the only valid reason I can think of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
I'm not sure why that is strictly true (that you need to add it to the state if for read-only), since you'd be guarding it with your own driver read-lock you can just priv->foo_state->bar.
Since it is read-only access, there is no roll-back to worry about for test-only or failed atomic_check()s..
That would be super ugly. We want to access the information the same way whether it has been modified or not.
Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
I mean, in mdp5 the assigned global resources go in plane/crtc state, and tracking of what is assigned to which plane/crtc is in global state, so it fits nicely in the current locking model. For i915, I'm not quite sure what is the global state you are concerned about, so it is a bit hard to talk about the best solution in the abstract. Maybe the better option is to teach modeset-lock how to be a rwlock instead?
BR, -R
BR, -R
BR, -R
(And ofc drivers could add there own locks in addition to what is done by core, but I'd rather look at that on a case by case basis, rather than it being part of the boilerplate in each driver.)
BR, -R
>> >> Signed-off-by: Rob Clark robdclark@gmail.com >> --- >> drivers/gpu/drm/drm_atomic.c | 9 ++++++++- >> include/drm/drm_atomic.h | 5 +++++ >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index fc8c4da409ff..004e621ab307 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, >> { >> memset(obj, 0, sizeof(*obj)); >> >> + drm_modeset_lock_init(&obj->lock); >> + >> obj->state = state; >> obj->funcs = funcs; >> } >> @@ -1093,6 +1095,7 @@ void >> drm_atomic_private_obj_fini(struct drm_private_obj *obj) >> { >> obj->funcs->atomic_destroy_state(obj, obj->state); >> + drm_modeset_lock_fini(&obj->lock); >> } >> EXPORT_SYMBOL(drm_atomic_private_obj_fini); >> >> @@ -1113,7 +1116,7 @@ struct drm_private_state * >> drm_atomic_get_private_obj_state(struct drm_atomic_state *state, >> struct drm_private_obj *obj) >> { >> - int index, num_objs, i; >> + int index, num_objs, i, ret; >> size_t size; >> struct __drm_private_objs_state *arr; >> struct drm_private_state *obj_state; >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, >> if (obj == state->private_objs[i].ptr) >> return state->private_objs[i].state; >> >> + ret = drm_modeset_lock(&obj->lock, state->acquire_ctx); >> + if (ret) >> + return ERR_PTR(ret); >> + >> num_objs = state->num_private_objs + 1; >> size = sizeof(*state->private_objs) * num_objs; >> arr = krealloc(state->private_objs, size, GFP_KERNEL); >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >> index 09076a625637..9ae53b73c9d2 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs { >> * &drm_modeset_lock is required to duplicate and update this object's state. >> */ >> struct drm_private_obj { >> + /** >> + * @lock: Modeset lock to protect the state object. >> + */ >> + struct drm_modeset_lock lock; >> + >> /** >> * @state: Current atomic state for this driver private object. >> */ >> -- >> 2.14.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Wed, Feb 21, 2018 at 11:17:21AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote: > On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä > ville.syrjala@linux.intel.com wrote: > > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote: > >> Follow the same pattern of locking as with other state objects. This > >> avoids boilerplate in the driver. > > > > I'm not sure we really want to do this. What if the driver wants a > > custom locking scheme for this state? > > That seems like something we want to discourage, ie. all the more > reason for this patch. > > There is no reason drivers could not split their global state into > multiple private objs's, each with their own lock, for more fine > grained locking. That is basically the only valid reason I can think > of for "custom locking".
In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again.
Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
I'm not sure why that is strictly true (that you need to add it to the state if for read-only), since you'd be guarding it with your own driver read-lock you can just priv->foo_state->bar.
Since it is read-only access, there is no roll-back to worry about for test-only or failed atomic_check()s..
That would be super ugly. We want to access the information the same way whether it has been modified or not.
Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
I mean, in mdp5 the assigned global resources go in plane/crtc state, and tracking of what is assigned to which plane/crtc is in global state, so it fits nicely in the current locking model. For i915, I'm not quite sure what is the global state you are concerned about, so it is a bit hard to talk about the best solution in the abstract. Maybe the better option is to teach modeset-lock how to be a rwlock instead?
The thing I'm thinking is the core display clock (cdclk) frequency which we need to consult whenever computing plane states and whatnot. We don't want a modeset on one crtc to block a plane update on another crtc unless we actually have to bump the cdclk (which would generally require all crtcs to undergo a full modeset). Seems like a generally useful pattern to me.
On Wed, Feb 21, 2018 at 11:36 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 11:17:21AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote: > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote: >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä >> ville.syrjala@linux.intel.com wrote: >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote: >> >> Follow the same pattern of locking as with other state objects. This >> >> avoids boilerplate in the driver. >> > >> > I'm not sure we really want to do this. What if the driver wants a >> > custom locking scheme for this state? >> >> That seems like something we want to discourage, ie. all the more >> reason for this patch. >> >> There is no reason drivers could not split their global state into >> multiple private objs's, each with their own lock, for more fine >> grained locking. That is basically the only valid reason I can think >> of for "custom locking". > > In i915 we have at least one case that would want something close to an > rwlock. Any crtc lock is enough for read, need all of them for write. > Though if we wanted to use private objs for that we might need to > actually make the states refcounted as well, otherwise I can imagine > we might land in some use-after-free issues once again. > > Maybe we could duplicate the state into per-crtc and global copies, but > then we have to keep all of those in sync somehow which doesn't sound > particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
I'm not sure why that is strictly true (that you need to add it to the state if for read-only), since you'd be guarding it with your own driver read-lock you can just priv->foo_state->bar.
Since it is read-only access, there is no roll-back to worry about for test-only or failed atomic_check()s..
That would be super ugly. We want to access the information the same way whether it has been modified or not.
Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
I mean, in mdp5 the assigned global resources go in plane/crtc state, and tracking of what is assigned to which plane/crtc is in global state, so it fits nicely in the current locking model. For i915, I'm not quite sure what is the global state you are concerned about, so it is a bit hard to talk about the best solution in the abstract. Maybe the better option is to teach modeset-lock how to be a rwlock instead?
The thing I'm thinking is the core display clock (cdclk) frequency which we need to consult whenever computing plane states and whatnot. We don't want a modeset on one crtc to block a plane update on another crtc unless we actually have to bump the cdclk (which would generally require all crtcs to undergo a full modeset). Seems like a generally useful pattern to me.
Hmm.. I suppose either way, you'd have to make modeset-lock have rw semantics or invent some way to play nicely with acquire_ctx.. once you have that, I'd have no objection to make private state use rwlock semantics. That seems better than having drivers (badly) roll their own. And maybe as you say, it would be a useful pattern for other drivers.
BR, -R
On Wed, Feb 21, 2018 at 06:36:54PM +0200, Ville Syrjälä wrote:
On Wed, Feb 21, 2018 at 11:17:21AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote: > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote: >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä >> ville.syrjala@linux.intel.com wrote: >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote: >> >> Follow the same pattern of locking as with other state objects. This >> >> avoids boilerplate in the driver. >> > >> > I'm not sure we really want to do this. What if the driver wants a >> > custom locking scheme for this state? >> >> That seems like something we want to discourage, ie. all the more >> reason for this patch. >> >> There is no reason drivers could not split their global state into >> multiple private objs's, each with their own lock, for more fine >> grained locking. That is basically the only valid reason I can think >> of for "custom locking". > > In i915 we have at least one case that would want something close to an > rwlock. Any crtc lock is enough for read, need all of them for write. > Though if we wanted to use private objs for that we might need to > actually make the states refcounted as well, otherwise I can imagine > we might land in some use-after-free issues once again. > > Maybe we could duplicate the state into per-crtc and global copies, but > then we have to keep all of those in sync somehow which doesn't sound > particularly pleasant.
Or just keep your own driver lock for read, and use that plus the core modeset lock for write?
If we can't add the private obj to the state we can't really use it.
I'm not sure why that is strictly true (that you need to add it to the state if for read-only), since you'd be guarding it with your own driver read-lock you can just priv->foo_state->bar.
Since it is read-only access, there is no roll-back to worry about for test-only or failed atomic_check()s..
That would be super ugly. We want to access the information the same way whether it has been modified or not.
Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
I mean, in mdp5 the assigned global resources go in plane/crtc state, and tracking of what is assigned to which plane/crtc is in global state, so it fits nicely in the current locking model. For i915, I'm not quite sure what is the global state you are concerned about, so it is a bit hard to talk about the best solution in the abstract. Maybe the better option is to teach modeset-lock how to be a rwlock instead?
The thing I'm thinking is the core display clock (cdclk) frequency which we need to consult whenever computing plane states and whatnot. We don't want a modeset on one crtc to block a plane update on another crtc unless we actually have to bump the cdclk (which would generally require all crtcs to undergo a full modeset). Seems like a generally useful pattern to me.
The usual way to fix that is to have read-only copies of the state in the plane or crtc states. And for writing (or if the requirement changes) you have to lock all the objects. Essentially what Rob's doing for his plane/crtc assignment stuff.
What we do in i915 is kinda not what I've been recommending to everyone else, because it is a rather tricky and complicated way to get things done. Sure there's a tradeoff between duplicating data and complicated locking schemes, but I think for the kms case having to explicitly type code that reflects the depencies in computation (instead of having that embedded implicitly in the locking scheme) is a feature, not a bug. -Daniel
Hey,
Op 21-02-18 om 15:37 schreef Rob Clark:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm afraid this will prohibit any uses of this on i915, since it still uses legacy lock_all().
Oh well, afaict nothing in i915 uses private objects, so I don't think it's harmful. :)
Could you cc intel-gfx just in case?
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
- drm_modeset_lock_init(&obj->lock);
- obj->state = state; obj->funcs = funcs;
} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
- drm_modeset_lock_fini(&obj->lock);
} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
- int index, num_objs, i;
- int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;
@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
- ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
- if (ret)
return ERR_PTR(ret);- num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
- /**
* @lock: Modeset lock to protect the state object.*/- struct drm_modeset_lock lock;
- /**
*/
- @state: Current atomic state for this driver private object.
On Wed, Feb 21, 2018 at 04:19:40PM +0100, Maarten Lankhorst wrote:
Hey,
Op 21-02-18 om 15:37 schreef Rob Clark:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm afraid this will prohibit any uses of this on i915, since it still uses legacy lock_all().
Oh well, afaict nothing in i915 uses private objects, so I don't think it's harmful. :)
We do use private objects, as part of dp mst helpers. But I also thought that the only users left of lock_all are in the debugfs code, where this really doesn't matter all that much.
Could you cc intel-gfx just in case?
Yeah, best to double-check with CI.
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
- drm_modeset_lock_init(&obj->lock);
- obj->state = state; obj->funcs = funcs;
} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
- drm_modeset_lock_fini(&obj->lock);
} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
- int index, num_objs, i;
- int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;
@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
- ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
- if (ret)
return ERR_PTR(ret);- num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
- /**
* @lock: Modeset lock to protect the state object.*/- struct drm_modeset_lock lock;
- /**
*/
- @state: Current atomic state for this driver private object.
On Tue, Mar 06, 2018 at 08:29:20AM +0100, Daniel Vetter wrote:
On Wed, Feb 21, 2018 at 04:19:40PM +0100, Maarten Lankhorst wrote:
Hey,
Op 21-02-18 om 15:37 schreef Rob Clark:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
I'm afraid this will prohibit any uses of this on i915, since it still uses legacy lock_all().
Oh well, afaict nothing in i915 uses private objects, so I don't think it's harmful. :)
We do use private objects, as part of dp mst helpers. But I also thought that the only users left of lock_all are in the debugfs code, where this really doesn't matter all that much.
Correction, we use it in other places than debugfs. But thanks to Ville's private state obj refactoring we now have drm_atomic_private_obj_init(), so it's easy to add all the private state objects to a new list in drm_dev->mode_config.private_states or so, and use that list in drm_modeset_lock_all_ctx to also take driver private locks.
I think that would actually be useful in other places, just in case. -Daniel
Could you cc intel-gfx just in case?
Yeah, best to double-check with CI.
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
- drm_modeset_lock_init(&obj->lock);
- obj->state = state; obj->funcs = funcs;
} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
- drm_modeset_lock_fini(&obj->lock);
} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
- int index, num_objs, i;
- int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;
@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
- ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
- if (ret)
return ERR_PTR(ret);- num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
- /**
* @lock: Modeset lock to protect the state object.*/- struct drm_modeset_lock lock;
- /**
*/
- @state: Current atomic state for this driver private object.
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
Follow the same pattern of locking as with other state objects. This avoids boilerplate in the driver.
Signed-off-by: Rob Clark robdclark@gmail.com
Please also adjust the kernel doc, and I think we can remove the locking WARN_ON in drm_atomic_get_mst_topology_state after this patch (plus again adjust the kerneldoc for that please).
Otherwise I think this makes sense, and ecnourages reasonable semantics for driver private state objects. -Daniel
drivers/gpu/drm/drm_atomic.c | 9 ++++++++- include/drm/drm_atomic.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index fc8c4da409ff..004e621ab307 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, { memset(obj, 0, sizeof(*obj));
- drm_modeset_lock_init(&obj->lock);
- obj->state = state; obj->funcs = funcs;
} @@ -1093,6 +1095,7 @@ void drm_atomic_private_obj_fini(struct drm_private_obj *obj) { obj->funcs->atomic_destroy_state(obj, obj->state);
- drm_modeset_lock_fini(&obj->lock);
} EXPORT_SYMBOL(drm_atomic_private_obj_fini);
@@ -1113,7 +1116,7 @@ struct drm_private_state * drm_atomic_get_private_obj_state(struct drm_atomic_state *state, struct drm_private_obj *obj) {
- int index, num_objs, i;
- int index, num_objs, i, ret; size_t size; struct __drm_private_objs_state *arr; struct drm_private_state *obj_state;
@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, if (obj == state->private_objs[i].ptr) return state->private_objs[i].state;
- ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
- if (ret)
return ERR_PTR(ret);- num_objs = state->num_private_objs + 1; size = sizeof(*state->private_objs) * num_objs; arr = krealloc(state->private_objs, size, GFP_KERNEL);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 09076a625637..9ae53b73c9d2 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
- &drm_modeset_lock is required to duplicate and update this object's state.
*/ struct drm_private_obj {
- /**
* @lock: Modeset lock to protect the state object.*/- struct drm_modeset_lock lock;
- /**
*/
- @state: Current atomic state for this driver private object.
-- 2.14.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Archit Taneja architt@codeaurora.org
Global shared resources (hwpipes, hwmixers and SMP) for MDP5 are implemented as a part of atomic state by subclassing drm_atomic_state.
The preferred approach is to use the drm_private_obj infrastructure available in the atomic core.
mdp5_global_state is introduced as a drm atomic private object. The two funcs mdp5_get_global_state() and mdp5_get_existing_global_state() are the two variants that will be used to access mdp5_global_state.
This will replace the existing mdp5_state struct (which subclasses drm_atomic_state) and the funcs around it. These will be removed later once we mdp5_global_state is put to use everywhere.
Signed-off-by: Archit Taneja architt@codeaurora.org Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 79 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 24 ++++++++++ 2 files changed, 103 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 7ad0560dbc9f..0f314c1f3e8a 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -106,6 +106,79 @@ static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) swap(to_kms_state(state)->state, mdp5_kms->state); }
+/* Global/shared object state funcs */ + +/* + * This is a helper that returns the private state currently in operation. + * Note that this would return the "old_state" if called in the atomic check + * path, and the "new_state" after the atomic swap has been done. + */ +struct mdp5_global_state * +mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms) +{ + return to_mdp5_global_state(mdp5_kms->glob_state.state); +} + +/* + * This acquires the modeset lock set aside for global state, creates + * a new duplicated private object state. + */ +struct mdp5_global_state *mdp5_get_global_state(struct drm_atomic_state *s) +{ + struct msm_drm_private *priv = s->dev->dev_private; + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_private_obj_state(s, &mdp5_kms->glob_state); + if (IS_ERR(priv_state)) + return ERR_CAST(priv_state); + + return to_mdp5_global_state(priv_state); +} + +static struct drm_private_state * +mdp5_global_duplicate_state(struct drm_private_obj *obj) +{ + struct mdp5_global_state *state; + + state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL); + if (!state) + return NULL; + + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); + + return &state->base; +} + +static void mdp5_global_destroy_state(struct drm_private_obj *obj, + struct drm_private_state *state) +{ + struct mdp5_global_state *mdp5_state = to_mdp5_global_state(state); + + kfree(mdp5_state); +} + +static const struct drm_private_state_funcs mdp5_global_state_funcs = { + .atomic_duplicate_state = mdp5_global_duplicate_state, + .atomic_destroy_state = mdp5_global_destroy_state, +}; + +static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms) +{ + struct mdp5_global_state *state; + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (!state) + return -ENOMEM; + + state->mdp5_kms = mdp5_kms; + + drm_atomic_private_obj_init(&mdp5_kms->glob_state, + &state->base, + &mdp5_global_state_funcs); + return 0; +} + static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state) { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); @@ -779,6 +852,8 @@ static void mdp5_destroy(struct platform_device *pdev) if (mdp5_kms->rpm_enabled) pm_runtime_disable(&pdev->dev);
+ drm_atomic_private_obj_fini(&mdp5_kms->glob_state); + kfree(mdp5_kms->state); }
@@ -939,6 +1014,10 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) goto fail; }
+ ret = mdp5_global_obj_init(mdp5_kms); + if (ret) + goto fail; + mdp5_kms->mmio = msm_ioremap(pdev, "mdp_phys", "MDP5"); if (IS_ERR(mdp5_kms->mmio)) { ret = PTR_ERR(mdp5_kms->mmio); diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h index 312e6a47dff8..1c70aac20a98 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h @@ -57,6 +57,12 @@ struct mdp5_kms { struct mdp5_state *state; struct drm_modeset_lock state_lock;
+ /* + * Global private object state, Do not access directly, use + * mdp5_global_get_state() + */ + struct drm_private_obj glob_state; + struct mdp5_smp *smp; struct mdp5_ctl_manager *ctlm;
@@ -97,6 +103,24 @@ struct mdp5_state { struct mdp5_state *__must_check mdp5_get_state(struct drm_atomic_state *s);
+/* Global private object state for tracking resources that are shared across + * multiple kms objects (planes/crtcs/etc). + */ +#define to_mdp5_global_state(x) container_of(x, struct mdp5_global_state, base) +struct mdp5_global_state { + struct drm_private_state base; + + struct drm_atomic_state *state; + struct mdp5_kms *mdp5_kms; + + struct mdp5_hw_pipe_state hwpipe; + struct mdp5_hw_mixer_state hwmixer; + struct mdp5_smp_state smp; +}; + +struct mdp5_global_state * mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms); +struct mdp5_global_state *__must_check mdp5_get_global_state(struct drm_atomic_state *s); + /* Atomic plane state. Subclasses the base drm_plane_state in order to * track assigned hwpipe and hw specific state. */
From: Archit Taneja architt@codeaurora.org
This replaces the usage of the subclassed atomic state (mdp5_state) with a private_obj state embedded within drm_atomic_state. The latter method is the preferred approach, since it's simpler to implement and less prone to errors.
The new API replaces the older and equivalent mdp5_state usage in the following pattern: - References to "mdp5_kms->state" (i.e, the old/existing state) is replaced with mdp5_get_existing_global_state(). In the atomic_check path, this should be called with the glob_state_lock drm_modeset_lock alredy taken. - References to "mdp5_get_state()" are replaced with mdp5_get_global_state(). This acquires glob_state_lock and uses drm_atomic_get_private_obj_state() to create a new duplicated state.
Signed-off-by: Archit Taneja architt@codeaurora.org Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 10 ++++++++-- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++++++------ drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 20 +++++++++++--------- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 17 ++++++++++++----- 4 files changed, 37 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 0f314c1f3e8a..a7e80ebf0207 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -183,11 +183,14 @@ static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); struct device *dev = &mdp5_kms->pdev->dev; + struct mdp5_global_state *global_state; + + global_state = mdp5_get_existing_global_state(mdp5_kms);
pm_runtime_get_sync(dev);
if (mdp5_kms->smp) - mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_kms->state->smp); + mdp5_smp_prepare_commit(mdp5_kms->smp, &global_state->smp); }
static void mdp5_commit(struct msm_kms *kms, struct drm_atomic_state *state) @@ -209,9 +212,12 @@ static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); struct device *dev = &mdp5_kms->pdev->dev; + struct mdp5_global_state *global_state; + + global_state = mdp5_get_existing_global_state(mdp5_kms);
if (mdp5_kms->smp) - mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_kms->state->smp); + mdp5_smp_complete_commit(mdp5_kms->smp, &global_state->smp);
pm_runtime_put_sync(dev); } diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c index 8a00991f03c7..113e6b569562 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c @@ -52,14 +52,14 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state = mdp5_get_state(s); + struct mdp5_global_state *global_state = mdp5_get_global_state(s); struct mdp5_hw_mixer_state *new_state; int i;
- if (IS_ERR(state)) - return PTR_ERR(state); + if (IS_ERR(global_state)) + return PTR_ERR(global_state);
- new_state = &state->hwmixer; + new_state = &global_state->hwmixer;
for (i = 0; i < mdp5_kms->num_hwmixers; i++) { struct mdp5_hw_mixer *cur = mdp5_kms->hwmixers[i]; @@ -129,8 +129,8 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc,
void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) { - struct mdp5_state *state = mdp5_get_state(s); - struct mdp5_hw_mixer_state *new_state = &state->hwmixer; + struct mdp5_global_state *global_state = mdp5_get_global_state(s); + struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer;
if (!mixer) return; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c index ff52c49095f9..1ef26bc63163 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c @@ -24,17 +24,19 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state; + struct mdp5_global_state *new_global_state, *old_global_state; struct mdp5_hw_pipe_state *old_state, *new_state; int i, j;
- state = mdp5_get_state(s); - if (IS_ERR(state)) - return PTR_ERR(state); + new_global_state = mdp5_get_global_state(s); + if (IS_ERR(new_global_state)) + return PTR_ERR(new_global_state);
- /* grab old_state after mdp5_get_state(), since now we hold lock: */ - old_state = &mdp5_kms->state->hwpipe; - new_state = &state->hwpipe; + /* grab old_state after mdp5_get_global_state(), since now we hold lock: */ + old_global_state = mdp5_get_existing_global_state(mdp5_kms); + + old_state = &old_global_state->hwpipe; + new_state = &new_global_state->hwpipe;
for (i = 0; i < mdp5_kms->num_hwpipes; i++) { struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i]; @@ -107,7 +109,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, WARN_ON(r_hwpipe);
DBG("%s: alloc SMP blocks", (*hwpipe)->name); - ret = mdp5_smp_assign(mdp5_kms->smp, &state->smp, + ret = mdp5_smp_assign(mdp5_kms->smp, &new_global_state->smp, (*hwpipe)->pipe, blkcfg); if (ret) return -ENOMEM; @@ -132,7 +134,7 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe) { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state = mdp5_get_state(s); + struct mdp5_global_state *state = mdp5_get_global_state(s); struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
if (!hwpipe) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c index ae4983d9d0a5..1408eec75440 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c @@ -340,17 +340,20 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) struct mdp5_kms *mdp5_kms = get_kms(smp); struct mdp5_hw_pipe_state *hwpstate; struct mdp5_smp_state *state; + struct mdp5_global_state *global_state; int total = 0, i, j;
drm_printf(p, "name\tinuse\tplane\n"); drm_printf(p, "----\t-----\t-----\n");
if (drm_can_sleep()) - drm_modeset_lock(&mdp5_kms->state_lock, NULL); + drm_modeset_lock(&mdp5_kms->glob_state.lock, NULL); + + global_state = mdp5_get_existing_global_state(mdp5_kms);
/* grab these *after* we hold the state_lock */ - hwpstate = &mdp5_kms->state->hwpipe; - state = &mdp5_kms->state->smp; + hwpstate = &global_state->hwpipe; + state = &global_state->smp;
for (i = 0; i < mdp5_kms->num_hwpipes; i++) { struct mdp5_hw_pipe *hwpipe = mdp5_kms->hwpipes[i]; @@ -374,7 +377,7 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) bitmap_weight(state->state, smp->blk_cnt));
if (drm_can_sleep()) - drm_modeset_unlock(&mdp5_kms->state_lock); + drm_modeset_unlock(&mdp5_kms->glob_state.lock); }
void mdp5_smp_destroy(struct mdp5_smp *smp) @@ -384,7 +387,8 @@ void mdp5_smp_destroy(struct mdp5_smp *smp)
struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_block *cfg) { - struct mdp5_smp_state *state = &mdp5_kms->state->smp; + struct mdp5_smp_state *state; + struct mdp5_global_state *global_state; struct mdp5_smp *smp = NULL; int ret;
@@ -398,6 +402,9 @@ struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_ smp->blk_cnt = cfg->mmb_count; smp->blk_size = cfg->mmb_size;
+ global_state = mdp5_get_existing_global_state(mdp5_kms); + state = &global_state->smp; + /* statically tied MMBs cannot be re-allocated: */ bitmap_copy(state->state, cfg->reserved_state, smp->blk_cnt); memcpy(smp->reserved, cfg->reserved, sizeof(smp->reserved));
From: Archit Taneja architt@codeaurora.org
With the addition of "private_objs" in drm_atomic_state, we no longer need to subclass drm_atomic_state to store state of share resources that don't perfectly fit within planes/crtc/connector state information. We can now save this state within drm_atomic_state itself using the private objects.
Remove the infrastructure that allowed subclassing of drm_atomic_state in the driver.
Signed-off-by: Archit Taneja architt@codeaurora.org Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 46 -------------------------------- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 22 --------------- drivers/gpu/drm/msm/msm_atomic.c | 31 --------------------- drivers/gpu/drm/msm/msm_drv.c | 3 --- drivers/gpu/drm/msm/msm_kms.h | 14 ---------- 5 files changed, 116 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index a7e80ebf0207..da9c4a4eb050 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -70,42 +70,6 @@ static int mdp5_hw_init(struct msm_kms *kms) return 0; }
-struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s) -{ - struct msm_drm_private *priv = s->dev->dev_private; - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct msm_kms_state *state = to_kms_state(s); - struct mdp5_state *new_state; - int ret; - - if (state->state) - return state->state; - - ret = drm_modeset_lock(&mdp5_kms->state_lock, s->acquire_ctx); - if (ret) - return ERR_PTR(ret); - - new_state = kmalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); - if (!new_state) - return ERR_PTR(-ENOMEM); - - /* Copy state: */ - new_state->hwpipe = mdp5_kms->state->hwpipe; - new_state->hwmixer = mdp5_kms->state->hwmixer; - if (mdp5_kms->smp) - new_state->smp = mdp5_kms->state->smp; - - state->state = new_state; - - return new_state; -} - -static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) -{ - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); - swap(to_kms_state(state)->state, mdp5_kms->state); -} - /* Global/shared object state funcs */
/* @@ -323,7 +287,6 @@ static const struct mdp_kms_funcs kms_funcs = { .irq = mdp5_irq, .enable_vblank = mdp5_enable_vblank, .disable_vblank = mdp5_disable_vblank, - .swap_state = mdp5_swap_state, .prepare_commit = mdp5_prepare_commit, .commit = mdp5_commit, .complete_commit = mdp5_complete_commit, @@ -859,8 +822,6 @@ static void mdp5_destroy(struct platform_device *pdev) pm_runtime_disable(&pdev->dev);
drm_atomic_private_obj_fini(&mdp5_kms->glob_state); - - kfree(mdp5_kms->state); }
static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, @@ -1013,13 +974,6 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) mdp5_kms->dev = dev; mdp5_kms->pdev = pdev;
- drm_modeset_lock_init(&mdp5_kms->state_lock); - mdp5_kms->state = kzalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); - if (!mdp5_kms->state) { - ret = -ENOMEM; - goto fail; - } - ret = mdp5_global_obj_init(mdp5_kms); if (ret) goto fail; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h index 1c70aac20a98..a75fbd3aeaf7 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h @@ -30,8 +30,6 @@ #include "mdp5_ctl.h" #include "mdp5_smp.h"
-struct mdp5_state; - struct mdp5_kms { struct mdp_kms base;
@@ -51,12 +49,6 @@ struct mdp5_kms { struct mdp5_cfg_handler *cfg; uint32_t caps; /* MDP capabilities (MDP_CAP_XXX bits) */
- /** - * Global atomic state. Do not access directly, use mdp5_get_state() - */ - struct mdp5_state *state; - struct drm_modeset_lock state_lock; - /* * Global private object state, Do not access directly, use * mdp5_global_get_state() @@ -89,20 +81,6 @@ struct mdp5_kms { }; #define to_mdp5_kms(x) container_of(x, struct mdp5_kms, base)
-/* Global atomic state for tracking resources that are shared across - * multiple kms objects (planes/crtcs/etc). - * - * For atomic updates which require modifying global state, - */ -struct mdp5_state { - struct mdp5_hw_pipe_state hwpipe; - struct mdp5_hw_mixer_state hwmixer; - struct mdp5_smp_state smp; -}; - -struct mdp5_state *__must_check -mdp5_get_state(struct drm_atomic_state *s); - /* Global private object state for tracking resources that are shared across * multiple kms objects (planes/crtcs/etc). */ diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 9de579e0a1df..761d9bd65302 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -226,11 +226,7 @@ int msm_atomic_commit(struct drm_device *dev, * This is the point of no return - everything below never fails except * when the hw goes bonghits. Which means we can commit the new state on * the software side now. - * - * swap driver private state while still holding state_lock */ - if (to_kms_state(state)->state) - priv->kms->funcs->swap_state(priv->kms, state);
/* * Everything below can be run asynchronously without the need to grab @@ -264,30 +260,3 @@ int msm_atomic_commit(struct drm_device *dev, drm_atomic_helper_cleanup_planes(dev, state); return ret; } - -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev) -{ - struct msm_kms_state *state = kzalloc(sizeof(*state), GFP_KERNEL); - - if (!state || drm_atomic_state_init(dev, &state->base) < 0) { - kfree(state); - return NULL; - } - - return &state->base; -} - -void msm_atomic_state_clear(struct drm_atomic_state *s) -{ - struct msm_kms_state *state = to_kms_state(s); - drm_atomic_state_default_clear(&state->base); - kfree(state->state); - state->state = NULL; -} - -void msm_atomic_state_free(struct drm_atomic_state *state) -{ - kfree(to_kms_state(state)->state); - drm_atomic_state_default_release(state); - kfree(state); -} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 65d3b0bb7c86..a631e94ca731 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -42,9 +42,6 @@ static const struct drm_mode_config_funcs mode_config_funcs = { .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = drm_atomic_helper_check, .atomic_commit = msm_atomic_commit, - .atomic_state_alloc = msm_atomic_state_alloc, - .atomic_state_clear = msm_atomic_state_clear, - .atomic_state_free = msm_atomic_state_free, };
#ifdef CONFIG_DRM_MSM_REGISTER_LOGGING diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h index 7585a6b988c7..c776651eed08 100644 --- a/drivers/gpu/drm/msm/msm_kms.h +++ b/drivers/gpu/drm/msm/msm_kms.h @@ -40,8 +40,6 @@ struct msm_kms_funcs { irqreturn_t (*irq)(struct msm_kms *kms); int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc); void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc); - /* swap global atomic state: */ - void (*swap_state)(struct msm_kms *kms, struct drm_atomic_state *state); /* modeset, bracketing atomic_commit(): */ void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state); void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state); @@ -78,18 +76,6 @@ struct msm_kms { struct msm_gem_address_space *aspace; };
-/** - * Subclass of drm_atomic_state, to allow kms backend to have driver - * private global state. The kms backend can do whatever it wants - * with the ->state ptr. On ->atomic_state_clear() the ->state ptr - * is kfree'd and set back to NULL. - */ -struct msm_kms_state { - struct drm_atomic_state base; - void *state; -}; -#define to_kms_state(x) container_of(x, struct msm_kms_state, base) - static inline void msm_kms_init(struct msm_kms *kms, const struct msm_kms_funcs *funcs) {
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst -
Rob Clark -
Ville Syrjälä