From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

It is necessary to track states for objects other than connector, crtc and plane for atomic modesets. But adding objects like DP MST link bandwidth to drm_atomic_state would mean that a non-core object will be modified by the core helper functions for swapping and clearing it's state. So, lets add void * objects and helper functions that operate on void * types to keep these objects and states private to the core. Drivers can then implement specific functions to swap and clear states. The other advantage having just void * for these objects in drm_atomic_state is that objects of different types can be managed in the same state array.

v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) v3: Macro alignment (Chris) v2: Added docs and new iterator to filter private objects (Daniel)

Acked-by: Harry Wentland harry.wentland@amd.com Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..e590148 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state) kfree(state->connectors); kfree(state->crtcs); kfree(state->planes); + kfree(state->private_objs); } EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->planes[i].ptr = NULL; state->planes[i].state = NULL; } + + for (i = 0; i < state->num_private_objs; i++) { + void *private_obj = state->private_objs[i].obj; + void *obj_state = state->private_objs[i].obj_state; + + if (!private_obj) + continue; + + state->private_objs[i].funcs->destroy_state(obj_state); + state->private_objs[i].obj = NULL; + state->private_objs[i].obj_state = NULL; + state->private_objs[i].funcs = NULL; + } + state->num_private_objs = 0; + } EXPORT_SYMBOL(drm_atomic_state_default_clear);

@@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, }

/** + * drm_atomic_get_private_obj_state - get private object state + * @state: global atomic state + * @obj: private object to get the state for + * @funcs: pointer to the struct of function pointers that identify the object + * type + * + * This function returns the private object state for the given private object, + * allocating the state if needed. It does not grab any locks as the caller is + * expected to care of any required locking. + * + * RETURNS: + * + * Either the allocated state or the error code encoded into a pointer. + */ +void * +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj, + const struct drm_private_state_funcs *funcs) +{ + int index, num_objs, i; + size_t size; + struct __drm_private_objs_state *arr; + + for (i = 0; i < state->num_private_objs; i++) + if (obj == state->private_objs[i].obj && + state->private_objs[i].obj_state) + return state->private_objs[i].obj_state; + + num_objs = state->num_private_objs + 1; + size = sizeof(*state->private_objs) * num_objs; + arr = krealloc(state->private_objs, size, GFP_KERNEL); + if (!arr) + return ERR_PTR(-ENOMEM); + + state->private_objs = arr; + index = state->num_private_objs; + memset(&state->private_objs[index], 0, sizeof(*state->private_objs)); + + state->private_objs[index].obj_state = funcs->duplicate_state(state, obj); + if (!state->private_objs[index].obj_state) + return ERR_PTR(-ENOMEM); + + state->private_objs[index].obj = obj; + state->private_objs[index].funcs = funcs; + state->num_private_objs = num_objs; + + DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n", + state->private_objs[index].obj_state, state); + + return state->private_objs[index].obj_state; +} +EXPORT_SYMBOL(drm_atomic_get_private_obj_state); + +/** * drm_atomic_get_connector_state - get connector state * @state: global atomic state object * @connector: connector to get state object for diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4e26b73..1403334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit; + void *obj, *obj_state; + const struct drm_private_state_funcs *funcs;

if (stall) { for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { @@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, state->planes[i].state = old_plane_state; plane->state = new_plane_state; } + + __for_each_private_obj(state, obj, obj_state, i, funcs) + funcs->swap_state(obj, &state->private_objs[i].obj_state); } EXPORT_SYMBOL(drm_atomic_helper_swap_state);

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 0147a04..c17da39 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -155,6 +155,53 @@ struct __drm_connnectors_state { };

/** + * struct drm_private_state_funcs - atomic state functions for private objects + * + * These hooks are used by atomic helpers to create, swap and destroy states of + * private objects. The structure itself is used as a vtable to identify the + * associated private object type. Each private object type that needs to be + * added to the atomic states is expected to have an implementation of these + * hooks and pass a pointer to it's drm_private_state_funcs struct to + * drm_atomic_get_private_obj_state(). + */ +struct drm_private_state_funcs { + /** + * @duplicate_state: + * + * Duplicate the current state of the private object and return it. It + * is an error to call this before obj->state has been initialized. + * + * RETURNS: + * + * Duplicated atomic state or NULL when obj->state is not + * initialized or allocation failed. + */ + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj); + + /** + * @swap_state: + * + * This function swaps the existing state of a private object @obj with + * it's newly created state, the pointer to which is passed as + * @obj_state_ptr. + */ + void (*swap_state)(void *obj, void **obj_state_ptr); + + /** + * @destroy_state: + * + * Frees the private object state created with @duplicate_state. + */ + void (*destroy_state)(void *obj_state); +}; + +struct __drm_private_objs_state { + void *obj; + void *obj_state; + const struct drm_private_state_funcs *funcs; +}; + +/** * struct drm_atomic_state - the global state object for atomic updates * @ref: count of all references to this state (will not be freed until zero) * @dev: parent DRM device @@ -165,6 +212,8 @@ struct __drm_connnectors_state { * @crtcs: pointer to array of CRTC pointers * @num_connector: size of the @connectors and @connector_states arrays * @connectors: pointer to array of structures with per-connector data + * @num_private_objs: size of the @private_objs array + * @private_objs: pointer to array of private object pointers * @acquire_ctx: acquire context for this atomic modeset state update */ struct drm_atomic_state { @@ -178,6 +227,8 @@ struct drm_atomic_state { struct __drm_crtcs_state *crtcs; int num_connector; struct __drm_connnectors_state *connectors; + int num_private_objs; + struct __drm_private_objs_state *private_objs;

struct drm_modeset_acquire_ctx *acquire_ctx;

@@ -270,6 +321,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, struct drm_connector_state *state, struct drm_property *property, uint64_t val);

+void * __must_check +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, + void *obj, + const struct drm_private_state_funcs *funcs); + /** * drm_atomic_get_existing_crtc_state - get crtc state, if it exists * @state: global atomic state object @@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (plane)

/** + * __for_each_private_obj - iterate over all private objects + * @__state: the atomic state + * + * This macro iterates over the array containing private object data in atomic + * state + */ +#define __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \ + for ((__i) = 0; \ + (__i) < (__state)->num_private_objs && \ + ((obj) = (__state)->private_objs[__i].obj, \ + (__funcs) = (__state)->private_objs[__i].funcs, \ + (obj_state) = (__state)->private_objs[__i].obj_state, \ + 1); \ + (__i)++) \ + for_each_if (__funcs) + +/** + * for_each_private_obj - iterate over a specify type of private object + * @__state: the atomic state + * @obj_funcs: function table to filter the private objects + * + * This macro iterates over the private objects state array while filtering the + * objects based on the vfunc table that is passed as @obj_funcs. New macros + * can be created by passing in the vfunc table associated with a specific + * private object. + */ +#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs) \ + for ((__i) = 0; \ + (__i) < (__state)->num_private_objs && \ + ((obj) = (__state)->private_objs[__i].obj, \ + (__funcs) = (__state)->private_objs[__i].funcs, \ + (obj_state) = (__state)->private_objs[__i].obj_state, \ + 1); \ + (__i)++) \ + for_each_if (__funcs == obj_funcs) + +/** * drm_atomic_crtc_needs_modeset - compute combined modeset need * @state: &drm_crtc_state for the CRTC *

On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:

From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

It is necessary to track states for objects other than connector, crtc and plane for atomic modesets. But adding objects like DP MST link bandwidth to drm_atomic_state would mean that a non-core object will be modified by the core helper functions for swapping and clearing it's state. So, lets add void * objects and helper functions that operate on void * types to keep these objects and states private to the core. Drivers can then implement specific functions to swap and clear states. The other advantage having just void * for these objects in drm_atomic_state is that objects of different types can be managed in the same state array.

v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) v3: Macro alignment (Chris) v2: Added docs and new iterator to filter private objects (Daniel)

Acked-by: Harry Wentland harry.wentland@amd.com Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..e590148 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state) kfree(state->connectors); kfree(state->crtcs); kfree(state->planes);

• kfree(state->private_objs);

} EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->planes[i].ptr = NULL; state->planes[i].state = NULL; }

• for (i = 0; i < state->num_private_objs; i++) {

• void *private_obj = state->private_objs[i].obj;
  

• void *obj_state = state->private_objs[i].obj_state;
  

• if (!private_obj)
  

• 	continue;
  

• state->private_objs[i].funcs->destroy_state(obj_state);
  

• state->private_objs[i].obj = NULL;
  

• state->private_objs[i].obj_state = NULL;
  

• state->private_objs[i].funcs = NULL;
  

• }
• state->num_private_objs = 0;

Here we set num_private_objs = 0;

} EXPORT_SYMBOL(drm_atomic_state_default_clear);

@@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, }

/**

• • drm_atomic_get_private_obj_state - get private object state
• • @state: global atomic state
• • @obj: private object to get the state for
• • @funcs: pointer to the struct of function pointers that identify the object
• • type
• • This function returns the private object state for the given private object,
• • allocating the state if needed. It does not grab any locks as the caller is
• • expected to care of any required locking.
• • RETURNS:
• • Either the allocated state or the error code encoded into a pointer.
• */

+void * +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,

• 	      const struct drm_private_state_funcs *funcs)
  

+{

• int index, num_objs, i;
• size_t size;
• struct __drm_private_objs_state *arr;
• for (i = 0; i < state->num_private_objs; i++)

• if (obj == state->private_objs[i].obj &&
  

•     state->private_objs[i].obj_state)
  

• 	return state->private_objs[i].obj_state;
  

• num_objs = state->num_private_objs + 1;
• size = sizeof(*state->private_objs) * num_objs;
• arr = krealloc(state->private_objs, size, GFP_KERNEL);

But here we unconditionally realloc to a presumably smaller size. If you look at drm_atomic_state->num_connector (which also does dynamic array realloc), that one works a bit differently (and hence needs these NULL checks).

I think aligning with how we do things with connectors, for consistency (no other reason really) would be good.

Just noticed this while reading Maarten's review, which seems to go even farther away from how we handle this for connectors. -Daniel

On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:

Op 27-03-17 om 08:38 schreef Daniel Vetter:

...

On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:

...

From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

It is necessary to track states for objects other than connector, crtc and plane for atomic modesets. But adding objects like DP MST link bandwidth to drm_atomic_state would mean that a non-core object will be modified by the core helper functions for swapping and clearing it's state. So, lets add void * objects and helper functions that operate on void * types to keep these objects and states private to the core. Drivers can then implement specific functions to swap and clear states. The other advantage having just void * for these objects in drm_atomic_state is that objects of different types can be managed in the same state array.

v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) v3: Macro alignment (Chris) v2: Added docs and new iterator to filter private objects (Daniel)

Acked-by: Harry Wentland harry.wentland@amd.com Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..e590148 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state) kfree(state->connectors); kfree(state->crtcs); kfree(state->planes);

• kfree(state->private_objs);

} EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->planes[i].ptr = NULL; state->planes[i].state = NULL; }

• for (i = 0; i < state->num_private_objs; i++) {

• void *private_obj = state->private_objs[i].obj;
  

• void *obj_state = state->private_objs[i].obj_state;
  

• if (!private_obj)
  

• 	continue;
  

• state->private_objs[i].funcs->destroy_state(obj_state);
  

• state->private_objs[i].obj = NULL;
  

• state->private_objs[i].obj_state = NULL;
  

• state->private_objs[i].funcs = NULL;
  

• }
• state->num_private_objs = 0;

Here we set num_private_objs = 0;

