Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Mon, Jul 13, 2015 at 11:49:01AM +0200, Maarten Lankhorst wrote:
Op 13-07-15 om 11:45 schreef Daniel Vetter:
On Mon, Jul 13, 2015 at 11:23:45AM +0200, Maarten Lankhorst wrote:
Op 13-07-15 om 11:13 schreef Daniel Vetter:
On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 22:12 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst
wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst
wrote:
This allows the first atomic call during hw init to be a
real modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in
any specific
way. What exactly do you need this for, what's the
implications?
In the hw readout I noticed some warnings when I wasn't
setting any mode property in the readout.
I want the first function to be the modeset, so we have a sane
base to commit changes on.
Ideally this whole function would have a atomic counterpart
which does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can
replace all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now
because
intel_modeset_compute_config is only called in set_config so
this works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a
modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something
that takes
time like modeset or vblank wait) we need to make sure we filter
them out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out
no-op changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time.
Especially
since fbcon only force-disable non-primary plane, and for driver
load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@
drm_atomic_helper_connector_set_property(struct drm_connector
*connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property,
retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still
result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause
trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 22:12 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting
any mode property in the readout.
I want the first function to be the modeset, so we have a sane
base to commit changes on.
Ideally this whole function would have a atomic counterpart which
does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace
all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this
works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something that
takes
time like modeset or vblank wait) we need to make sure we filter them
out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out no-op
changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property,
retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in
no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause
trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Mon, Jul 13, 2015 at 11:23:45AM +0200, Maarten Lankhorst wrote:
Op 13-07-15 om 11:13 schreef Daniel Vetter:
On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 22:12 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst
wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting
any mode property in the readout.
I want the first function to be the modeset, so we have a sane
base to commit changes on.
Ideally this whole function would have a atomic counterpart
which does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can
replace all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now
because
intel_modeset_compute_config is only called in set_config so this
works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something
that takes
time like modeset or vblank wait) we need to make sure we filter
them out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out no-op
changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time.
Especially
since fbcon only force-disable non-primary plane, and for driver
load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(crtc-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(plane-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@
drm_atomic_helper_connector_set_property(struct drm_connector
*connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(connector-base, property,
retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result
in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause
trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented
once
we switch over to atomic props. Since atm
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 13-07-15 om 11:13 schreef Daniel Vetter:
On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 22:12 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting
any mode property in the readout.
I want the first function to be the modeset, so we have a sane
base to commit changes on.
Ideally this whole function would have a atomic counterpart which
does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace
all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this
works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something that
takes
time like modeset or vblank wait) we need to make sure we filter them
out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out no-op
changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property,
retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in
no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause
trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation prop to 0° again when
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 08-07-15 om 22:12 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any
mode property in the readout.
I want the first function to be the modeset, so we have a sane base
to commit changes on.
Ideally this whole function would have a atomic counterpart which
does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace
all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this
works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something that
takes
time like modeset or vblank wait) we need to make sure we filter them
out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out no-op
changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(crtc-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(plane-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(connector-base, property,
retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation prop to 0° again when fbdev starts up).
Or do I miss something still here?
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 13-07-15 om 11:45 schreef Daniel Vetter:
On Mon, Jul 13, 2015 at 11:23:45AM +0200, Maarten Lankhorst wrote:
Op 13-07-15 om 11:13 schreef Daniel Vetter:
On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 22:12 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst
wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting
any mode property in the readout.
I want the first function to be the modeset, so we have a sane
base to commit changes on.
Ideally this whole function would have a atomic counterpart
which does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can
replace all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now
because
intel_modeset_compute_config is only called in set_config so this
works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something
that takes
time like modeset or vblank wait) we need to make sure we filter
them out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out no-op
changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time.
Especially
since fbcon only force-disable non-primary plane, and for driver
load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(crtc-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(plane-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@
drm_atomic_helper_connector_set_property(struct drm_connector
*connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(connector-base, property,
retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result
in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause
trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented
once
we switch over to atomic props. Since atm we only have atomic props
which
get updated in pageflips we shouldn't have serious
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any mode
property in the readout.
I want the first function to be the modeset, so we have a sane base to
commit changes on.
Ideally this whole function would have a atomic counterpart which does
it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace all
the legacy entry points with atomic helpers, and then even plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this works as
expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double modeset.
For expensive properties (i.e. a no-op changes causes something that takes
time like modeset or vblank wait) we need to make sure we filter them out
in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
the existing legacy set_prop functions should all filter out no-op changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc
*crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane
*plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 424c83323aaa..5bab7bff8a15 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
{
int ret;
- if (!plane-fb)
+ if ((plane-state !plane-state-fb) ||
+ (!plane-state !plane-fb))
return;
Nah, atomic helpers should figure this out imo. Since if userspace does
the same (loop over all planes) then it won't go through force_disable.
-Daniel
plane-old_fb = plane-fb;
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any mode
property in the readout.
I want the first function to be the modeset, so we have a sane base to
commit changes on.
Ideally this whole function would have a atomic counterpart which does it
in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace all
the legacy entry points with atomic helpers, and then even plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this works as
expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double modeset.
For expensive properties (i.e. a no-op changes causes something that takes
time like modeset or vblank wait) we need to make sure we filter them out
in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
the existing legacy set_prop functions should all filter out no-op changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc
*crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane
*plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 424c83323aaa..5bab7bff8a15 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
{
int ret;
- if (!plane-fb)
+ if ((plane-state !plane-state-fb) ||
+ (!plane-state !plane-fb))
return;
plane-old_fb = plane-fb;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any mode
property in the readout.
I want the first function to be the modeset, so we have a sane base to
commit changes on.
Ideally this whole function would have a atomic counterpart which does
it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace all
the legacy entry points with atomic helpers, and then even plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this works as
expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double modeset.
For expensive properties (i.e. a no-op changes causes something that takes
time like modeset or vblank wait) we need to make sure we filter them out
in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
the existing legacy set_prop functions should all filter out no-op changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc
*crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(crtc-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane
*plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(plane-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(connector-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
Is that a ack or nack?
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 424c83323aaa..5bab7bff8a15 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
{
int ret;
-if (!plane-fb)
+if ((plane-state !plane-state-fb) ||
+(!plane-state !plane-fb))
return;
Nah, atomic helpers should figure this out imo. Since if userspace does
the same (loop over all planes) then it won't go through force_disable.
-Daniel
plane-old_fb = plane-fb;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any
mode property in the readout.
I want the first function to be the modeset, so we have a sane base to
commit changes on.
Ideally this whole function would have a atomic counterpart which does
it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace all
the legacy entry points with atomic helpers, and then even plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this works
as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double modeset.
For expensive properties (i.e. a no-op changes causes something that takes
time like modeset or vblank wait) we need to make sure we filter them out
in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
the existing legacy set_prop functions should all filter out no-op changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc
*crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation prop to 0° again when fbdev starts up).
Or do I miss something still here?
Yes, if the hardware mode is incompatible with its calculated sw mode,
and we set a different mode from fbdev you get 2 modesets instead of 1.
First to make the mode compatible because of the rotate_0, second to set the
new mode.
~Maarten
___
Intel-gfx mailing
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any
mode property in the readout.
I want the first function to be the modeset, so we have a sane base to
commit changes on.
Ideally this whole function would have a atomic counterpart which does
it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace all
the legacy entry points with atomic helpers, and then even plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this works
as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double modeset.
For expensive properties (i.e. a no-op changes causes something that takes
time like modeset or vblank wait) we need to make sure we filter them out
in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
the existing legacy set_prop functions should all filter out no-op changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc
*crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(crtc-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(plane-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
- int ret = 0;
+ uint64_t retval;
+ int ret;
+
+ ret = drm_atomic_get_property(connector-base, property, retval);
+ if (!ret val == retval)
+ return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation prop to 0° again when fbdev starts up).
Or do I miss something still here?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 19:52 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
Op 08-07-15 om 10:55 schreef Daniel Vetter:
On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 18:43 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 14:10 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real
modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any
specific
way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any
mode property in the readout.
I want the first function to be the modeset, so we have a sane base
to commit changes on.
Ideally this whole function would have a atomic counterpart which
does it in one go. :)
Yeah. Otoh as soon as we have atomic modeset working we can replace
all
the legacy entry points with atomic helpers, and then even
plane_disable
will be a full atomic modeset.
What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in
intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this
works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double
modeset.
For expensive properties (i.e. a no-op changes causes something that
takes
time like modeset or vblank wait) we need to make sure we filter them
out
in atomic_check. Yeah not quite there yet with pure atomic, but
meanwhile
the existing legacy set_prop functions should all filter out no-op
changes
themselves. If we don't do that for rotation then that's a bug.
Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_crtc_helper.h
#include drm/drm_atomic_helper.h
+#include drm_crtc_internal.h
#include linux/fence.h
/**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
drm_crtc *crtc,
{
struct drm_atomic_state *state;
struct drm_crtc_state *crtc_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(crtc-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(crtc-dev);
if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
drm_plane *plane,
{
struct drm_atomic_state *state;
struct drm_plane_state *plane_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(plane-base, property, retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(plane-dev);
if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
drm_connector *connector,
{
struct drm_atomic_state *state;
struct drm_connector_state *connector_state;
-int ret = 0;
+uint64_t retval;
+int ret;
+
+ret = drm_atomic_get_property(connector-base, property,
retval);
+if (!ret val == retval)
+return 0;
state = drm_atomic_state_alloc(connector-dev);
if (!state)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic-explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
Is that a ack or nack?
I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 14:10 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 11:18 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :) Yeah. Otoh as soon as we have atomic modeset working we can replace all the legacy entry points with atomic helpers, and then even plane_disable will be a full atomic modeset. What did fall apart with just touching properties/planes now? Also when i915 is fully atomic it calculates in intel_modeset_compute_config if a modeset is needed after the first atomic call. Right now because intel_modeset_compute_config is only called in set_config so this works as expected. Otherwise drm_plane_force_disable or rotate_0 will force a modeset, and if the final mode is different this will introduce a double modeset. For expensive properties (i.e. a no-op changes causes something that takes time like modeset or vblank wait) we need to make sure we filter them out in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile the existing legacy set_prop functions should all filter out no-op changes themselves. If we don't do that for rotation then that's a bug. Same for disabling planes harder, that shouldn't take time. Especially since fbcon only force-disable non-primary plane, and for driver load that's the exact thing we already do in the driver anyway. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
This allows the first atomic call during hw init to be a real modeset,
which is useful for forcing a recalculation.
fbcon is optional, you can't rely on anything being done in any specific
way. What exactly do you need this for, what's the implications?
-Daniel
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst maarten.lankho...@linux.intel.com
---
drivers/gpu/drm/drm_fb_helper.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cac422916c7a..33b5e4ecaf46 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -322,10 +322,12 @@ static bool restore_fbdev_mode(struct drm_fb_helper
*fb_helper)
drm_warn_on_modeset_not_all_locked(dev);
list_for_each_entry(plane, dev-mode_config.plane_list, head) {
- if (plane-type != DRM_PLANE_TYPE_PRIMARY)
+ if (plane-type != DRM_PLANE_TYPE_PRIMARY
+ (!plane-state || plane-state-fb))
drm_plane_force_disable(plane);
- if (dev-mode_config.rotation_property) {
+ if (dev-mode_config.rotation_property
+ (!plane-state || plane-state-rotation !=
BIT(DRM_ROTATE_0))) {
drm_mode_plane_set_obj_prop(plane,
dev-mode_config.rotation_property,
BIT(DRM_ROTATE_0));
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Tue, Jul 07, 2015 at 04:32:44PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 14:10 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 11:18 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :) Yeah. Otoh as soon as we have atomic modeset working we can replace all the legacy entry points with atomic helpers, and then even plane_disable will be a full atomic modeset. What did fall apart with just touching properties/planes now? Setting rotation on the primary plane caused it to be disabled because the src and dst rectangle are not set up yet until the modeset. So the check function saw that the plane should be invisible and performs the update. Sounds like a bug - we need to recreate more of the primary plane state. Or maybe we need to call the atomic_check function for the primary plane to compute all that derived state. But we really can't rely upon userspace to do a modeset first, e.g. X at start (if there's no fbcon) loves to read and then write back all the properties (or at least did). We really need to handle this in the backend properly. It's also an extra vblank wait when the primary plane is visible. Another bug, no-op changes with the same fb should not result in a vblank wait. At least not when using the helpers (or if there is a case I need to copypaste the fix again). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 07-07-15 om 14:10 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 11:18 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :) Yeah. Otoh as soon as we have atomic modeset working we can replace all the legacy entry points with atomic helpers, and then even plane_disable will be a full atomic modeset. What did fall apart with just touching properties/planes now? Also when i915 is fully atomic it calculates in intel_modeset_compute_config if a modeset is needed after the first atomic call. Right now because intel_modeset_compute_config is only called in set_config so this works as expected. Otherwise drm_plane_force_disable or rotate_0 will force a modeset, and if the final mode is different this will introduce a double modeset. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 11:18 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :) Yeah. Otoh as soon as we have atomic modeset working we can replace all the legacy entry points with atomic helpers, and then even plane_disable will be a full atomic modeset. What did fall apart with just touching properties/planes now? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 07-07-15 om 11:18 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same
Op 07-07-15 om 14:10 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: Op 07-07-15 om 11:18 schreef Daniel Vetter: On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :) Yeah. Otoh as soon as we have atomic modeset working we can replace all the legacy entry points with atomic helpers, and then even plane_disable will be a full atomic modeset. What did fall apart with just touching properties/planes now? Setting rotation on the primary plane caused it to be disabled because the src and dst rectangle are not set up yet until the modeset. So the check function saw that the plane should be invisible and performs the update. It's also an extra vblank wait when the primary plane is visible. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
