Link bandwidth is a shared resource between multiple displays in DP MST configurations. For atomic modesetting drivers, checking if there is sufficient link bandwidth for a mode needs to be done during the atomic_check phase to avoid failed modesets when multiple CRTC's and connectors are involved.
Managing shared resources like DP MST link bandwidth in the driver's subclassed atomic_state will result in duplicating the code in each atomic modesetting driver. But adding objects like DP MST link bandwidth to the DRM core's drm_atomic_state would mean that an object that is not a core modesetting object like connector, CRTC or a plane will be modified by the helper functions for swapping and clearing state. So, this series introduces void * type driver-private objects in drm_atomic_state and adds helper functions that operate on these private objects. Drivers can then implement object-specific functions to swap and clear states. The advantage of having void * for these objects in drm_atomic_state is that objects of different types can be managed in the same state array.
This version 1) splits and squashes patches 2) adds documentation 3) fixes vcpi slot accounting logic for suspend-resume and connector switching
Dhinakaran Pandiyan (8): drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr drm/dp: Kill unused MST vcpi slot availability tracking drm/dp: Split drm_dp_mst_allocate_vcpi drm: Add driver-private objects to atomic state drm/dp: Introduce MST topology state to track available link bandwidth drm/dp: Add DP MST helpers to atomically find and release vcpi slots drm: Connector helper function to release resources drm/dp: Track MST link bandwidth
drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 24 ++++ drivers/gpu/drm/drm_dp_mst_topology.c | 181 ++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_dp_mst.c | 42 +++++-- drivers/gpu/drm/nouveau/nv50_display.c | 3 +- drivers/gpu/drm/radeon/radeon_dp_mst.c | 4 +- include/drm/drm_atomic.h | 91 ++++++++++++++++ include/drm/drm_dp_mst_helper.h | 33 ++++-- include/drm/drm_modeset_helper_vtables.h | 13 +++ 9 files changed, 421 insertions(+), 38 deletions(-)
The total vcpi time slots is always 63 and does not depend on the link BW, remove total_slots from MST topology manager struct. The next change is to remove total_pbn which is hardcoded to 2560. The total PBN that the topology manager allocates from depends on the link rate and is not a constant. So, fix this by removing the total_pbn member itself.
Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++--- include/drm/drm_dp_mst_helper.h | 9 +-------- 2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 122a1b0..cf4b866 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2042,9 +2042,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms goto out_unlock; }
- mgr->total_pbn = 2560; - mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div); - mgr->avail_slots = mgr->total_slots; + /* max. time slots - one slot for MTP header */ + mgr->avail_slots = 63;
/* add initial branch device at LCT 1 */ mstb = drm_dp_add_mst_branch_device(1, NULL); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index f4b4d15..1a7e0f4 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -479,18 +479,11 @@ struct drm_dp_mst_topology_mgr { * @pbn_div: PBN to slots divisor. */ int pbn_div; - /** - * @total_slots: Total slots that can be allocated. - */ - int total_slots; + /** * @avail_slots: Still available slots that can be allocated. */ int avail_slots; - /** - * @total_pbn: Total PBN count. - */ - int total_pbn;
/** * @qlock: protects @tx_msg_downq, the &drm_dp_mst_branch.txslost and
The avail_slots member in the MST topology manager is never updated to reflect the available vcpi slots. The check is effectively against total slots, 63. So, let's make that check obvious and remove avail_slots. While at it, make debug messages more descriptive.
Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 15 ++++++++------- include/drm/drm_dp_mst_helper.h | 5 ----- 2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index cf4b866..d9edd84 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2042,9 +2042,6 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms goto out_unlock; }
- /* max. time slots - one slot for MTP header */ - mgr->avail_slots = 63; - /* add initial branch device at LCT 1 */ mstb = drm_dp_add_mst_branch_device(1, NULL); if (mstb == NULL) { @@ -2474,7 +2471,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
- if (num_slots > mgr->avail_slots) + /* max. time slots - one slot for MTP header */ + if (num_slots > 63) return -ENOSPC; return num_slots; } @@ -2488,7 +2486,8 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
- if (num_slots > mgr->avail_slots) + /* max. time slots - one slot for MTP header */ + if (num_slots > 63) return -ENOSPC;
vcpi->pbn = pbn; @@ -2527,10 +2526,12 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn); if (ret) { - DRM_DEBUG_KMS("failed to init vcpi %d %d %d\n", DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->avail_slots, ret); + DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n", + DIV_ROUND_UP(pbn, mgr->pbn_div), ret); goto out; } - DRM_DEBUG_KMS("initing vcpi for %d %d\n", pbn, port->vcpi.num_slots); + DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", + pbn, port->vcpi.num_slots); *slots = port->vcpi.num_slots;
drm_dp_put_port(port); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 1a7e0f4..d836511 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -481,11 +481,6 @@ struct drm_dp_mst_topology_mgr { int pbn_div;
/** - * @avail_slots: Still available slots that can be allocated. - */ - int avail_slots; - - /** * @qlock: protects @tx_msg_downq, the &drm_dp_mst_branch.txslost and * &drm_dp_sideband_msg_tx.state once they are queued */
drm_dp_mst_allocate_vcpi() apart from setting up the vcpi structure, also finds if there are enough slots available. This check is a duplicate of that implemented in drm_dp_mst_find_vcpi_slots(). Let's move this check out and reuse the existing drm_dp_mst_find_vcpi_slots() function to check if there are enough vcpi slots before allocating them.
This brings the check to one place. Additionally drivers that will use MST state tracking for atomic modesets can use the atomic version of find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()
Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 21 ++++++++++----------- drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++-- drivers/gpu/drm/nouveau/nv50_display.c | 3 ++- drivers/gpu/drm/radeon/radeon_dp_mst.c | 4 +++- include/drm/drm_dp_mst_helper.h | 3 ++- 5 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index d9edd84..37482b7 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2479,20 +2479,17 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, EXPORT_SYMBOL(drm_dp_find_vcpi_slots);
static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, - struct drm_dp_vcpi *vcpi, int pbn) + struct drm_dp_vcpi *vcpi, int pbn, int slots) { - int num_slots; int ret;
- num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div); - /* max. time slots - one slot for MTP header */ - if (num_slots > 63) + if (slots > 63) return -ENOSPC;
vcpi->pbn = pbn; - vcpi->aligned_pbn = num_slots * mgr->pbn_div; - vcpi->num_slots = num_slots; + vcpi->aligned_pbn = slots * mgr->pbn_div; + vcpi->num_slots = slots;
ret = drm_dp_mst_assign_payload_id(mgr, vcpi); if (ret < 0) @@ -2507,7 +2504,8 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, * @pbn: payload bandwidth number to request * @slots: returned number of slots for this PBN. */ -bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int *slots) +bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port, int pbn, int slots) { int ret;
@@ -2515,16 +2513,18 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp if (!port) return false;
+ if (slots < 0) + return false; + if (port->vcpi.vcpi > 0) { DRM_DEBUG_KMS("payload: vcpi %d already allocated for pbn %d - requested pbn %d\n", port->vcpi.vcpi, port->vcpi.pbn, pbn); if (pbn == port->vcpi.pbn) { - *slots = port->vcpi.num_slots; drm_dp_put_port(port); return true; } }
- ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn); + ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots); if (ret) { DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n", DIV_ROUND_UP(pbn, mgr->pbn_div), ret); @@ -2532,7 +2532,6 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp } DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", pbn, port->vcpi.num_slots); - *slots = port->vcpi.num_slots;
drm_dp_put_port(port); return true; diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 6a85d38..9b9dda8 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -149,7 +149,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, to_intel_connector(conn_state->connector); int ret; uint32_t temp; - int slots;
/* MST encoders are bound to a crtc, not to a connector, * force the mapping here for get_hw_state. @@ -179,7 +178,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr, connector->port, - pipe_config->pbn, &slots); + pipe_config->pbn, + pipe_config->dp_m_n.tu); if (ret == false) { DRM_ERROR("failed to allocate vcpi\n"); return; diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index cd75413..6bfdf47 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -2895,7 +2895,8 @@ nv50_msto_enable(struct drm_encoder *encoder) if (WARN_ON(!mstc)) return;
- r = drm_dp_mst_allocate_vcpi(&mstm->mgr, mstc->port, mstc->pbn, &slots); + slots = drm_dp_find_vcpi_slots(&mstm->mgr, mstc->pbn); + r = drm_dp_mst_allocate_vcpi(&mstm->mgr, mstc->port, mstc->pbn, slots); WARN_ON(!r);
if (mstm->outp->dcb->sorconf.link & 1) diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c index 7d5ada3..6598306 100644 --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c @@ -453,9 +453,11 @@ radeon_mst_encoder_dpms(struct drm_encoder *encoder, int mode) DRM_DEBUG_KMS("dig encoder is %d %d %d\n", dig_enc->dig_encoder, dig_enc->linkb, radeon_crtc->crtc_id);
+ slots = drm_dp_find_vcpi_slots(&radeon_connector->mst_port->mst_mgr, + mst_enc->pbn); ret = drm_dp_mst_allocate_vcpi(&radeon_connector->mst_port->mst_mgr, radeon_connector->port, - mst_enc->pbn, &slots); + mst_enc->pbn, slots); ret = drm_dp_update_payload_part1(&radeon_connector->mst_port->mst_mgr);
radeon_dp_mst_set_be_cntl(primary, mst_enc, diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index d836511..5b02476 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -567,7 +567,8 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_ int drm_dp_calc_pbn_mode(int clock, int bpp);
-bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int *slots); +bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port, int pbn, int slots);
int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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; + } + } EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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 3a4383f..8795088 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *plane_state; struct drm_crtc_commit *commit; + void *obj, *obj_state; + const struct drm_private_state_funcs *funcs;
if (stall) { for_each_crtc_in_state(state, crtc, crtc_state, i) { @@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, swap(state->planes[i].state, plane->state); plane->state->state = NULL; } + + __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 052ab16..cafa404 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 @@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (plane_state)
/** + * __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, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
+#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); \
Align to ( and put the trailing 1 on its own line so it stands out.
(__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 Thu, 2017-02-09 at 08:08 +0000, Chris Wilson wrote:
On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
+#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); \Align to ( and put the trailing 1 on its own line so it stands out.
Sure, will do that. Looks like I have to change other macros in that file too.
-DK
(__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
Hi,
On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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;
Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
- 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 3a4383f..8795088 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *plane_state; struct drm_crtc_commit *commit;
void *obj, *obj_state;
const struct drm_private_state_funcs *funcs;
if (stall) { for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, swap(state->planes[i].state, plane->state); plane->state->state = NULL; }
- __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 052ab16..cafa404 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
@@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (plane_state)
/**
- __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, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Hi,
On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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;Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
-DK
- 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 3a4383f..8795088 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *plane_state; struct drm_crtc_commit *commit;
void *obj, *obj_state;
const struct drm_private_state_funcs *funcs;
if (stall) { for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, swap(state->planes[i].state, plane->state); plane->state->state = NULL; }
- __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 052ab16..cafa404 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
@@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (plane_state)
/**
- __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 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Hi,
On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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;Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock?
Thanks, Archit
On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Hi,
On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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;Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock?
Thanks, Archit
That's right, the connection_mutex takes care of the locking for the MST private state. I can add that as a comment to the caller's (MST helper) kernel doc with a
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-DK
On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:
On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Hi,
On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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;Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock?
Thanks, Archit
That's right, the connection_mutex takes care of the locking for the MST private state. I can add that as a comment to the caller's (MST helper) kernel doc with a
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
That would be nice to have.
In the comment: "It does not grab any locks as the caller is expected to care of any required locking.", you could maybe be a bit more specific and rephrase it as "the caller needs to grab the &drm_modeset_lock responsible for protecting the private object state"
Thanks, Archit
-DK
On Wed, 2017-02-22 at 09:59 +0530, Archit Taneja wrote:
On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:
On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Hi,
On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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;Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock?
Thanks, Archit
That's right, the connection_mutex takes care of the locking for the MST private state. I can add that as a comment to the caller's (MST helper) kernel doc with a
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
That would be nice to have.
In the comment: "It does not grab any locks as the caller is expected to care of any required locking.", you could maybe be a bit more specific and rephrase it as "the caller needs to grab the &drm_modeset_lock responsible for protecting the private object state"
Thanks, Archit
The core leaves it to the drivers to choose the private-object types to add. So, I believe the core should do not mandate the use of modeset_locks. MST happens to be one example where connection_mutex is the appropriate lock.
-DK
-DK
On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote:
On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock?
Thanks, Archit
That's right, the connection_mutex takes care of the locking for the MST private state. I can add that as a comment to the caller's (MST helper) kernel doc with a
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
Please don't add this as a comment, but as real code so it is checked at runtime :-) Personally I wouldn't mention locking rules in kernel-doc, that part tends to get outdated fast. Better to enforce with runtime-checks. -Daniel
On Sun, 2017-02-26 at 20:57 +0100, Daniel Vetter wrote:
On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote:
On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I don't understand the locking stuff toowell, I just noticed this difference when comparing this approach with what is done in the msm kms driver (where we have subclassed drm_atomic_state to msm_kms_state).
Thanks, Archit
The caller is expected to take care of any required locking. The driver-private objects are opaque from core's pov, so the core is not aware of necessary locks for that object type.
I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock?
Thanks, Archit
That's right, the connection_mutex takes care of the locking for the MST private state. I can add that as a comment to the caller's (MST helper) kernel doc with a
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
Please don't add this as a comment, but as real code so it is checked at runtime :-) Personally I wouldn't mention locking rules in kernel-doc, that part tends to get outdated fast. Better to enforce with runtime-checks. -Daniel
That's what I meant but evidently didn't type it that way:) I was going to add that the connection_mutex does the locking for MST state as a comment and put the WARN_ON() in code.
-DK
IRC acked by Harry Wentland
"<hwentlan> dhnkrn, the patch for driver-private atomic state object makes sense to me. Didn't realize that's the same one from early February. Feel free to add my Acked-by"
-DK On Wed, 2017-02-08 at 22:38 -0800, Dhinakaran Pandiyan wrote:
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.
v2: Added docs and new iterator to filter private objects (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a567310..1a9ffe8 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,20 @@ 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;- }
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -974,6 +989,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 3a4383f..8795088 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *plane_state; struct drm_crtc_commit *commit;
void *obj, *obj_state;
const struct drm_private_state_funcs *funcs;
if (stall) { for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, swap(state->planes[i].state, plane->state); plane->state->state = NULL; }
- __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 052ab16..cafa404 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
@@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (plane_state)
/**
- __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
Link bandwidth is shared between multiple display streams in DP MST configurations. The DP MST topology manager structure maintains the shared link bandwidth for a primary link directly connected to the GPU. For atomic modesetting drivers, checking if there is sufficient link bandwidth for a mode needs to be done during the atomic_check phase to avoid failed modesets. Let's encapsulate the available link bw information in a private state structure so that bw can be allocated and released atomically for each of the ports sharing the primary link.
v2: Included kernel doc, moved state initialization and switched to kmemdup() for allocation (Daniel)
Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 71 +++++++++++++++++++++++++++++++++++ include/drm/drm_dp_mst_helper.h | 20 ++++++++++ 2 files changed, 91 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 37482b7..a891c36 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2936,6 +2936,65 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) (*mgr->cbs->hotplug)(mgr); }
+void *drm_dp_mst_duplicate_state(struct drm_atomic_state *state, void *obj) +{ + struct drm_dp_mst_topology_mgr *mgr = obj; + struct drm_dp_mst_topology_state *new_mst_state; + + if (WARN_ON(!mgr->state)) + return NULL; + + new_mst_state = kmemdup(mgr->state, sizeof(*new_mst_state), GFP_KERNEL); + if (new_mst_state) + new_mst_state->state = state; + return new_mst_state; +} + +void drm_dp_mst_swap_state(void *obj, void **obj_state_ptr) +{ + struct drm_dp_mst_topology_mgr *mgr = obj; + struct drm_dp_mst_topology_state **topology_state_ptr; + + topology_state_ptr = (struct drm_dp_mst_topology_state **)obj_state_ptr; + + mgr->state->state = (*topology_state_ptr)->state; + swap(*topology_state_ptr, mgr->state); + mgr->state->state = NULL; +} + +void drm_dp_mst_destroy_state(void *obj_state) +{ + kfree(obj_state); +} + +static const struct drm_private_state_funcs mst_state_funcs = { + .duplicate_state = drm_dp_mst_duplicate_state, + .swap_state = drm_dp_mst_swap_state, + .destroy_state = drm_dp_mst_destroy_state, +}; + +/** + * drm_atomic_get_mst_topology_state: get MST topology state + * + * @state: global atomic state + * @mgr: MST topology manager, also the private object in this case + * + * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic + * state vtable so that the private object state returned is that of a MST + * topology object. + * + * RETURNS: + * + * The MST topology state or error pointer. + */ +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state, + struct drm_dp_mst_topology_mgr *mgr) +{ + return drm_atomic_get_private_obj_state(state, mgr, + &mst_state_funcs); +} +EXPORT_SYMBOL(drm_atomic_get_mst_topology_state); + /** * drm_dp_mst_topology_mgr_init - initialise a topology manager * @mgr: manager struct to initialise @@ -2980,6 +3039,15 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, if (test_calc_pbn_mode() < 0) DRM_ERROR("MST PBN self-test failed\n");
+ mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL); + if (mgr->state == NULL) + return -ENOMEM; + mgr->state->mgr = mgr; + + /* max. time slots - one slot for MTP header */ + mgr->state->avail_slots = 63; + mgr->funcs = &mst_state_funcs; + return 0; } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init); @@ -3000,6 +3068,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mutex_unlock(&mgr->payload_lock); mgr->dev = NULL; mgr->aux = NULL; + kfree(mgr->state); + mgr->state = NULL; + mgr->funcs = NULL; } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 5b02476..0b371df 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -24,6 +24,7 @@
#include <linux/types.h> #include <drm/drm_dp_helper.h> +#include <drm/drm_atomic.h>
struct drm_dp_mst_branch;
@@ -403,6 +404,12 @@ struct drm_dp_payload { int vcpi; };
+struct drm_dp_mst_topology_state { + int avail_slots; + struct drm_atomic_state *state; + struct drm_dp_mst_topology_mgr *mgr; +}; + /** * struct drm_dp_mst_topology_mgr - DisplayPort MST manager * @@ -481,6 +488,16 @@ struct drm_dp_mst_topology_mgr { int pbn_div;
/** + * @state: State information for topology manager + */ + struct drm_dp_mst_topology_state *state; + + /** + * @funcs: Atomic helper callbacks + */ + const struct drm_private_state_funcs *funcs; + + /** * @qlock: protects @tx_msg_downq, the &drm_dp_mst_branch.txslost and * &drm_dp_sideband_msg_tx.state once they are queued */ @@ -596,4 +613,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr); int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr); +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state, + struct drm_dp_mst_topology_mgr *mgr); + #endif
drm_dp_atomic_find_vcpi_slots() should be called from ->atomic_check() to check there are sufficient vcpi slots for a mode and to add that to the state. This should be followed by a call to drm_dp_mst_allocate_vcpi() in ->atomic_commit() to initialize a struct vcpi for the port.
drm_dp_atomic_release_vcpi_slots() should be called from ->atomic_check() to release a port's vcpi slot allocation from the state.
Drivers that do not make use of this atomic helper are expected to call drm_dp_find_vcpi_slots() instead before calling drm_dp_mst_allocate_vcpi().
v2: Added checks for verifying the port reference is valid Moved get_mst_topology_state() into the helpers (Daniel) Changed find_vcpi_slots() to not depend on current allocation
Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 75 +++++++++++++++++++++++++++++++++++ include/drm/drm_dp_mst_helper.h | 6 +++ 2 files changed, 81 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index a891c36..93de257 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2498,6 +2498,81 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, }
/** + * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state + * @state: global atomic state + * @mgr: MST topology manager for the port + * @port: port to find vcpi slots for + * @pbn: bandwidth required for the mode in PBN + * + * RETURNS: + * Total slots in the atomic state assigned for this port or error + */ +int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port, int pbn) +{ + struct drm_dp_mst_topology_state *topology_state; + int req_slots; + + topology_state = drm_atomic_get_mst_topology_state(state, mgr); + if (topology_state == NULL) + return -ENOMEM; + + port = drm_dp_get_validated_port_ref(mgr, port); + if (port == NULL) + return -EINVAL; + req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div); + DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n", + req_slots, topology_state->avail_slots); + + if (req_slots > topology_state->avail_slots) { + drm_dp_put_port(port); + return -ENOSPC; + } + + topology_state->avail_slots -= req_slots; + DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots); + + drm_dp_put_port(port); + return req_slots; +} +EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); + +/** + * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots + * @state: global atomic state + * @mgr: MST topology manager for the port + * @port: port to release the vcpi slots for + * + * RETURNS: + * Number of slots released from the atomic state for this port + */ +int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port) +{ + struct drm_dp_mst_topology_state *topology_state; + int curr_slots; + + topology_state = drm_atomic_get_mst_topology_state(state, mgr); + if (topology_state == NULL) + return -ENOMEM; + + port = drm_dp_get_validated_port_ref(mgr, port); + if (port == NULL) + return -EINVAL; + + curr_slots = port->vcpi.num_slots; + topology_state->avail_slots += curr_slots; + DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n", + curr_slots, topology_state->avail_slots); + + drm_dp_put_port(port); + return curr_slots; +} +EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots); + +/** * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel * @mgr: manager for this port * @port: port to allocate a virtual channel for. diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 0b371df..64e7dac 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -615,5 +615,11 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr); int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr); struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state, struct drm_dp_mst_topology_mgr *mgr); +int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port, int pbn); +int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port);
#endif
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
+ for_each_connector_in_state(state, connector, 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 (!connector->state->crtc) + continue; + + crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc); + + if (crtc_state->connectors_changed || + crtc_state->mode_changed || + (crtc_state->active_changed && !crtc_state->active)) + conn_funcs->atomic_release(connector, 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. + */ + void (*atomic_release)(struct drm_connector *connector, + struct drm_connector_state *connector_state); };
/**
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
- for_each_connector_in_state(state, connector,
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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-active))
conn_funcs->atomic_release(connector,connector_state);
- }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :)
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
- for_each_connector_in_state(state, connector,
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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-active))
conn_funcs->atomic_release(connector,connector_state);
- }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
- for_each_connector_in_state(state, connector,
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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state-
crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-active))
conn_funcs->atomic_release(connector,connector_state);
- }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
- for_each_connector_in_state(state, connector,
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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state-
crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-active))
conn_funcs->atomic_release(connector,connector_state);
- }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
-DK
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2017-02-13 at 21:26 +0000, Pandiyan, Dhinakaran wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
- for_each_connector_in_state(state, connector,
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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state-
crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-active))
conn_funcs->atomic_release(connector,connector_state);
- }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
-DK
This is what I mean -https://pastebin.ubuntu.com/23991405/ But, I do have one concern with calling this conn_func->atomic_check(). We are not validating the new connector_state like atomic_check() seems to do generally but only cleaning up vcpi resources for compute_config() to later acquire. Let me know if I am wrong in my understanding what atomic_check() is expected to do.
-DK
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Pandiyan, Dhinakaran schreef op ma 13-02-2017 om 22:48 [+0000]:
On Mon, 2017-02-13 at 21:26 +0000, Pandiyan, Dhinakaran wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [- 0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int el.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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
- for_each_connector_in_state(state, connector,
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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state- > > crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed &&!crtc_state- > > > active))
conn_funcs->atomic_release(connector, connector_state);
- }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
-DK
This is what I mean -https://pastebin.ubuntu.com/23991405/ But, I do have one concern with calling this conn_func-
atomic_check().
We are not validating the new connector_state like atomic_check() seems to do generally but only cleaning up vcpi resources for compute_config() to later acquire. Let me know if I am wrong in my understanding what atomic_check() is expected to do.
Yeah looks good. I think it makes sense to have such a validation function. There may not be much in it now but that could change when i915 connector properties are made atomic. :)
~Maarten
On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
for_each_connector_in_state(state, connector,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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state-
crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-active))
conn_funcs->atomic_release(connector,connector_state);
}Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
Adding ->atomic_check that's unconditionally called sounds troubling, because all the other ->atomic_check functions are _only_ called when enabling stuff. ->atomic_release sounds much better to me, and from a helper pov DK's patch above is the right place.
If that place doesn't work for i915.ko, then we need our own callback (like we already have with e.g. ->compute_config, we could do a ->release_config). But if it's just cosmetics, then I don't see the reason why we need to change this. On that issue: How exactly does our compute_config work if we haven't updated the routing (using the above helper) yet? That sounds like a pretty big misdesign on our side ... -Daniel
-DK
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2017-02-14 at 20:51 +0100, Daniel Vetter wrote:
On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
for_each_connector_in_state(state, connector,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 (!connector->state->crtc)continue;crtc_state =drm_atomic_get_existing_crtc_state(state, connector->state- >crtc);
if (crtc_state->connectors_changed ||crtc_state->mode_changed ||(crtc_state->active_changed && !crtc_state-> > active))
conn_funcs->atomic_release(connector,connector_state);
}Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
Adding ->atomic_check that's unconditionally called sounds troubling, because all the other ->atomic_check functions are _only_ called when enabling stuff. ->atomic_release sounds much better to me, and from a helper pov DK's patch above is the right place.
If that place doesn't work for i915.ko, then we need our own callback (like we already have with e.g. ->compute_config, we could do a ->release_config). But if it's just cosmetics, then I don't see the reason why we need to change this. On that issue: How exactly does our compute_config work if we haven't updated the routing (using the above helper) yet? That sounds like a pretty big misdesign on our side ... -Daniel
Are you referring to the drm_atomic_helper_check_modeset() helper? We do call it to update connector routing before compute_config .
-DK
-DK
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [- 0800]:
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.
v2: Moved the caller hunk to this patch (Daniel)
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int el.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 8795088..92bd741 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }
+ for_each_connector_in_state(state, connector, 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 (!connector->state->crtc) + continue;
+ crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state- > crtc);
+ if (crtc_state->connectors_changed || + crtc_state->mode_changed || + (crtc_state->active_changed && !crtc_state- > > active))
+ conn_funcs- >atomic_release(connector, connector_state); + }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
Adding ->atomic_check that's unconditionally called sounds troubling, because all the other ->atomic_check functions are _only_ called when enabling stuff. ->atomic_release sounds much better to me, and from a helper pov DK's patch above is the right place.
Having an atomic check would be nice for implementing connector properties. Some of them may need to be validated regardless of crtc.
I would really like to be able to do the validation in atomic_check instead of during the set_property callback. The state is not completely valid at that point yet, so this would be a logical place.
If that place doesn't work for i915.ko, then we need our own callback (like we already have with e.g. ->compute_config, we could do a ->release_config). But if it's just cosmetics, then I don't see the reason why we need to change this. On that issue: How exactly does our compute_config work if we haven't updated the routing (using the above helper) yet? That sounds like a pretty big misdesign on our side ... -Daniel
-DK
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :)
On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [- 0800]: > > 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. > > v2: Moved the caller hunk to this patch (Daniel) > > Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int > el.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 8795088..92bd741 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct > drm_device *dev, > } > } > > + for_each_connector_in_state(state, connector, > 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 (!connector->state->crtc) > + continue; > + > + crtc_state = > drm_atomic_get_existing_crtc_state(state, connector->state- > > crtc); > > + > + if (crtc_state->connectors_changed || > + crtc_state->mode_changed || > + (crtc_state->active_changed && > !crtc_state- > > > > active)) > > + conn_funcs- > >atomic_release(connector, > connector_state); > + }
Could we deal with the VCPI state separately in intel_modeset_checks, like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
Adding ->atomic_check that's unconditionally called sounds troubling, because all the other ->atomic_check functions are _only_ called when enabling stuff. ->atomic_release sounds much better to me, and from a helper pov DK's patch above is the right place.
Having an atomic check would be nice for implementing connector properties. Some of them may need to be validated regardless of crtc.
Can we add this later when we need state validation that is appropriate for an ->atomic_check()?
-DK
I would really like to be able to do the validation in atomic_check instead of during the set_property callback. The state is not completely valid at that point yet, so this would be a logical place.
If that place doesn't work for i915.ko, then we need our own callback (like we already have with e.g. ->compute_config, we could do a ->release_config). But if it's just cosmetics, then I don't see the reason why we need to change this. On that issue: How exactly does our compute_config work if we haven't updated the routing (using the above helper) yet? That sounds like a pretty big misdesign on our side ... -Daniel
-DK
Maybe implementing the relevant VCPI state could be done as an atomic helper function too, so other atomic drivers can just plug it in.
The idea was to reduce boilerplate in the drivers and use the private_obj state for different object types.
Not sure how doable this is, but if it's not too hard, then it's probably cleaner :)
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Feb 24, 2017 at 12:52:53AM +0000, Pandiyan, Dhinakaran wrote:
On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> Could we deal with the VCPI state separately in > intel_modeset_checks, > like we do with dpll?
We'd want to release the VCPI slots before they are acquired in ->compute_config(). intel_modeset_checks() will be too late to release them. Are you suggesting both acquiring and releasing slots should be done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
Adding ->atomic_check that's unconditionally called sounds troubling, because all the other ->atomic_check functions are _only_ called when enabling stuff. ->atomic_release sounds much better to me, and from a helper pov DK's patch above is the right place.
Having an atomic check would be nice for implementing connector properties. Some of them may need to be validated regardless of crtc.
Can we add this later when we need state validation that is appropriate for an ->atomic_check()?
+1 on not solving problems we don't have yet :-) If we'd write code for every eventuality that we can come up with, we'd get nothing done. And ime, such unused code will also be full of bugs.
Discussing issues like this is still good and useful, just to make sure we have a coherent plan for the eventual future, once it happens. -Daniel
Daniel Vetter schreef op zo 26-02-2017 om 21:00 [+0100]:
On Fri, Feb 24, 2017 at 12:52:53AM +0000, Pandiyan, Dhinakaran wrote:
On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:
On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]: > > > > > Could we deal with the VCPI state separately in > > intel_modeset_checks, > > like we do with dpll? > > We'd want to release the VCPI slots before they are > acquired in > ->compute_config(). intel_modeset_checks() will be too > late to > release > them. Are you suggesting both acquiring and releasing > slots > should be > done in intel_modeset_checks()?
That makes things a bit more nasty. Maybe add a conn_funcs->atomic_check that always gets called, something like I did below?
I'd love to use it for some atomic connector properties too.
Adding and unconditionally calling conn_funcs->atomic_check() should be doable. It also follows the pattern we have for encoders and CRTCs. But I'll have to move the connector->state->crtc state checks inside the function.
Adding ->atomic_check that's unconditionally called sounds troubling, because all the other ->atomic_check functions are _only_ called when enabling stuff. ->atomic_release sounds much better to me, and from a helper pov DK's patch above is the right place.
Having an atomic check would be nice for implementing connector properties. Some of them may need to be validated regardless of crtc.
Can we add this later when we need state validation that is appropriate for an ->atomic_check()?
+1 on not solving problems we don't have yet :-) If we'd write code for every eventuality that we can come up with, we'd get nothing done. And ime, such unused code will also be full of bugs.
Discussing issues like this is still good and useful, just to make sure we have a coherent plan for the eventual future, once it happens.
Near future, I'm working on converting i915 specific connector properties to atomic, and it would be nice if I had a connector atomic check function like this. :)
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.
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 9b9dda8..7aec0c7 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, @@ -401,6 +424,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)
dri-devel@lists.freedesktop.org
-
Archit Taneja -
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Dhinakaran Pandiyan -
Lankhorst, Maarten -
Pandiyan, Dhinakaran