...

} EXPORT_SYMBOL(drm_atomic_state_default_clear);

@@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, }

/**

• • drm_atomic_get_private_obj_state - get private object state
• • @state: global atomic state
• • @obj: private object to get the state for
• • @funcs: pointer to the struct of function pointers that identify the object
• • type
• • This function returns the private object state for the given private object,
• • allocating the state if needed. It does not grab any locks as the caller is
• • expected to care of any required locking.
• • RETURNS:
• • Either the allocated state or the error code encoded into a pointer.
• */

+void * +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,

• 	      const struct drm_private_state_funcs *funcs)
  

+{

• int index, num_objs, i;
• size_t size;
• struct __drm_private_objs_state *arr;
• for (i = 0; i < state->num_private_objs; i++)

• if (obj == state->private_objs[i].obj &&
  

•     state->private_objs[i].obj_state)
  

• 	return state->private_objs[i].obj_state;
  

• num_objs = state->num_private_objs + 1;
• size = sizeof(*state->private_objs) * num_objs;
• arr = krealloc(state->private_objs, size, GFP_KERNEL);

But here we unconditionally realloc to a presumably smaller size. If you look at drm_atomic_state->num_connector (which also does dynamic array realloc), that one works a bit differently (and hence needs these NULL checks).

I think aligning with how we do things with connectors, for consistency (no other reason really) would be good.

Just noticed this while reading Maarten's review, which seems to go even farther away from how we handle this for connectors. -Daniel

Connectors are handled differently, because there's a fixed number of connectors and each connector is assigned to its slot at state->connectors[drm_connector_index];

For private objects this is not the case, there's no way to put them in a fixed index, so the array is resized and reallocated as needed. If you care about the realloc to a smaller size, add a separate variable max_private_objs and multiply its size by 2 every time it's not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).

I don't propose you should though, because N is small enough and the increased complexity isn't worth the decreased readability. So just set num to zero and don't worry about null checks. :)

Hm, in that case shouldn't we also kfree the allocation in default_clear? Makes no sense resetting to 0 and not freeing, when we do an unconditional krealloc afterwards. That's the part that confused me ... I'm not worried about the realloc overahead (and that's easy to fix indeed). -Daniel

On Mon, 2017-03-27 at 10:35 +0200, Maarten Lankhorst wrote:

Op 27-03-17 om 10:31 schreef Daniel Vetter:

...

On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:

...

Op 27-03-17 om 08:38 schreef Daniel Vetter:

...

On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:

...

From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

It is necessary to track states for objects other than connector, crtc and plane for atomic modesets. But adding objects like DP MST link bandwidth to drm_atomic_state would mean that a non-core object will be modified by the core helper functions for swapping and clearing it's state. So, lets add void * objects and helper functions that operate on void * types to keep these objects and states private to the core. Drivers can then implement specific functions to swap and clear states. The other advantage having just void * for these objects in drm_atomic_state is that objects of different types can be managed in the same state array.

v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) v3: Macro alignment (Chris) v2: Added docs and new iterator to filter private objects (Daniel)

Acked-by: Harry Wentland harry.wentland@amd.com Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..e590148 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state) kfree(state->connectors); kfree(state->crtcs); kfree(state->planes);

• kfree(state->private_objs);

} EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->planes[i].ptr = NULL; state->planes[i].state = NULL; }

• for (i = 0; i < state->num_private_objs; i++) {

• void *private_obj = state->private_objs[i].obj;
  

• void *obj_state = state->private_objs[i].obj_state;
  

• if (!private_obj)
  

• 	continue;
  

• state->private_objs[i].funcs->destroy_state(obj_state);
  

• state->private_objs[i].obj = NULL;
  

• state->private_objs[i].obj_state = NULL;
  

• state->private_objs[i].funcs = NULL;
  

• }
• state->num_private_objs = 0;

Here we set num_private_objs = 0;

