Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable; - crtc->x = crtc->primary->state->src_x >> 16; - crtc->y = crtc->primary->state->src_y >> 16; + + if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) { + crtc->x = crtc->primary->state->src_x >> 16; + crtc->y = crtc->primary->state->src_y >> 16; + }
if (crtc->state->enable) drm_calc_timestamping_constants(crtc,
On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;}
What's the benefit here of only updating when something changed? The atomic state should be the master source so copying a few too many times shouldn't matter really. -Daniel
if (crtc->state->enable) drm_calc_timestamping_constants(crtc,-- 2.1.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 16-07-15 om 11:19 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;}What's the benefit here of only updating when something changed? The atomic state should be the master source so copying a few too many times shouldn't matter really.
Because you might not be holding plane lock, so primary->state may be garbage.
~Maarten
On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
Op 16-07-15 om 11:19 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;}What's the benefit here of only updating when something changed? The atomic state should be the master source so copying a few too many times shouldn't matter really.
Because you might not be holding plane lock, so primary->state may be garbage.
Anyone who wants to touch primary plane must grab the crtc lock, so crtc lock would give you an implicit read lock. At least that's been my thinking, but it could be that the primary plane is used on some other crtc, and then this is indeed garbage.
So maybe we need even more checks than what you propose:
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) && crtc->primary->state->crtc == crtc) { crtc->x = crtc->primary->state->src_x >> 16; crtc->y = crtc->primary->state->src_y >> 16; }
I think a comment explaining this would help (or at least in the commit message!). -Daniel
Op 16-07-15 om 11:29 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
Op 16-07-15 om 11:19 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;}What's the benefit here of only updating when something changed? The atomic state should be the master source so copying a few too many times shouldn't matter really.
Because you might not be holding plane lock, so primary->state may be garbage.
Anyone who wants to touch primary plane must grab the crtc lock, so crtc lock would give you an implicit read lock. At least that's been my thinking, but it could be that the primary plane is used on some other crtc, and then this is indeed garbage.
This is only true if the plane is active. If there is none you can still update properties and swap the plane state without locking the crtc.
So maybe we need even more checks than what you propose:
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) && crtc->primary->state->crtc == crtc) { crtc->x = crtc->primary->state->src_x >> 16; crtc->y = crtc->primary->state->src_y >> 16; }I think a comment explaining this would help (or at least in the commit message!).
But the primary and cursor planes are not allowed to move between crtc's?
~Maarten
On Thu, Jul 16, 2015 at 11:38:18AM +0200, Maarten Lankhorst wrote:
Op 16-07-15 om 11:29 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
Op 16-07-15 om 11:19 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;}What's the benefit here of only updating when something changed? The atomic state should be the master source so copying a few too many times shouldn't matter really.
Because you might not be holding plane lock, so primary->state may be garbage.
Anyone who wants to touch primary plane must grab the crtc lock, so crtc lock would give you an implicit read lock. At least that's been my thinking, but it could be that the primary plane is used on some other crtc, and then this is indeed garbage.
This is only true if the plane is active. If there is none you can still update properties and swap the plane state without locking the crtc.
Ah right, so still possible to chase a being-freed primary->state pointer.
So maybe we need even more checks than what you propose:
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) && crtc->primary->state->crtc == crtc) { crtc->x = crtc->primary->state->src_x >> 16; crtc->y = crtc->primary->state->src_y >> 16; }I think a comment explaining this would help (or at least in the commit message!).
But the primary and cursor planes are not allowed to move between crtc's?
They are allowed to do that actually. crtc->primary and crtc->cursor is only really a hint to implement backwards compatibility. If you have generic plane hw with 2 crtc and planes can be freely assigned it would be silly to artificially restrict the backwards compat planes to 1 crtc. Otherwise we'd force 2 planes to be unusable when there's no external screen plugged in, defeating a lot of the value of making planes freely assignable. -Daniel
Op 16-07-15 om 14:34 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 11:38:18AM +0200, Maarten Lankhorst wrote:
Op 16-07-15 om 11:29 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
Op 16-07-15 om 11:19 schreef Daniel Vetter:
On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..70e69904291d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;}What's the benefit here of only updating when something changed? The atomic state should be the master source so copying a few too many times shouldn't matter really.
Because you might not be holding plane lock, so primary->state may be garbage.
Anyone who wants to touch primary plane must grab the crtc lock, so crtc lock would give you an implicit read lock. At least that's been my thinking, but it could be that the primary plane is used on some other crtc, and then this is indeed garbage.
This is only true if the plane is active. If there is none you can still update properties and swap the plane state without locking the crtc.
Ah right, so still possible to chase a being-freed primary->state pointer.
So maybe we need even more checks than what you propose:
if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) && crtc->primary->state->crtc == crtc) { crtc->x = crtc->primary->state->src_x >> 16; crtc->y = crtc->primary->state->src_y >> 16; }I think a comment explaining this would help (or at least in the commit message!).
But the primary and cursor planes are not allowed to move between crtc's?
They are allowed to do that actually. crtc->primary and crtc->cursor is only really a hint to implement backwards compatibility. If you have generic plane hw with 2 crtc and planes can be freely assigned it would be silly to artificially restrict the backwards compat planes to 1 crtc. Otherwise we'd force 2 planes to be unusable when there's no external screen plugged in, defeating a lot of the value of making planes freely assignable.
Ok, in that case your change looks reasonable. I'll respin.
Universal planes may not be assigned to the current crtc, so only update crtc->x/y when the primary is part of the state and bound to the current crtc.
Changes since v1: - Add the crtc check.
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e52dfc828e60..9ede58365ae1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -665,10 +665,16 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
/* set legacy state in the crtc structure */ for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + struct drm_plane *primary = crtc->primary; + crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable; - crtc->x = crtc->primary->state->src_x >> 16; - crtc->y = crtc->primary->state->src_y >> 16; + + if (drm_atomic_get_existing_plane_state(old_state, primary) && + primary->state->crtc == crtc) { + crtc->x = primary->state->src_x >> 16; + crtc->y = primary->state->src_y >> 16; + }
if (crtc->state->enable) drm_calc_timestamping_constants(crtc,
On Thu, Jul 16, 2015 at 03:51:01PM +0200, Maarten Lankhorst wrote:
Universal planes may not be assigned to the current crtc, so only update crtc->x/y when the primary is part of the state and bound to the current crtc.
Changes since v1:
- Add the crtc check.
Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Applied to topic/drm-misc, thanks. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e52dfc828e60..9ede58365ae1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -665,10 +665,16 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
/* set legacy state in the crtc structure */ for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
struct drm_plane *primary = crtc->primary;- crtc->mode = crtc->state->mode; crtc->enabled = crtc->state->enable;
crtc->x = crtc->primary->state->src_x >> 16;crtc->y = crtc->primary->state->src_y >> 16;
if (drm_atomic_get_existing_plane_state(old_state, primary) &&primary->state->crtc == crtc) {crtc->x = primary->state->src_x >> 16;crtc->y = primary->state->src_y >> 16;}if (crtc->state->enable) drm_calc_timestamping_constants(crtc,
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst