* [PATCH v4 0/8] Adding driver-private objects to atomic state
@ 2017-03-16 7:10 Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 1/8] drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr Dhinakaran Pandiyan
` (9 more replies)
0 siblings, 10 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Dhinakaran Pandiyan,
Harry Wentland
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 rebased version includes a few minor changes -
1) Used for_each_oldnew_connector_in_state() macro (7/8)
2) Added a WARN_ON() to check for connection_mutex (5/8)
3) Alignment fix. (4/8)
Pandiyan, Dhinakaran (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 | 185 ++++++++++++++++++++++++++++---
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 | 93 ++++++++++++++++
include/drm/drm_dp_mst_helper.h | 33 ++++--
include/drm/drm_modeset_helper_vtables.h | 13 +++
9 files changed, 427 insertions(+), 38 deletions(-)
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/8] drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 2/8] drm/dp: Kill unused MST vcpi slot availability tracking Dhinakaran Pandiyan
` (8 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
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.
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/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 f2cc375..66a611a 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
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 2/8] drm/dp: Kill unused MST vcpi slot availability tracking
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 1/8] drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 3/8] drm/dp: Split drm_dp_mst_allocate_vcpi Dhinakaran Pandiyan
` (7 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
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.
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/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 66a611a..2e2af13 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
*/
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 3/8] drm/dp: Split drm_dp_mst_allocate_vcpi
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 1/8] drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 2/8] drm/dp: Kill unused MST vcpi slot availability tracking Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 4/8] drm: Add driver-private objects to atomic state Dhinakaran Pandiyan
` (6 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
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 2e2af13..d3fc7e4 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 094cbdc..c1f62eb 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.
@@ -165,7 +164,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 7ad1ee5..418872b 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);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 4/8] drm: Add driver-private objects to atomic state
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (2 preceding siblings ...)
2017-03-16 7:10 ` [PATCH v4 3/8] drm/dp: Split drm_dp_mst_allocate_vcpi Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-22 10:00 ` Maarten Lankhorst
2017-03-16 7:10 ` [PATCH v4 5/8] drm/dp: Introduce MST topology state to track available link bandwidth Dhinakaran Pandiyan
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
It is necessary to track states for objects other than connector, crtc
and plane for atomic modesets. But adding objects like DP MST link
bandwidth to drm_atomic_state would mean that a non-core object will be
modified by the core helper functions for swapping and clearing
it's state. So, lets add void * objects and helper functions that operate
on void * types to keep these objects and states private to the core.
Drivers can then implement specific functions to swap and clear states.
The other advantage having just void * for these objects in
drm_atomic_state is that objects of different types can be managed in the
same state array.
v2: Added docs and new iterator to filter private objects (Daniel)
v3: Macro alignment (Chris)
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Acked-by: Harry Wentland <harry.wentland@amd.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++
drivers/gpu/drm/drm_atomic_helper.c | 5 ++
include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
3 files changed, 166 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9b892af..d89be5c 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);
@@ -978,6 +993,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
}
/**
+ * drm_atomic_get_private_obj_state - get private object state
+ * @state: global atomic state
+ * @obj: private object to get the state for
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * This function returns the private object state for the given private object,
+ * allocating the state if needed. It does not grab any locks as the caller is
+ * expected to care of any required locking.
+ *
+ * RETURNS:
+ *
+ * Either the allocated state or the error code encoded into a pointer.
+ */
+void *
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
+ const struct drm_private_state_funcs *funcs)
+{
+ int index, num_objs, i;
+ size_t size;
+ struct __drm_private_objs_state *arr;
+
+ for (i = 0; i < state->num_private_objs; i++)
+ if (obj == state->private_objs[i].obj &&
+ state->private_objs[i].obj_state)
+ return state->private_objs[i].obj_state;
+
+ num_objs = state->num_private_objs + 1;
+ size = sizeof(*state->private_objs) * num_objs;
+ arr = krealloc(state->private_objs, size, GFP_KERNEL);
+ if (!arr)
+ return ERR_PTR(-ENOMEM);
+
+ state->private_objs = arr;
+ index = state->num_private_objs;
+ memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
+
+ state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
+ if (!state->private_objs[index].obj_state)
+ return ERR_PTR(-ENOMEM);
+
+ state->private_objs[index].obj = obj;
+ state->private_objs[index].funcs = funcs;
+ state->num_private_objs = num_objs;
+
+ DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
+ state->private_objs[index].obj_state, state);
+
+ return state->private_objs[index].obj_state;
+}
+EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
+
+/**
* drm_atomic_get_connector_state - get connector state
* @state: global atomic state object
* @connector: connector to get state object for
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e26b73..1403334 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
struct drm_plane *plane;
struct drm_plane_state *old_plane_state, *new_plane_state;
struct drm_crtc_commit *commit;
+ void *obj, *obj_state;
+ const struct drm_private_state_funcs *funcs;
if (stall) {
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
state->planes[i].state = old_plane_state;
plane->state = new_plane_state;
}
+
+ __for_each_private_obj(state, obj, obj_state, i, funcs)
+ funcs->swap_state(obj, &state->private_objs[i].obj_state);
}
EXPORT_SYMBOL(drm_atomic_helper_swap_state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 0147a04..c17da39 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -155,6 +155,53 @@ struct __drm_connnectors_state {
};
/**
+ * struct drm_private_state_funcs - atomic state functions for private objects
+ *
+ * These hooks are used by atomic helpers to create, swap and destroy states of
+ * private objects. The structure itself is used as a vtable to identify the
+ * associated private object type. Each private object type that needs to be
+ * added to the atomic states is expected to have an implementation of these
+ * hooks and pass a pointer to it's drm_private_state_funcs struct to
+ * drm_atomic_get_private_obj_state().
+ */
+struct drm_private_state_funcs {
+ /**
+ * @duplicate_state:
+ *
+ * Duplicate the current state of the private object and return it. It
+ * is an error to call this before obj->state has been initialized.
+ *
+ * RETURNS:
+ *
+ * Duplicated atomic state or NULL when obj->state is not
+ * initialized or allocation failed.
+ */
+ void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
+
+ /**
+ * @swap_state:
+ *
+ * This function swaps the existing state of a private object @obj with
+ * it's newly created state, the pointer to which is passed as
+ * @obj_state_ptr.
+ */
+ void (*swap_state)(void *obj, void **obj_state_ptr);
+
+ /**
+ * @destroy_state:
+ *
+ * Frees the private object state created with @duplicate_state.
+ */
+ void (*destroy_state)(void *obj_state);
+};
+
+struct __drm_private_objs_state {
+ void *obj;
+ void *obj_state;
+ const struct drm_private_state_funcs *funcs;
+};
+
+/**
* struct drm_atomic_state - the global state object for atomic updates
* @ref: count of all references to this state (will not be freed until zero)
* @dev: parent DRM device
@@ -165,6 +212,8 @@ struct __drm_connnectors_state {
* @crtcs: pointer to array of CRTC pointers
* @num_connector: size of the @connectors and @connector_states arrays
* @connectors: pointer to array of structures with per-connector data
+ * @num_private_objs: size of the @private_objs array
+ * @private_objs: pointer to array of private object pointers
* @acquire_ctx: acquire context for this atomic modeset state update
*/
struct drm_atomic_state {
@@ -178,6 +227,8 @@ struct drm_atomic_state {
struct __drm_crtcs_state *crtcs;
int num_connector;
struct __drm_connnectors_state *connectors;
+ int num_private_objs;
+ struct __drm_private_objs_state *private_objs;
struct drm_modeset_acquire_ctx *acquire_ctx;
@@ -270,6 +321,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
struct drm_connector_state *state, struct drm_property *property,
uint64_t val);
+void * __must_check
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
+ void *obj,
+ const struct drm_private_state_funcs *funcs);
+
/**
* drm_atomic_get_existing_crtc_state - get crtc state, if it exists
* @state: global atomic state object
@@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
for_each_if (plane)
/**
+ * __for_each_private_obj - iterate over all private objects
+ * @__state: the atomic state
+ *
+ * This macro iterates over the array containing private object data in atomic
+ * state
+ */
+#define __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \
+ for ((__i) = 0; \
+ (__i) < (__state)->num_private_objs && \
+ ((obj) = (__state)->private_objs[__i].obj, \
+ (__funcs) = (__state)->private_objs[__i].funcs, \
+ (obj_state) = (__state)->private_objs[__i].obj_state, \
+ 1); \
+ (__i)++) \
+ for_each_if (__funcs)
+
+/**
+ * for_each_private_obj - iterate over a specify type of private object
+ * @__state: the atomic state
+ * @obj_funcs: function table to filter the private objects
+ *
+ * This macro iterates over the private objects state array while filtering the
+ * objects based on the vfunc table that is passed as @obj_funcs. New macros
+ * can be created by passing in the vfunc table associated with a specific
+ * private object.
+ */
+#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs) \
+ for ((__i) = 0; \
+ (__i) < (__state)->num_private_objs && \
+ ((obj) = (__state)->private_objs[__i].obj, \
+ (__funcs) = (__state)->private_objs[__i].funcs, \
+ (obj_state) = (__state)->private_objs[__i].obj_state, \
+ 1); \
+ (__i)++) \
+ for_each_if (__funcs == obj_funcs)
+
+/**
* drm_atomic_crtc_needs_modeset - compute combined modeset need
* @state: &drm_crtc_state for the CRTC
*
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 5/8] drm/dp: Introduce MST topology state to track available link bandwidth
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (3 preceding siblings ...)
2017-03-16 7:10 ` [PATCH v4 4/8] drm: Add driver-private objects to atomic state Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 6/8] drm/dp: Add DP MST helpers to atomically find and release vcpi slots Dhinakaran Pandiyan
` (4 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
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.
v3: WARN_ON() if connection_mutex is not held (Archit)
v2: Included kernel doc, moved state initialization and switched to
kmemdup() for allocation (Daniel)
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
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 | 20 ++++++++++
2 files changed, 95 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d3fc7e4..0ad0baa 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2936,6 +2936,69 @@ 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. Also, drm_atomic_get_private_obj_state() expects the caller
+ * to care of the locking, so warn if don't hold the connection_mutex.
+ *
+ * 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)
+{
+ struct drm_device *dev = mgr->dev;
+
+ WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+ 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 +3043,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 +3072,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
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 6/8] drm/dp: Add DP MST helpers to atomically find and release vcpi slots
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (4 preceding siblings ...)
2017-03-16 7:10 ` [PATCH v4 5/8] drm/dp: Introduce MST topology state to track available link bandwidth Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 7/8] drm: Connector helper function to release resources Dhinakaran Pandiyan
` (3 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
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
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/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 0ad0baa..9f3954e 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
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 7/8] drm: Connector helper function to release resources
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (5 preceding siblings ...)
2017-03-16 7:10 ` [PATCH v4 6/8] drm/dp: Add DP MST helpers to atomically find and release vcpi slots Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-16 8:27 ` Daniel Vetter
2017-03-16 7:10 ` [PATCH v4 8/8] drm/dp: Track MST link bandwidth Dhinakaran Pandiyan
` (2 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Having a ->atomic_release callback is useful to release shared resources
that get allocated in compute_config(). This function is expected to be
called in the atomic_check() phase before new resources are acquired.
v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
v2: Moved the caller hunk to this patch (Daniel)
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/drm_atomic_helper.c | 19 +++++++++++++++++++
include/drm/drm_modeset_helper_vtables.h | 13 +++++++++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1403334..c6c8397 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
}
+ for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+ const struct drm_connector_helper_funcs *conn_funcs;
+ struct drm_crtc_state *crtc_state;
+
+ conn_funcs = connector->helper_private;
+ if (!conn_funcs->atomic_release)
+ continue;
+
+ if (!old_connector_state->crtc)
+ continue;
+
+ crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
+
+ if (crtc_state->connectors_changed ||
+ crtc_state->mode_changed ||
+ (crtc_state->active_changed && !crtc_state->active))
+ conn_funcs->atomic_release(connector, new_connector_state);
+ }
+
return mode_fixup(state);
}
EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 091c422..394ec0c 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -836,6 +836,19 @@ struct drm_connector_helper_funcs {
*/
struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
struct drm_connector_state *connector_state);
+
+ /**
+ * @atomic_release:
+ *
+ * This function is used to release shared resources that were
+ * previously acquired.
+ *
+ * NOTE:
+ *
+ * This function is called in the check phase of an atomic update.
+ */
+ void (*atomic_release)(struct drm_connector *connector,
+ struct drm_connector_state *connector_state);
};
/**
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 8/8] drm/dp: Track MST link bandwidth
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (6 preceding siblings ...)
2017-03-16 7:10 ` [PATCH v4 7/8] drm: Connector helper function to release resources Dhinakaran Pandiyan
@ 2017-03-16 7:10 ` Dhinakaran Pandiyan
2017-03-22 12:30 ` Maarten Lankhorst
2017-03-16 7:29 ` ✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state (rev3) Patchwork
2017-03-16 23:08 ` ✗ Fi.CI.BAT: failure for Adding driver-private objects to atomic state (rev4) Patchwork
9 siblings, 1 reply; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 7:10 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Use the added helpers to track MST link bandwidth for atomic modesets.
Link bw is acquired in the ->atomic_check() phase when CRTCs are being
enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
Similarly, link bw is released during ->atomic_check() with the connector
helper callback ->atomic_release() when CRTCs are disabled.
v2:
Squashed atomic_release() implementation and caller (Daniel)
Fixed logic for connector-crtc switching case (Daniel)
Fixed logic for suspend-resume case.
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index c1f62eb..a8f40fa 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
struct intel_dp *intel_dp = &intel_dig_port->dp;
struct intel_connector *connector =
to_intel_connector(conn_state->connector);
- struct drm_atomic_state *state;
+ struct drm_atomic_state *state = pipe_config->base.state;
int bpp;
- int lane_count, slots;
+ int lane_count, slots = 0;
const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
int mst_pbn;
@@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
* seem to suggest we should do otherwise.
*/
lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
-
pipe_config->lane_count = lane_count;
pipe_config->pipe_bpp = bpp;
pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
- state = pipe_config->base.state;
-
if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
pipe_config->has_audio = true;
- mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
+ mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
pipe_config->pbn = mst_pbn;
- slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
intel_link_compute_m_n(bpp, lane_count,
adjusted_mode->crtc_clock,
pipe_config->port_clock,
&pipe_config->dp_m_n);
+ if (pipe_config->base.active) {
+ slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
+ connector->port, mst_pbn);
+ if (slots < 0) {
+ DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
+ return false;
+ }
+ }
pipe_config->dp_m_n.tu = slots;
return true;
+}
+
+static void intel_dp_mst_atomic_release(struct drm_connector *connector,
+ struct drm_connector_state *conn_state)
+{
+ struct intel_dp_mst_encoder *intel_mst;
+ struct drm_dp_mst_topology_mgr *mgr;
+ struct drm_encoder *encoder;
+ struct intel_connector *intel_connector = to_intel_connector(connector);
+ struct drm_atomic_state *state = conn_state->state;
+ int slots;
+
+ encoder = connector->state->best_encoder;
+ intel_mst = enc_to_mst(encoder);
+ mgr = &intel_mst->primary->dp.mst_mgr;
+ slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
+ intel_connector->port);
+ if (slots < 0)
+ DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
}
static void intel_mst_disable_dp(struct intel_encoder *encoder,
@@ -387,6 +410,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
.mode_valid = intel_dp_mst_mode_valid,
.atomic_best_encoder = intel_mst_atomic_best_encoder,
.best_encoder = intel_mst_best_encoder,
+ .atomic_release = intel_dp_mst_atomic_release,
};
static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* ✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state (rev3)
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (7 preceding siblings ...)
2017-03-16 7:10 ` [PATCH v4 8/8] drm/dp: Track MST link bandwidth Dhinakaran Pandiyan
@ 2017-03-16 7:29 ` Patchwork
2017-03-16 23:08 ` ✗ Fi.CI.BAT: failure for Adding driver-private objects to atomic state (rev4) Patchwork
9 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2017-03-16 7:29 UTC (permalink / raw)
To: Lankhorst, Maarten; +Cc: intel-gfx
== Series Details ==
Series: Adding driver-private objects to atomic state (rev3)
URL : https://patchwork.freedesktop.org/series/19355/
State : success
== Summary ==
Series 19355v3 Adding driver-private objects to atomic state
https://patchwork.freedesktop.org/api/1.0/series/19355/revisions/3/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail -> PASS (fi-snb-2600) fdo#100007
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (fi-byt-n2820) fdo#100126
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100126 https://bugs.freedesktop.org/show_bug.cgi?id=100126
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 458s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 572s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 528s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 543s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 503s
fi-byt-n2820 total:278 pass:246 dwarn:1 dfail:0 fail:0 skip:31 time: 500s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 444s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 433s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 436s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 514s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 498s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 493s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 472s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 592s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 479s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 511s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 554s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 421s
d7cb114e1327b50c609ee6e67122cc0293ea515f drm-tip: 2017y-03m-15d-21h-47m-56s UTC integration manifest
6827919 drm/dp: Track MST link bandwidth
0b43dcc drm: Connector helper function to release resources
d9198ad drm/dp: Add DP MST helpers to atomically find and release vcpi slots
d9e58c7 drm/dp: Introduce MST topology state to track available link bandwidth
0922c61 drm: Add driver-private objects to atomic state
9c041fe drm/dp: Split drm_dp_mst_allocate_vcpi
02d8c85 drm/dp: Kill unused MST vcpi slot availability tracking
cade256 drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4196/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 7/8] drm: Connector helper function to release resources
2017-03-16 7:10 ` [PATCH v4 7/8] drm: Connector helper function to release resources Dhinakaran Pandiyan
@ 2017-03-16 8:27 ` Daniel Vetter
2017-03-16 22:43 ` [PATCH v5 " Dhinakaran Pandiyan
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2017-03-16 8:27 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Thu, Mar 16, 2017 at 12:10:30AM -0700, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> Having a ->atomic_release callback is useful to release shared resources
> that get allocated in compute_config(). This function is expected to be
> called in the atomic_check() phase before new resources are acquired.
>
> v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
> v2: Moved the caller hunk to this patch (Daniel)
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 19 +++++++++++++++++++
> include/drm/drm_modeset_helper_vtables.h | 13 +++++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1403334..c6c8397 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> }
> }
>
> + for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
> + const struct drm_connector_helper_funcs *conn_funcs;
> + struct drm_crtc_state *crtc_state;
> +
> + conn_funcs = connector->helper_private;
> + if (!conn_funcs->atomic_release)
> + continue;
> +
> + if (!old_connector_state->crtc)
> + continue;
> +
> + crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
> +
> + if (crtc_state->connectors_changed ||
> + crtc_state->mode_changed ||
> + (crtc_state->active_changed && !crtc_state->active))
> + conn_funcs->atomic_release(connector, new_connector_state);
> + }
> +
> return mode_fixup(state);
> }
> EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 091c422..394ec0c 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -836,6 +836,19 @@ struct drm_connector_helper_funcs {
> */
> struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
> struct drm_connector_state *connector_state);
> +
> + /**
> + * @atomic_release:
> + *
> + * This function is used to release shared resources that were
> + * previously acquired.
> + *
> + * NOTE:
> + *
> + * This function is called in the check phase of an atomic update.
"is _unconditionally_ called" I think that would be a useful
clarification. Also, probably good to state that this is called before any
atomic_check calls.
With those two nits addressed:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + */
> + void (*atomic_release)(struct drm_connector *connector,
> + struct drm_connector_state *connector_state);
> };
>
> /**
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 7/8] drm: Connector helper function to release resources
2017-03-16 8:27 ` Daniel Vetter
@ 2017-03-16 22:43 ` Dhinakaran Pandiyan
0 siblings, 0 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-16 22:43 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Having an ->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.
v4: Document that the function is conditionally called and before
other atomic_checks() (Daniel)
v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
v2: Moved the caller hunk to this patch (Daniel)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
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 | 16 ++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1403334..c6c8397 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
}
+ for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+ const struct drm_connector_helper_funcs *conn_funcs;
+ struct drm_crtc_state *crtc_state;
+
+ conn_funcs = connector->helper_private;
+ if (!conn_funcs->atomic_release)
+ continue;
+
+ if (!old_connector_state->crtc)
+ continue;
+
+ crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
+
+ if (crtc_state->connectors_changed ||
+ crtc_state->mode_changed ||
+ (crtc_state->active_changed && !crtc_state->active))
+ conn_funcs->atomic_release(connector, new_connector_state);
+ }
+
return mode_fixup(state);
}
EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 091c422..84e80aa 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -836,6 +836,22 @@ 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 conditionally called to release shared resources
+ * when the attached CRTC's mode changes or it's connectors change or
+ * becomes inactive. It is called before the corresponding
+ * &drm_crtc_helper_funcs.atomic_check or
+ * &drm_crtc_helper_funcs.mode_fixup hooks are called.
+ *
+ * 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);
};
/**
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* ✗ Fi.CI.BAT: failure for Adding driver-private objects to atomic state (rev4)
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
` (8 preceding siblings ...)
2017-03-16 7:29 ` ✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state (rev3) Patchwork
@ 2017-03-16 23:08 ` Patchwork
9 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2017-03-16 23:08 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
== Series Details ==
Series: Adding driver-private objects to atomic state (rev4)
URL : https://patchwork.freedesktop.org/series/19355/
State : failure
== Summary ==
Series 19355v4 Adding driver-private objects to atomic state
https://patchwork.freedesktop.org/api/1.0/series/19355/revisions/4/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
pass -> FAIL (fi-snb-2600) fdo#100007
Test gem_sync:
Subgroup basic-all:
pass -> INCOMPLETE (fi-ivb-3770)
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 461s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 577s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 530s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 559s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 503s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 496s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 437s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 436s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 439s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 516s
fi-ivb-3770 total:147 pass:134 dwarn:0 dfail:0 fail:0 skip:12 time: 0s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 486s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 598s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 488s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 523s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 547s
fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time: 423s
99bd31bdd4d2c4c07a9eef33c9c68611a315c5b0 drm-tip: 2017y-03m-16d-22h-22m-27s UTC integration manifest
60f7d1f drm/dp: Track MST link bandwidth
cfd737f drm: Connector helper function to release resources
8be38a8 drm/dp: Add DP MST helpers to atomically find and release vcpi slots
de35d90 drm/dp: Introduce MST topology state to track available link bandwidth
f62c2b3 drm: Add driver-private objects to atomic state
0a293a4 drm/dp: Split drm_dp_mst_allocate_vcpi
4b204a4 drm/dp: Kill unused MST vcpi slot availability tracking
f092f57 drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4210/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/8] drm: Add driver-private objects to atomic state
2017-03-16 7:10 ` [PATCH v4 4/8] drm: Add driver-private objects to atomic state Dhinakaran Pandiyan
@ 2017-03-22 10:00 ` Maarten Lankhorst
2017-03-22 21:05 ` Pandiyan, Dhinakaran
2017-03-22 22:30 ` [PATCH v5 " Dhinakaran Pandiyan
0 siblings, 2 replies; 26+ messages in thread
From: Maarten Lankhorst @ 2017-03-22 10:00 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx
Cc: Daniel Vetter, Archit Taneja, Harry Wentland, dri-devel
Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
>
> v2: Added docs and new iterator to filter private objects (Daniel)
> v3: Macro alignment (Chris)
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Mostly looks good, but too many null checks. I think it's best to get rid of them all
by freeing state->driver_private in default_clear() or setting num_private_objs to 0.
It would remove the need for all null checks I think..
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 8/8] drm/dp: Track MST link bandwidth
2017-03-16 7:10 ` [PATCH v4 8/8] drm/dp: Track MST link bandwidth Dhinakaran Pandiyan
@ 2017-03-22 12:30 ` Maarten Lankhorst
2017-03-22 20:42 ` Pandiyan, Dhinakaran
2017-03-22 20:48 ` Daniel Vetter
0 siblings, 2 replies; 26+ messages in thread
From: Maarten Lankhorst @ 2017-03-22 12:30 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx; +Cc: Daniel Vetter, dri-devel
Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> Use the added helpers to track MST link bandwidth for atomic modesets.
> Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> Similarly, link bw is released during ->atomic_check() with the connector
> helper callback ->atomic_release() when CRTCs are disabled.
>
> v2:
> Squashed atomic_release() implementation and caller (Daniel)
> Fixed logic for connector-crtc switching case (Daniel)
> Fixed logic for suspend-resume case.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index c1f62eb..a8f40fa 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> struct intel_dp *intel_dp = &intel_dig_port->dp;
> struct intel_connector *connector =
> to_intel_connector(conn_state->connector);
> - struct drm_atomic_state *state;
> + struct drm_atomic_state *state = pipe_config->base.state;
> int bpp;
> - int lane_count, slots;
> + int lane_count, slots = 0;
> const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> int mst_pbn;
>
> @@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> * seem to suggest we should do otherwise.
> */
> lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> -
> pipe_config->lane_count = lane_count;
>
> pipe_config->pipe_bpp = bpp;
> pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
>
> - state = pipe_config->base.state;
> -
> if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
> pipe_config->has_audio = true;
> - mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>
> + mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> pipe_config->pbn = mst_pbn;
> - slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
>
> intel_link_compute_m_n(bpp, lane_count,
> adjusted_mode->crtc_clock,
> pipe_config->port_clock,
> &pipe_config->dp_m_n);
>
> + if (pipe_config->base.active) {
> + slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> + connector->port, mst_pbn);
> + if (slots < 0) {
> + DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> + return false;
> + }
> + }
> pipe_config->dp_m_n.tu = slots;
>
> return true;
> +}
> +
> +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> + struct drm_connector_state *conn_state)
> +{
> + struct intel_dp_mst_encoder *intel_mst;
> + struct drm_dp_mst_topology_mgr *mgr;
> + struct drm_encoder *encoder;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
> + struct drm_atomic_state *state = conn_state->state;
> + int slots;
> +
> + encoder = connector->state->best_encoder;
> + intel_mst = enc_to_mst(encoder);
> + mgr = &intel_mst->primary->dp.mst_mgr;
>
> + slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
> + intel_connector->port);
> + if (slots < 0)
> + DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
> }
>
> static void intel_mst_disable_dp(struct intel_encoder *encoder,
> @@ -387,6 +410,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
> .mode_valid = intel_dp_mst_mode_valid,
> .atomic_best_encoder = intel_mst_atomic_best_encoder,
> .best_encoder = intel_mst_best_encoder,
> + .atomic_release = intel_dp_mst_atomic_release,
> };
>
> static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 8/8] drm/dp: Track MST link bandwidth
2017-03-22 12:30 ` Maarten Lankhorst
@ 2017-03-22 20:42 ` Pandiyan, Dhinakaran
2017-03-22 20:48 ` Daniel Vetter
1 sibling, 0 replies; 26+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-22 20:42 UTC (permalink / raw)
To: maarten.lankhorst
Cc: daniel.vetter, intel-gfx, Harry.wentland, architt, dri-devel
On Wed, 2017-03-22 at 13:30 +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> >
> > v2:
> > Squashed atomic_release() implementation and caller (Daniel)
> > Fixed logic for connector-crtc switching case (Daniel)
> > Fixed logic for suspend-resume case.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Harry Wentland <Harry.wentland@amd.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++-------
> > 1 file changed, 31 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index c1f62eb..a8f40fa 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > struct intel_connector *connector =
> > to_intel_connector(conn_state->connector);
> > - struct drm_atomic_state *state;
> > + struct drm_atomic_state *state = pipe_config->base.state;
> > int bpp;
> > - int lane_count, slots;
> > + int lane_count, slots = 0;
> > const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > int mst_pbn;
> >
> > @@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > * seem to suggest we should do otherwise.
> > */
> > lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> > -
> > pipe_config->lane_count = lane_count;
> >
> > pipe_config->pipe_bpp = bpp;
> > pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
> >
> > - state = pipe_config->base.state;
> > -
> > if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
> > pipe_config->has_audio = true;
> > - mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >
> > + mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> > pipe_config->pbn = mst_pbn;
> > - slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> >
> > intel_link_compute_m_n(bpp, lane_count,
> > adjusted_mode->crtc_clock,
> > pipe_config->port_clock,
> > &pipe_config->dp_m_n);
> >
> > + if (pipe_config->base.active) {
> > + slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> > + connector->port, mst_pbn);
> > + if (slots < 0) {
> > + DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> > + return false;
> > + }
> > + }
> > pipe_config->dp_m_n.tu = slots;
> >
> > return true;
> > +}
> > +
> > +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct intel_dp_mst_encoder *intel_mst;
> > + struct drm_dp_mst_topology_mgr *mgr;
> > + struct drm_encoder *encoder;
> > + struct intel_connector *intel_connector = to_intel_connector(connector);
> > + struct drm_atomic_state *state = conn_state->state;
> > + int slots;
> > +
> > + encoder = connector->state->best_encoder;
> > + intel_mst = enc_to_mst(encoder);
> > + mgr = &intel_mst->primary->dp.mst_mgr;
> >
> > + slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > + intel_connector->port);
> > + if (slots < 0)
> > + DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
> > }
> >
> > static void intel_mst_disable_dp(struct intel_encoder *encoder,
> > @@ -387,6 +410,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
> > .mode_valid = intel_dp_mst_mode_valid,
> > .atomic_best_encoder = intel_mst_atomic_best_encoder,
> > .best_encoder = intel_mst_best_encoder,
> > + .atomic_release = intel_dp_mst_atomic_release,
> > };
> >
> > static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
>
> Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
Thanks for the review.
If we already had an atomic commit that released the slots, then
vcpi.num_slots in 'struct drm_dp_mst_port' would have been reset to 0.
The second time, release_vcpi_slots() is called, the topology_state
slot tracking won't be updated because it gets the current allocation
from per-port structure. So, it should work fine.
-DK
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 8/8] drm/dp: Track MST link bandwidth
2017-03-22 12:30 ` Maarten Lankhorst
2017-03-22 20:42 ` Pandiyan, Dhinakaran
@ 2017-03-22 20:48 ` Daniel Vetter
1 sibling, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2017-03-22 20:48 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Daniel Vetter, intel-gfx, dri-devel, Dhinakaran Pandiyan
On Wed, Mar 22, 2017 at 01:30:30PM +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Merged patches 1-3 to drm-misc-next, thanks.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/8] drm: Add driver-private objects to atomic state
2017-03-22 10:00 ` Maarten Lankhorst
@ 2017-03-22 21:05 ` Pandiyan, Dhinakaran
2017-03-23 8:54 ` [Intel-gfx] " Maarten Lankhorst
2017-03-22 22:30 ` [PATCH v5 " Dhinakaran Pandiyan
1 sibling, 1 reply; 26+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-22 21:05 UTC (permalink / raw)
To: maarten.lankhorst
Cc: daniel.vetter, intel-gfx, Harry.wentland, architt, dri-devel
On Wed, 2017-03-22 at 11:00 +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> > v3: Macro alignment (Chris)
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Harry Wentland <Harry.wentland@amd.com>
> > Acked-by: Harry Wentland <harry.wentland@amd.com>
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> Mostly looks good, but too many null checks. I think it's best to get rid of them all
> by freeing state->driver_private in default_clear() or setting num_private_objs to 0.
> It would remove the need for all null checks I think..
>
> ~Maarten
>
Did you mean the NULL checks in this loop inside
drm_atomic_get_private_obj_state()
+ 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;
and the fact that I am not setting num_private_objs = 0 in
drm_atomic_state_default_clear() ?
-DK
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-22 10:00 ` Maarten Lankhorst
2017-03-22 21:05 ` Pandiyan, Dhinakaran
@ 2017-03-22 22:30 ` Dhinakaran Pandiyan
2017-03-27 6:16 ` Maarten Lankhorst
2017-03-27 6:38 ` Daniel Vetter
1 sibling, 2 replies; 26+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-22 22:30 UTC (permalink / raw)
To: intel-gfx
Cc: Archit Taneja, Daniel Vetter, dri-devel, Pandiyan, Dhinakaran,
Harry Wentland
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
It is necessary to track states for objects other than connector, crtc
and plane for atomic modesets. But adding objects like DP MST link
bandwidth to drm_atomic_state would mean that a non-core object will be
modified by the core helper functions for swapping and clearing
it's state. So, lets add void * objects and helper functions that operate
on void * types to keep these objects and states private to the core.
Drivers can then implement specific functions to swap and clear states.
The other advantage having just void * for these objects in
drm_atomic_state is that objects of different types can be managed in the
same state array.
v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
v3: Macro alignment (Chris)
v2: Added docs and new iterator to filter private objects (Daniel)
Acked-by: Harry Wentland <harry.wentland@amd.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
drivers/gpu/drm/drm_atomic_helper.c | 5 ++
include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
3 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9b892af..e590148 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
kfree(state->connectors);
kfree(state->crtcs);
kfree(state->planes);
+ kfree(state->private_objs);
}
EXPORT_SYMBOL(drm_atomic_state_default_release);
@@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
state->planes[i].ptr = NULL;
state->planes[i].state = NULL;
}
+
+ for (i = 0; i < state->num_private_objs; i++) {
+ void *private_obj = state->private_objs[i].obj;
+ void *obj_state = state->private_objs[i].obj_state;
+
+ if (!private_obj)
+ continue;
+
+ state->private_objs[i].funcs->destroy_state(obj_state);
+ state->private_objs[i].obj = NULL;
+ state->private_objs[i].obj_state = NULL;
+ state->private_objs[i].funcs = NULL;
+ }
+ state->num_private_objs = 0;
+
}
EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
}
/**
+ * drm_atomic_get_private_obj_state - get private object state
+ * @state: global atomic state
+ * @obj: private object to get the state for
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * This function returns the private object state for the given private object,
+ * allocating the state if needed. It does not grab any locks as the caller is
+ * expected to care of any required locking.
+ *
+ * RETURNS:
+ *
+ * Either the allocated state or the error code encoded into a pointer.
+ */
+void *
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
+ const struct drm_private_state_funcs *funcs)
+{
+ int index, num_objs, i;
+ size_t size;
+ struct __drm_private_objs_state *arr;
+
+ for (i = 0; i < state->num_private_objs; i++)
+ if (obj == state->private_objs[i].obj &&
+ state->private_objs[i].obj_state)
+ return state->private_objs[i].obj_state;
+
+ num_objs = state->num_private_objs + 1;
+ size = sizeof(*state->private_objs) * num_objs;
+ arr = krealloc(state->private_objs, size, GFP_KERNEL);
+ if (!arr)
+ return ERR_PTR(-ENOMEM);
+
+ state->private_objs = arr;
+ index = state->num_private_objs;
+ memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
+
+ state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
+ if (!state->private_objs[index].obj_state)
+ return ERR_PTR(-ENOMEM);
+
+ state->private_objs[index].obj = obj;
+ state->private_objs[index].funcs = funcs;
+ state->num_private_objs = num_objs;
+
+ DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
+ state->private_objs[index].obj_state, state);
+
+ return state->private_objs[index].obj_state;
+}
+EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
+
+/**
* drm_atomic_get_connector_state - get connector state
* @state: global atomic state object
* @connector: connector to get state object for
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e26b73..1403334 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
struct drm_plane *plane;
struct drm_plane_state *old_plane_state, *new_plane_state;
struct drm_crtc_commit *commit;
+ void *obj, *obj_state;
+ const struct drm_private_state_funcs *funcs;
if (stall) {
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
state->planes[i].state = old_plane_state;
plane->state = new_plane_state;
}
+
+ __for_each_private_obj(state, obj, obj_state, i, funcs)
+ funcs->swap_state(obj, &state->private_objs[i].obj_state);
}
EXPORT_SYMBOL(drm_atomic_helper_swap_state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 0147a04..c17da39 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -155,6 +155,53 @@ struct __drm_connnectors_state {
};
/**
+ * struct drm_private_state_funcs - atomic state functions for private objects
+ *
+ * These hooks are used by atomic helpers to create, swap and destroy states of
+ * private objects. The structure itself is used as a vtable to identify the
+ * associated private object type. Each private object type that needs to be
+ * added to the atomic states is expected to have an implementation of these
+ * hooks and pass a pointer to it's drm_private_state_funcs struct to
+ * drm_atomic_get_private_obj_state().
+ */
+struct drm_private_state_funcs {
+ /**
+ * @duplicate_state:
+ *
+ * Duplicate the current state of the private object and return it. It
+ * is an error to call this before obj->state has been initialized.
+ *
+ * RETURNS:
+ *
+ * Duplicated atomic state or NULL when obj->state is not
+ * initialized or allocation failed.
+ */
+ void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
+
+ /**
+ * @swap_state:
+ *
+ * This function swaps the existing state of a private object @obj with
+ * it's newly created state, the pointer to which is passed as
+ * @obj_state_ptr.
+ */
+ void (*swap_state)(void *obj, void **obj_state_ptr);
+
+ /**
+ * @destroy_state:
+ *
+ * Frees the private object state created with @duplicate_state.
+ */
+ void (*destroy_state)(void *obj_state);
+};
+
+struct __drm_private_objs_state {
+ void *obj;
+ void *obj_state;
+ const struct drm_private_state_funcs *funcs;
+};
+
+/**
* struct drm_atomic_state - the global state object for atomic updates
* @ref: count of all references to this state (will not be freed until zero)
* @dev: parent DRM device
@@ -165,6 +212,8 @@ struct __drm_connnectors_state {
* @crtcs: pointer to array of CRTC pointers
* @num_connector: size of the @connectors and @connector_states arrays
* @connectors: pointer to array of structures with per-connector data
+ * @num_private_objs: size of the @private_objs array
+ * @private_objs: pointer to array of private object pointers
* @acquire_ctx: acquire context for this atomic modeset state update
*/
struct drm_atomic_state {
@@ -178,6 +227,8 @@ struct drm_atomic_state {
struct __drm_crtcs_state *crtcs;
int num_connector;
struct __drm_connnectors_state *connectors;
+ int num_private_objs;
+ struct __drm_private_objs_state *private_objs;
struct drm_modeset_acquire_ctx *acquire_ctx;
@@ -270,6 +321,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
struct drm_connector_state *state, struct drm_property *property,
uint64_t val);
+void * __must_check
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
+ void *obj,
+ const struct drm_private_state_funcs *funcs);
+
/**
* drm_atomic_get_existing_crtc_state - get crtc state, if it exists
* @state: global atomic state object
@@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
for_each_if (plane)
/**
+ * __for_each_private_obj - iterate over all private objects
+ * @__state: the atomic state
+ *
+ * This macro iterates over the array containing private object data in atomic
+ * state
+ */
+#define __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \
+ for ((__i) = 0; \
+ (__i) < (__state)->num_private_objs && \
+ ((obj) = (__state)->private_objs[__i].obj, \
+ (__funcs) = (__state)->private_objs[__i].funcs, \
+ (obj_state) = (__state)->private_objs[__i].obj_state, \
+ 1); \
+ (__i)++) \
+ for_each_if (__funcs)
+
+/**
+ * for_each_private_obj - iterate over a specify type of private object
+ * @__state: the atomic state
+ * @obj_funcs: function table to filter the private objects
+ *
+ * This macro iterates over the private objects state array while filtering the
+ * objects based on the vfunc table that is passed as @obj_funcs. New macros
+ * can be created by passing in the vfunc table associated with a specific
+ * private object.
+ */
+#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs) \
+ for ((__i) = 0; \
+ (__i) < (__state)->num_private_objs && \
+ ((obj) = (__state)->private_objs[__i].obj, \
+ (__funcs) = (__state)->private_objs[__i].funcs, \
+ (obj_state) = (__state)->private_objs[__i].obj_state, \
+ 1); \
+ (__i)++) \
+ for_each_if (__funcs == obj_funcs)
+
+/**
* drm_atomic_crtc_needs_modeset - compute combined modeset need
* @state: &drm_crtc_state for the CRTC
*
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH v4 4/8] drm: Add driver-private objects to atomic state
2017-03-22 21:05 ` Pandiyan, Dhinakaran
@ 2017-03-23 8:54 ` Maarten Lankhorst
0 siblings, 0 replies; 26+ messages in thread
From: Maarten Lankhorst @ 2017-03-23 8:54 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: daniel.vetter, intel-gfx, dri-devel
Op 22-03-17 om 22:05 schreef Pandiyan, Dhinakaran:
> On Wed, 2017-03-22 at 11:00 +0100, Maarten Lankhorst wrote:
>> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>>
>>> It is necessary to track states for objects other than connector, crtc
>>> and plane for atomic modesets. But adding objects like DP MST link
>>> bandwidth to drm_atomic_state would mean that a non-core object will be
>>> modified by the core helper functions for swapping and clearing
>>> it's state. So, lets add void * objects and helper functions that operate
>>> on void * types to keep these objects and states private to the core.
>>> Drivers can then implement specific functions to swap and clear states.
>>> The other advantage having just void * for these objects in
>>> drm_atomic_state is that objects of different types can be managed in the
>>> same state array.
>>>
>>> v2: Added docs and new iterator to filter private objects (Daniel)
>>> v3: Macro alignment (Chris)
>>>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Archit Taneja <architt@codeaurora.org>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Harry Wentland <Harry.wentland@amd.com>
>>> Acked-by: Harry Wentland <harry.wentland@amd.com>
>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Mostly looks good, but too many null checks. I think it's best to get rid of them all
>> by freeing state->driver_private in default_clear() or setting num_private_objs to 0.
>> It would remove the need for all null checks I think..
>>
>> ~Maarten
>>
> Did you mean the NULL checks in this loop inside
> drm_atomic_get_private_obj_state()
>
> + 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;
>
> and the fact that I am not setting num_private_objs = 0 in
> drm_atomic_state_default_clear() ?
All of them, the NULL check in default_clear goes away, same for get_private_obj_state, and __for_each_private_obj
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-22 22:30 ` [PATCH v5 " Dhinakaran Pandiyan
@ 2017-03-27 6:16 ` Maarten Lankhorst
2017-03-27 6:38 ` Daniel Vetter
1 sibling, 0 replies; 26+ messages in thread
From: Maarten Lankhorst @ 2017-03-27 6:16 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx
Cc: Daniel Vetter, Archit Taneja, Harry Wentland, dri-devel
Hey,
There are still 2 unnecessary NULL checks, afaict.
Op 22-03-17 om 23:30 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
>
> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> v3: Macro alignment (Chris)
> v2: Added docs and new iterator to filter private objects (Daniel)
>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 167 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9b892af..e590148 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> kfree(state->connectors);
> kfree(state->crtcs);
> kfree(state->planes);
> + kfree(state->private_objs);
> }
> EXPORT_SYMBOL(drm_atomic_state_default_release);
>
> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->planes[i].ptr = NULL;
> state->planes[i].state = NULL;
> }
> +
> + for (i = 0; i < state->num_private_objs; i++) {
> + void *private_obj = state->private_objs[i].obj;
> + void *obj_state = state->private_objs[i].obj_state;
> +
> + if (!private_obj)
> + continue;
^This one.
> + state->private_objs[i].funcs->destroy_state(obj_state);
> + state->private_objs[i].obj = NULL;
> + state->private_objs[i].obj_state = NULL;
> + state->private_objs[i].funcs = NULL;
> + }
> + state->num_private_objs = 0;
> +
> }
> EXPORT_SYMBOL(drm_atomic_state_default_clear);
>
> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> }
>
> /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the object
> + * type
> + *
> + * This function returns the private object state for the given private object,
> + * allocating the state if needed. It does not grab any locks as the caller is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> + const struct drm_private_state_funcs *funcs)
> +{
> + int index, num_objs, i;
> + size_t size;
> + struct __drm_private_objs_state *arr;
> +
> + for (i = 0; i < state->num_private_objs; i++)
> + if (obj == state->private_objs[i].obj &&
> + state->private_objs[i].obj_state)
> + return state->private_objs[i].obj_state;
> +
> + num_objs = state->num_private_objs + 1;
> + size = sizeof(*state->private_objs) * num_objs;
> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> + if (!arr)
> + return ERR_PTR(-ENOMEM);
> +
> + state->private_objs = arr;
> + index = state->num_private_objs;
> + memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
> +
> + state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
> + if (!state->private_objs[index].obj_state)
> + return ERR_PTR(-ENOMEM);
> +
> + state->private_objs[index].obj = obj;
> + state->private_objs[index].funcs = funcs;
> + state->num_private_objs = num_objs;
> +
> + DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> + state->private_objs[index].obj_state, state);
> +
> + return state->private_objs[index].obj_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
> +
> +/**
> * drm_atomic_get_connector_state - get connector state
> * @state: global atomic state object
> * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4e26b73..1403334 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> struct drm_plane *plane;
> struct drm_plane_state *old_plane_state, *new_plane_state;
> struct drm_crtc_commit *commit;
> + void *obj, *obj_state;
> + const struct drm_private_state_funcs *funcs;
>
> if (stall) {
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> state->planes[i].state = old_plane_state;
> plane->state = new_plane_state;
> }
> +
> + __for_each_private_obj(state, obj, obj_state, i, funcs)
> + funcs->swap_state(obj, &state->private_objs[i].obj_state);
> }
> EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0147a04..c17da39 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -155,6 +155,53 @@ struct __drm_connnectors_state {
> };
>
> /**
> + * struct drm_private_state_funcs - atomic state functions for private objects
> + *
> + * These hooks are used by atomic helpers to create, swap and destroy states of
> + * private objects. The structure itself is used as a vtable to identify the
> + * associated private object type. Each private object type that needs to be
> + * added to the atomic states is expected to have an implementation of these
> + * hooks and pass a pointer to it's drm_private_state_funcs struct to
> + * drm_atomic_get_private_obj_state().
> + */
> +struct drm_private_state_funcs {
> + /**
> + * @duplicate_state:
> + *
> + * Duplicate the current state of the private object and return it. It
> + * is an error to call this before obj->state has been initialized.
> + *
> + * RETURNS:
> + *
> + * Duplicated atomic state or NULL when obj->state is not
> + * initialized or allocation failed.
> + */
> + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> +
> + /**
> + * @swap_state:
> + *
> + * This function swaps the existing state of a private object @obj with
> + * it's newly created state, the pointer to which is passed as
> + * @obj_state_ptr.
> + */
> + void (*swap_state)(void *obj, void **obj_state_ptr);
> +
> + /**
> + * @destroy_state:
> + *
> + * Frees the private object state created with @duplicate_state.
> + */
> + void (*destroy_state)(void *obj_state);
> +};
> +
> +struct __drm_private_objs_state {
> + void *obj;
> + void *obj_state;
> + const struct drm_private_state_funcs *funcs;
> +};
> +
> +/**
> * struct drm_atomic_state - the global state object for atomic updates
> * @ref: count of all references to this state (will not be freed until zero)
> * @dev: parent DRM device
> @@ -165,6 +212,8 @@ struct __drm_connnectors_state {
> * @crtcs: pointer to array of CRTC pointers
> * @num_connector: size of the @connectors and @connector_states arrays
> * @connectors: pointer to array of structures with per-connector data
> + * @num_private_objs: size of the @private_objs array
> + * @private_objs: pointer to array of private object pointers
> * @acquire_ctx: acquire context for this atomic modeset state update
> */
> struct drm_atomic_state {
> @@ -178,6 +227,8 @@ struct drm_atomic_state {
> struct __drm_crtcs_state *crtcs;
> int num_connector;
> struct __drm_connnectors_state *connectors;
> + int num_private_objs;
> + struct __drm_private_objs_state *private_objs;
>
> struct drm_modeset_acquire_ctx *acquire_ctx;
>
> @@ -270,6 +321,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> struct drm_connector_state *state, struct drm_property *property,
> uint64_t val);
>
> +void * __must_check
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> + void *obj,
> + const struct drm_private_state_funcs *funcs);
> +
> /**
> * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> * @state: global atomic state object
> @@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> for_each_if (plane)
>
> /**
> + * __for_each_private_obj - iterate over all private objects
> + * @__state: the atomic state
> + *
> + * This macro iterates over the array containing private object data in atomic
> + * state
> + */
> +#define __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \
> + for ((__i) = 0; \
> + (__i) < (__state)->num_private_objs && \
> + ((obj) = (__state)->private_objs[__i].obj, \
> + (__funcs) = (__state)->private_objs[__i].funcs, \
> + (obj_state) = (__state)->private_objs[__i].obj_state, \
> + 1); \
> + (__i)++) \
> + for_each_if (__funcs)
^This.
> +/**
> + * 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
> *
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-22 22:30 ` [PATCH v5 " Dhinakaran Pandiyan
2017-03-27 6:16 ` Maarten Lankhorst
@ 2017-03-27 6:38 ` Daniel Vetter
2017-03-27 8:28 ` Maarten Lankhorst
1 sibling, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2017-03-27 6:38 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
>
> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> v3: Macro alignment (Chris)
> v2: Added docs and new iterator to filter private objects (Daniel)
>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 167 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9b892af..e590148 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> kfree(state->connectors);
> kfree(state->crtcs);
> kfree(state->planes);
> + kfree(state->private_objs);
> }
> EXPORT_SYMBOL(drm_atomic_state_default_release);
>
> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->planes[i].ptr = NULL;
> state->planes[i].state = NULL;
> }
> +
> + for (i = 0; i < state->num_private_objs; i++) {
> + void *private_obj = state->private_objs[i].obj;
> + void *obj_state = state->private_objs[i].obj_state;
> +
> + if (!private_obj)
> + continue;
> +
> + state->private_objs[i].funcs->destroy_state(obj_state);
> + state->private_objs[i].obj = NULL;
> + state->private_objs[i].obj_state = NULL;
> + state->private_objs[i].funcs = NULL;
> + }
> + state->num_private_objs = 0;
Here we set num_private_objs = 0;
> +
> }
> EXPORT_SYMBOL(drm_atomic_state_default_clear);
>
> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> }
>
> /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the object
> + * type
> + *
> + * This function returns the private object state for the given private object,
> + * allocating the state if needed. It does not grab any locks as the caller is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> + const struct drm_private_state_funcs *funcs)
> +{
> + int index, num_objs, i;
> + size_t size;
> + struct __drm_private_objs_state *arr;
> +
> + for (i = 0; i < state->num_private_objs; i++)
> + if (obj == state->private_objs[i].obj &&
> + state->private_objs[i].obj_state)
> + return state->private_objs[i].obj_state;
> +
> + num_objs = state->num_private_objs + 1;
> + size = sizeof(*state->private_objs) * num_objs;
> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
But here we unconditionally realloc to a presumably smaller size. If you
look at drm_atomic_state->num_connector (which also does dynamic array
realloc), that one works a bit differently (and hence needs these NULL
checks).
I think aligning with how we do things with connectors, for consistency
(no other reason really) would be good.
Just noticed this while reading Maarten's review, which seems to go even
farther away from how we handle this for connectors.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-27 6:38 ` Daniel Vetter
@ 2017-03-27 8:28 ` Maarten Lankhorst
2017-03-27 8:31 ` Daniel Vetter
0 siblings, 1 reply; 26+ messages in thread
From: Maarten Lankhorst @ 2017-03-27 8:28 UTC (permalink / raw)
To: Daniel Vetter, Dhinakaran Pandiyan
Cc: Archit Taneja, Daniel Vetter, intel-gfx, dri-devel, Harry Wentland
Op 27-03-17 om 08:38 schreef Daniel Vetter:
> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>
>> It is necessary to track states for objects other than connector, crtc
>> and plane for atomic modesets. But adding objects like DP MST link
>> bandwidth to drm_atomic_state would mean that a non-core object will be
>> modified by the core helper functions for swapping and clearing
>> it's state. So, lets add void * objects and helper functions that operate
>> on void * types to keep these objects and states private to the core.
>> Drivers can then implement specific functions to swap and clear states.
>> The other advantage having just void * for these objects in
>> drm_atomic_state is that objects of different types can be managed in the
>> same state array.
>>
>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
>> v3: Macro alignment (Chris)
>> v2: Added docs and new iterator to filter private objects (Daniel)
>>
>> Acked-by: Harry Wentland <harry.wentland@amd.com>
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
>> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 167 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 9b892af..e590148 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
>> kfree(state->connectors);
>> kfree(state->crtcs);
>> kfree(state->planes);
>> + kfree(state->private_objs);
>> }
>> EXPORT_SYMBOL(drm_atomic_state_default_release);
>>
>> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>> state->planes[i].ptr = NULL;
>> state->planes[i].state = NULL;
>> }
>> +
>> + for (i = 0; i < state->num_private_objs; i++) {
>> + void *private_obj = state->private_objs[i].obj;
>> + void *obj_state = state->private_objs[i].obj_state;
>> +
>> + if (!private_obj)
>> + continue;
>> +
>> + state->private_objs[i].funcs->destroy_state(obj_state);
>> + state->private_objs[i].obj = NULL;
>> + state->private_objs[i].obj_state = NULL;
>> + state->private_objs[i].funcs = NULL;
>> + }
>> + state->num_private_objs = 0;
> Here we set num_private_objs = 0;
>
>> +
>> }
>> EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>
>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>> }
>>
>> /**
>> + * drm_atomic_get_private_obj_state - get private object state
>> + * @state: global atomic state
>> + * @obj: private object to get the state for
>> + * @funcs: pointer to the struct of function pointers that identify the object
>> + * type
>> + *
>> + * This function returns the private object state for the given private object,
>> + * allocating the state if needed. It does not grab any locks as the caller is
>> + * expected to care of any required locking.
>> + *
>> + * RETURNS:
>> + *
>> + * Either the allocated state or the error code encoded into a pointer.
>> + */
>> +void *
>> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
>> + const struct drm_private_state_funcs *funcs)
>> +{
>> + int index, num_objs, i;
>> + size_t size;
>> + struct __drm_private_objs_state *arr;
>> +
>> + for (i = 0; i < state->num_private_objs; i++)
>> + if (obj == state->private_objs[i].obj &&
>> + state->private_objs[i].obj_state)
>> + return state->private_objs[i].obj_state;
>> +
>> + num_objs = state->num_private_objs + 1;
>> + size = sizeof(*state->private_objs) * num_objs;
>> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> But here we unconditionally realloc to a presumably smaller size. If you
> look at drm_atomic_state->num_connector (which also does dynamic array
> realloc), that one works a bit differently (and hence needs these NULL
> checks).
>
> I think aligning with how we do things with connectors, for consistency
> (no other reason really) would be good.
>
> Just noticed this while reading Maarten's review, which seems to go even
> farther away from how we handle this for connectors.
> -Daniel
Connectors are handled differently, because there's a fixed number of connectors and each
connector is assigned to its slot at state->connectors[drm_connector_index];
For private objects this is not the case, there's no way to put them in a fixed index,
so the array is resized and reallocated as needed. If you care about the realloc to a smaller
size, add a separate variable max_private_objs and multiply its size by 2 every time it's
not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
I don't propose you should though, because N is small enough and the increased complexity
isn't worth the decreased readability. So just set num to zero and don't worry about null
checks. :)
~Maarten
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-27 8:28 ` Maarten Lankhorst
@ 2017-03-27 8:31 ` Daniel Vetter
2017-03-27 8:35 ` Maarten Lankhorst
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2017-03-27 8:31 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Archit Taneja, Daniel Vetter, intel-gfx, dri-devel,
Dhinakaran Pandiyan, Harry Wentland
On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
> Op 27-03-17 om 08:38 schreef Daniel Vetter:
> > On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> >> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >>
> >> It is necessary to track states for objects other than connector, crtc
> >> and plane for atomic modesets. But adding objects like DP MST link
> >> bandwidth to drm_atomic_state would mean that a non-core object will be
> >> modified by the core helper functions for swapping and clearing
> >> it's state. So, lets add void * objects and helper functions that operate
> >> on void * types to keep these objects and states private to the core.
> >> Drivers can then implement specific functions to swap and clear states.
> >> The other advantage having just void * for these objects in
> >> drm_atomic_state is that objects of different types can be managed in the
> >> same state array.
> >>
> >> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> >> v3: Macro alignment (Chris)
> >> v2: Added docs and new iterator to filter private objects (Daniel)
> >>
> >> Acked-by: Harry Wentland <harry.wentland@amd.com>
> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> ---
> >> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
> >> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
> >> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 167 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 9b892af..e590148 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> >> kfree(state->connectors);
> >> kfree(state->crtcs);
> >> kfree(state->planes);
> >> + kfree(state->private_objs);
> >> }
> >> EXPORT_SYMBOL(drm_atomic_state_default_release);
> >>
> >> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >> state->planes[i].ptr = NULL;
> >> state->planes[i].state = NULL;
> >> }
> >> +
> >> + for (i = 0; i < state->num_private_objs; i++) {
> >> + void *private_obj = state->private_objs[i].obj;
> >> + void *obj_state = state->private_objs[i].obj_state;
> >> +
> >> + if (!private_obj)
> >> + continue;
> >> +
> >> + state->private_objs[i].funcs->destroy_state(obj_state);
> >> + state->private_objs[i].obj = NULL;
> >> + state->private_objs[i].obj_state = NULL;
> >> + state->private_objs[i].funcs = NULL;
> >> + }
> >> + state->num_private_objs = 0;
> > Here we set num_private_objs = 0;
> >
> >> +
> >> }
> >> EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>
> >> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >> }
> >>
> >> /**
> >> + * drm_atomic_get_private_obj_state - get private object state
> >> + * @state: global atomic state
> >> + * @obj: private object to get the state for
> >> + * @funcs: pointer to the struct of function pointers that identify the object
> >> + * type
> >> + *
> >> + * This function returns the private object state for the given private object,
> >> + * allocating the state if needed. It does not grab any locks as the caller is
> >> + * expected to care of any required locking.
> >> + *
> >> + * RETURNS:
> >> + *
> >> + * Either the allocated state or the error code encoded into a pointer.
> >> + */
> >> +void *
> >> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> >> + const struct drm_private_state_funcs *funcs)
> >> +{
> >> + int index, num_objs, i;
> >> + size_t size;
> >> + struct __drm_private_objs_state *arr;
> >> +
> >> + for (i = 0; i < state->num_private_objs; i++)
> >> + if (obj == state->private_objs[i].obj &&
> >> + state->private_objs[i].obj_state)
> >> + return state->private_objs[i].obj_state;
> >> +
> >> + num_objs = state->num_private_objs + 1;
> >> + size = sizeof(*state->private_objs) * num_objs;
> >> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > But here we unconditionally realloc to a presumably smaller size. If you
> > look at drm_atomic_state->num_connector (which also does dynamic array
> > realloc), that one works a bit differently (and hence needs these NULL
> > checks).
> >
> > I think aligning with how we do things with connectors, for consistency
> > (no other reason really) would be good.
> >
> > Just noticed this while reading Maarten's review, which seems to go even
> > farther away from how we handle this for connectors.
> > -Daniel
>
> Connectors are handled differently, because there's a fixed number of connectors and each
> connector is assigned to its slot at state->connectors[drm_connector_index];
>
> For private objects this is not the case, there's no way to put them in a fixed index,
> so the array is resized and reallocated as needed. If you care about the realloc to a smaller
> size, add a separate variable max_private_objs and multiply its size by 2 every time it's
> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
>
> I don't propose you should though, because N is small enough and the increased complexity
> isn't worth the decreased readability. So just set num to zero and don't worry about null
> checks. :)
Hm, in that case shouldn't we also kfree the allocation in default_clear?
Makes no sense resetting to 0 and not freeing, when we do an
unconditional krealloc afterwards. That's the part that confused me ...
I'm not worried about the realloc overahead (and that's easy to fix
indeed).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-27 8:31 ` Daniel Vetter
@ 2017-03-27 8:35 ` Maarten Lankhorst
2017-03-27 18:24 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 26+ messages in thread
From: Maarten Lankhorst @ 2017-03-27 8:35 UTC (permalink / raw)
To: Daniel Vetter
Cc: Archit Taneja, Daniel Vetter, intel-gfx, dri-devel,
Dhinakaran Pandiyan, Harry Wentland
Op 27-03-17 om 10:31 schreef Daniel Vetter:
> On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
>> Op 27-03-17 om 08:38 schreef Daniel Vetter:
>>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
>>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>>>
>>>> It is necessary to track states for objects other than connector, crtc
>>>> and plane for atomic modesets. But adding objects like DP MST link
>>>> bandwidth to drm_atomic_state would mean that a non-core object will be
>>>> modified by the core helper functions for swapping and clearing
>>>> it's state. So, lets add void * objects and helper functions that operate
>>>> on void * types to keep these objects and states private to the core.
>>>> Drivers can then implement specific functions to swap and clear states.
>>>> The other advantage having just void * for these objects in
>>>> drm_atomic_state is that objects of different types can be managed in the
>>>> same state array.
>>>>
>>>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
>>>> v3: Macro alignment (Chris)
>>>> v2: Added docs and new iterator to filter private objects (Daniel)
>>>>
>>>> Acked-by: Harry Wentland <harry.wentland@amd.com>
>>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
>>>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
>>>> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 167 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index 9b892af..e590148 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
>>>> kfree(state->connectors);
>>>> kfree(state->crtcs);
>>>> kfree(state->planes);
>>>> + kfree(state->private_objs);
>>>> }
>>>> EXPORT_SYMBOL(drm_atomic_state_default_release);
>>>>
>>>> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>>>> state->planes[i].ptr = NULL;
>>>> state->planes[i].state = NULL;
>>>> }
>>>> +
>>>> + for (i = 0; i < state->num_private_objs; i++) {
>>>> + void *private_obj = state->private_objs[i].obj;
>>>> + void *obj_state = state->private_objs[i].obj_state;
>>>> +
>>>> + if (!private_obj)
>>>> + continue;
>>>> +
>>>> + state->private_objs[i].funcs->destroy_state(obj_state);
>>>> + state->private_objs[i].obj = NULL;
>>>> + state->private_objs[i].obj_state = NULL;
>>>> + state->private_objs[i].funcs = NULL;
>>>> + }
>>>> + state->num_private_objs = 0;
>>> Here we set num_private_objs = 0;
>>>
>>>> +
>>>> }
>>>> EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>>>
>>>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>>>> }
>>>>
>>>> /**
>>>> + * drm_atomic_get_private_obj_state - get private object state
>>>> + * @state: global atomic state
>>>> + * @obj: private object to get the state for
>>>> + * @funcs: pointer to the struct of function pointers that identify the object
>>>> + * type
>>>> + *
>>>> + * This function returns the private object state for the given private object,
>>>> + * allocating the state if needed. It does not grab any locks as the caller is
>>>> + * expected to care of any required locking.
>>>> + *
>>>> + * RETURNS:
>>>> + *
>>>> + * Either the allocated state or the error code encoded into a pointer.
>>>> + */
>>>> +void *
>>>> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
>>>> + const struct drm_private_state_funcs *funcs)
>>>> +{
>>>> + int index, num_objs, i;
>>>> + size_t size;
>>>> + struct __drm_private_objs_state *arr;
>>>> +
>>>> + for (i = 0; i < state->num_private_objs; i++)
>>>> + if (obj == state->private_objs[i].obj &&
>>>> + state->private_objs[i].obj_state)
>>>> + return state->private_objs[i].obj_state;
>>>> +
>>>> + num_objs = state->num_private_objs + 1;
>>>> + size = sizeof(*state->private_objs) * num_objs;
>>>> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
>>> But here we unconditionally realloc to a presumably smaller size. If you
>>> look at drm_atomic_state->num_connector (which also does dynamic array
>>> realloc), that one works a bit differently (and hence needs these NULL
>>> checks).
>>>
>>> I think aligning with how we do things with connectors, for consistency
>>> (no other reason really) would be good.
>>>
>>> Just noticed this while reading Maarten's review, which seems to go even
>>> farther away from how we handle this for connectors.
>>> -Daniel
>> Connectors are handled differently, because there's a fixed number of connectors and each
>> connector is assigned to its slot at state->connectors[drm_connector_index];
>>
>> For private objects this is not the case, there's no way to put them in a fixed index,
>> so the array is resized and reallocated as needed. If you care about the realloc to a smaller
>> size, add a separate variable max_private_objs and multiply its size by 2 every time it's
>> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
>>
>> I don't propose you should though, because N is small enough and the increased complexity
>> isn't worth the decreased readability. So just set num to zero and don't worry about null
>> checks. :)
> Hm, in that case shouldn't we also kfree the allocation in default_clear?
> Makes no sense resetting to 0 and not freeing, when we do an
> unconditional krealloc afterwards. That's the part that confused me ...
> I'm not worried about the realloc overahead (and that's easy to fix
> indeed).
> -Daniel
We might as well, strictly speaking not needed but probably reduces confusion. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state
2017-03-27 8:35 ` Maarten Lankhorst
@ 2017-03-27 18:24 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 26+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-27 18:24 UTC (permalink / raw)
To: maarten.lankhorst
Cc: architt, daniel.vetter, intel-gfx, dri-devel, Harry.wentland
On Mon, 2017-03-27 at 10:35 +0200, Maarten Lankhorst wrote:
> Op 27-03-17 om 10:31 schreef Daniel Vetter:
> > On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
> >> Op 27-03-17 om 08:38 schreef Daniel Vetter:
> >>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> >>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >>>>
> >>>> It is necessary to track states for objects other than connector, crtc
> >>>> and plane for atomic modesets. But adding objects like DP MST link
> >>>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>>> modified by the core helper functions for swapping and clearing
> >>>> it's state. So, lets add void * objects and helper functions that operate
> >>>> on void * types to keep these objects and states private to the core.
> >>>> Drivers can then implement specific functions to swap and clear states.
> >>>> The other advantage having just void * for these objects in
> >>>> drm_atomic_state is that objects of different types can be managed in the
> >>>> same state array.
> >>>>
> >>>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> >>>> v3: Macro alignment (Chris)
> >>>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>>
> >>>> Acked-by: Harry Wentland <harry.wentland@amd.com>
> >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++
> >>>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
> >>>> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 167 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>>> index 9b892af..e590148 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> >>>> kfree(state->connectors);
> >>>> kfree(state->crtcs);
> >>>> kfree(state->planes);
> >>>> + kfree(state->private_objs);
> >>>> }
> >>>> EXPORT_SYMBOL(drm_atomic_state_default_release);
> >>>>
> >>>> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >>>> state->planes[i].ptr = NULL;
> >>>> state->planes[i].state = NULL;
> >>>> }
> >>>> +
> >>>> + for (i = 0; i < state->num_private_objs; i++) {
> >>>> + void *private_obj = state->private_objs[i].obj;
> >>>> + void *obj_state = state->private_objs[i].obj_state;
> >>>> +
> >>>> + if (!private_obj)
> >>>> + continue;
> >>>> +
> >>>> + state->private_objs[i].funcs->destroy_state(obj_state);
> >>>> + state->private_objs[i].obj = NULL;
> >>>> + state->private_objs[i].obj_state = NULL;
> >>>> + state->private_objs[i].funcs = NULL;
> >>>> + }
> >>>> + state->num_private_objs = 0;
> >>> Here we set num_private_objs = 0;
> >>>
> >>>> +
> >>>> }
> >>>> EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>>>
> >>>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >>>> }
> >>>>
> >>>> /**
> >>>> + * drm_atomic_get_private_obj_state - get private object state
> >>>> + * @state: global atomic state
> >>>> + * @obj: private object to get the state for
> >>>> + * @funcs: pointer to the struct of function pointers that identify the object
> >>>> + * type
> >>>> + *
> >>>> + * This function returns the private object state for the given private object,
> >>>> + * allocating the state if needed. It does not grab any locks as the caller is
> >>>> + * expected to care of any required locking.
> >>>> + *
> >>>> + * RETURNS:
> >>>> + *
> >>>> + * Either the allocated state or the error code encoded into a pointer.
> >>>> + */
> >>>> +void *
> >>>> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> >>>> + const struct drm_private_state_funcs *funcs)
> >>>> +{
> >>>> + int index, num_objs, i;
> >>>> + size_t size;
> >>>> + struct __drm_private_objs_state *arr;
> >>>> +
> >>>> + for (i = 0; i < state->num_private_objs; i++)
> >>>> + if (obj == state->private_objs[i].obj &&
> >>>> + state->private_objs[i].obj_state)
> >>>> + return state->private_objs[i].obj_state;
> >>>> +
> >>>> + num_objs = state->num_private_objs + 1;
> >>>> + size = sizeof(*state->private_objs) * num_objs;
> >>>> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> >>> But here we unconditionally realloc to a presumably smaller size. If you
> >>> look at drm_atomic_state->num_connector (which also does dynamic array
> >>> realloc), that one works a bit differently (and hence needs these NULL
> >>> checks).
> >>>
> >>> I think aligning with how we do things with connectors, for consistency
> >>> (no other reason really) would be good.
> >>>
> >>> Just noticed this while reading Maarten's review, which seems to go even
> >>> farther away from how we handle this for connectors.
> >>> -Daniel
> >> Connectors are handled differently, because there's a fixed number of connectors and each
> >> connector is assigned to its slot at state->connectors[drm_connector_index];
> >>
> >> For private objects this is not the case, there's no way to put them in a fixed index,
> >> so the array is resized and reallocated as needed. If you care about the realloc to a smaller
> >> size, add a separate variable max_private_objs and multiply its size by 2 every time it's
> >> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
> >>
> >> I don't propose you should though, because N is small enough and the increased complexity
> >> isn't worth the decreased readability. So just set num to zero and don't worry about null
> >> checks. :)
> > Hm, in that case shouldn't we also kfree the allocation in default_clear?
> > Makes no sense resetting to 0 and not freeing, when we do an
> > unconditional krealloc afterwards. That's the part that confused me ...
> > I'm not worried about the realloc overahead (and that's easy to fix
> > indeed).
> > -Daniel
>
> We might as well, strictly speaking not needed but probably reduces confusion. :)
>
kfree'ing the state->private_objs array in
drm_atomic_state_default_clear() does not seem consistent with what we
do for crtcs, planes and connectors. default_release() seems to be the
function that frees memory for state arrays. We could just go with v4,
where state->num_private_objs is not reset. As 'n' is small, the NULL
checks shouldn't be costly. I can look at optimizing realloc's once we
have more consumers for private_objs?
-DK
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-03-27 18:24 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-16 7:10 [PATCH v4 0/8] Adding driver-private objects to atomic state Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 1/8] drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 2/8] drm/dp: Kill unused MST vcpi slot availability tracking Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 3/8] drm/dp: Split drm_dp_mst_allocate_vcpi Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 4/8] drm: Add driver-private objects to atomic state Dhinakaran Pandiyan
2017-03-22 10:00 ` Maarten Lankhorst
2017-03-22 21:05 ` Pandiyan, Dhinakaran
2017-03-23 8:54 ` [Intel-gfx] " Maarten Lankhorst
2017-03-22 22:30 ` [PATCH v5 " Dhinakaran Pandiyan
2017-03-27 6:16 ` Maarten Lankhorst
2017-03-27 6:38 ` Daniel Vetter
2017-03-27 8:28 ` Maarten Lankhorst
2017-03-27 8:31 ` Daniel Vetter
2017-03-27 8:35 ` Maarten Lankhorst
2017-03-27 18:24 ` Pandiyan, Dhinakaran
2017-03-16 7:10 ` [PATCH v4 5/8] drm/dp: Introduce MST topology state to track available link bandwidth Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 6/8] drm/dp: Add DP MST helpers to atomically find and release vcpi slots Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 7/8] drm: Connector helper function to release resources Dhinakaran Pandiyan
2017-03-16 8:27 ` Daniel Vetter
2017-03-16 22:43 ` [PATCH v5 " Dhinakaran Pandiyan
2017-03-16 7:10 ` [PATCH v4 8/8] drm/dp: Track MST link bandwidth Dhinakaran Pandiyan
2017-03-22 12:30 ` Maarten Lankhorst
2017-03-22 20:42 ` Pandiyan, Dhinakaran
2017-03-22 20:48 ` Daniel Vetter
2017-03-16 7:29 ` ✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state (rev3) Patchwork
2017-03-16 23:08 ` ✗ Fi.CI.BAT: failure for Adding driver-private objects to atomic state (rev4) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.