...

} EXPORT_SYMBOL(drm_atomic_state_default_clear);

@@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, }

/**

• • drm_atomic_get_private_obj_state - get private object state
• • @state: global atomic state
• • @obj: private object to get the state for
• • @funcs: pointer to the struct of function pointers that identify the object
• • type
• • This function returns the private object state for the given private object,
• • allocating the state if needed. It does not grab any locks as the caller is
• • expected to care of any required locking.
• • RETURNS:
• • Either the allocated state or the error code encoded into a pointer.
• */

+void * +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,

• 	      const struct drm_private_state_funcs *funcs)
  

+{

• int index, num_objs, i;
• size_t size;
• struct __drm_private_objs_state *arr;
• for (i = 0; i < state->num_private_objs; i++)

• if (obj == state->private_objs[i].obj &&
  

•     state->private_objs[i].obj_state)
  

• 	return state->private_objs[i].obj_state;
  

• num_objs = state->num_private_objs + 1;
• size = sizeof(*state->private_objs) * num_objs;
• arr = krealloc(state->private_objs, size, GFP_KERNEL);

But here we unconditionally realloc to a presumably smaller size. If you look at drm_atomic_state->num_connector (which also does dynamic array realloc), that one works a bit differently (and hence needs these NULL checks).

I think aligning with how we do things with connectors, for consistency (no other reason really) would be good.

Just noticed this while reading Maarten's review, which seems to go even farther away from how we handle this for connectors. -Daniel

Connectors are handled differently, because there's a fixed number of connectors and each connector is assigned to its slot at state->connectors[drm_connector_index];

For private objects this is not the case, there's no way to put them in a fixed index, so the array is resized and reallocated as needed. If you care about the realloc to a smaller size, add a separate variable max_private_objs and multiply its size by 2 every time it's not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).

I don't propose you should though, because N is small enough and the increased complexity isn't worth the decreased readability. So just set num to zero and don't worry about null checks. :)

Hm, in that case shouldn't we also kfree the allocation in default_clear? Makes no sense resetting to 0 and not freeing, when we do an unconditional krealloc afterwards. That's the part that confused me ... I'm not worried about the realloc overahead (and that's easy to fix indeed). -Daniel

We might as well, strictly speaking not needed but probably reduces confusion. :)

kfree'ing the state->private_objs array in drm_atomic_state_default_clear() does not seem consistent with what we do for crtcs, planes and connectors. default_release() seems to be the function that frees memory for state arrays. We could just go with v4, where state->num_private_objs is not reset. As 'n' is small, the NULL checks shouldn't be costly. I can look at optimizing realloc's once we have more consumers for private_objs?

-DK

---

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

On Thu, Mar 16, 2017 at 12:10:30AM -0700, Dhinakaran Pandiyan wrote:

From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

Having a ->atomic_release callback is useful to release shared resources that get allocated in compute_config(). This function is expected to be called in the atomic_check() phase before new resources are acquired.

v3: Use the new 'for_each_oldnew_connector_in_state()' macro. v2: Moved the caller hunk to this patch (Daniel)

Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Archit Taneja architt@codeaurora.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Harry Wentland Harry.wentland@amd.com Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_atomic_helper.c | 19 +++++++++++++++++++ include/drm/drm_modeset_helper_vtables.h | 13 +++++++++++++ 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 1403334..c6c8397 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }

• for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {

• const struct drm_connector_helper_funcs *conn_funcs;
  

• struct drm_crtc_state *crtc_state;
  

• conn_funcs = connector->helper_private;
  

• if (!conn_funcs->atomic_release)
  

• 	continue;
  

• if (!old_connector_state->crtc)
  

• 	continue;
  

• crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
  

• if (crtc_state->connectors_changed ||
  

•     crtc_state->mode_changed ||
  

•     (crtc_state->active_changed && !crtc_state->active))
  

• 	conn_funcs->atomic_release(connector, new_connector_state);
  

• }
• return mode_fixup(state);

} EXPORT_SYMBOL(drm_atomic_helper_check_modeset); diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 091c422..394ec0c 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -836,6 +836,19 @@ struct drm_connector_helper_funcs { */ struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector, struct drm_connector_state *connector_state);

• /**

• * @atomic_release:
  

• *
  

• * This function is used to release shared resources that were
  

• * previously acquired.
  

• *
  

• * NOTE:
  

• *
  

• * This function is called in the check phase of an atomic update.
  

"is _unconditionally_ called" I think that would be a useful clarification. Also, probably good to state that this is called before any atomic_check calls.

With those two nits addressed:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• */
  

• void (*atomic_release)(struct drm_connector *connector,

• 	       struct drm_connector_state *connector_state);
  

};

/**

2.7.4

On Wed, 2017-03-22 at 13:30 +0100, Maarten Lankhorst wrote:

Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:

...

From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

Use the added helpers to track MST link bandwidth for atomic modesets. Link bw is acquired in the ->atomic_check() phase when CRTCs are being enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots(). Similarly, link bw is released during ->atomic_check() with the connector helper callback ->atomic_release() when CRTCs are disabled.

v2: Squashed atomic_release() implementation and caller (Daniel) Fixed logic for connector-crtc switching case (Daniel) Fixed logic for suspend-resume case.

Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Archit Taneja architt@codeaurora.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Harry Wentland Harry.wentland@amd.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index c1f62eb..a8f40fa 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, struct intel_dp *intel_dp = &intel_dig_port->dp; struct intel_connector *connector = to_intel_connector(conn_state->connector);

• struct drm_atomic_state *state;

• struct drm_atomic_state *state = pipe_config->base.state; int bpp;

• int lane_count, slots;

• int lane_count, slots = 0; const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; int mst_pbn;

@@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, * seem to suggest we should do otherwise. */ lane_count = drm_dp_max_lane_count(intel_dp->dpcd);

• pipe_config->lane_count = lane_count;

  pipe_config->pipe_bpp = bpp; pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);

• state = pipe_config->base.state;

• if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port)) pipe_config->has_audio = true;

• mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

• mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp); pipe_config->pbn = mst_pbn;

• slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);

  intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock, &pipe_config->dp_m_n);

• if (pipe_config->base.active) {

• slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
  

• 			      connector->port, mst_pbn);
  

• if (slots < 0) {
  

• 	DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
  

• 	return false;
  

• }
  

• } pipe_config->dp_m_n.tu = slots;

  return true;

+}

+static void intel_dp_mst_atomic_release(struct drm_connector *connector,

• 			struct drm_connector_state *conn_state)
  

+{

• struct intel_dp_mst_encoder *intel_mst;

• struct drm_dp_mst_topology_mgr *mgr;

• struct drm_encoder *encoder;

• struct intel_connector *intel_connector = to_intel_connector(connector);

• struct drm_atomic_state *state = conn_state->state;

• int slots;

• encoder = connector->state->best_encoder;

• intel_mst = enc_to_mst(encoder);

• mgr = &intel_mst->primary->dp.mst_mgr;

• slots = drm_dp_atomic_release_vcpi_slots(state, mgr,

• 				 intel_connector->port);
  

• if (slots < 0)

• DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
  

}

static void intel_mst_disable_dp(struct intel_encoder *encoder, @@ -387,6 +410,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun .mode_valid = intel_dp_mst_mode_valid, .atomic_best_encoder = intel_mst_atomic_best_encoder, .best_encoder = intel_mst_best_encoder,

• .atomic_release = intel_dp_mst_atomic_release,

};

static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)

Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8

Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

Thanks for the review.

If we already had an atomic commit that released the slots, then vcpi.num_slots in 'struct drm_dp_mst_port' would have been reset to 0. The second time, release_vcpi_slots() is called, the topology_state slot tracking won't be updated because it gets the current allocation from per-port structure. So, it should work fine.

-DK

---

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