* [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 18:53 ` Sam Ravnborg
2019-06-11 16:08 ` [PATCH v5 02/11] drm: Add drm_atomic_get_(old|new_connector_for_encoder() helpers Sean Paul
` (10 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel; +Cc: Maxime Ripard, David Airlie, Sean Paul, Sean Paul
From: Sean Paul <seanpaul@chromium.org>
This patch adds atomic_enable and atomic_disable callbacks to the
encoder helpers. This will allow encoders to make informed decisions in
their start-up/shutdown based on the committed state.
Aside from the new hooks, this patch also introduces the new signature
for .atomic_* functions going forward. Instead of passing object state
(well, encoders don't have atomic state, but let's ignore that), we pass
the entire atomic state so the driver can inspect more than what's
happening locally.
This is particularly important for the upcoming self refresh helpers.
Changes in v3:
- Added patch to the set
Changes in v4:
- Move atomic_disable above prepare (Daniel)
- Add breadcrumb to .enable() docbook (Daniel)
Changes in v5:
- None
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-2-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-2-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 8 +++-
include/drm/drm_modeset_helper_vtables.h | 48 ++++++++++++++++++++++++
2 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0fc63d6822451..62e29b5ebb6e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1002,7 +1002,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
/* Right function depends upon target state. */
if (funcs) {
- if (new_conn_state->crtc && funcs->prepare)
+ if (funcs->atomic_disable)
+ funcs->atomic_disable(encoder, old_state);
+ else if (new_conn_state->crtc && funcs->prepare)
funcs->prepare(encoder);
else if (funcs->disable)
funcs->disable(encoder);
@@ -1311,7 +1313,9 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
drm_bridge_pre_enable(encoder->bridge);
if (funcs) {
- if (funcs->enable)
+ if (funcs->atomic_enable)
+ funcs->atomic_enable(encoder, old_state);
+ else if (funcs->enable)
funcs->enable(encoder);
else if (funcs->commit)
funcs->commit(encoder);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index f9c94c2a13646..0701b3a3108e5 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -679,6 +679,51 @@ struct drm_encoder_helper_funcs {
enum drm_connector_status (*detect)(struct drm_encoder *encoder,
struct drm_connector *connector);
+ /**
+ * @atomic_disable:
+ *
+ * This callback should be used to disable the encoder. With the atomic
+ * drivers it is called before this encoder's CRTC has been shut off
+ * using their own &drm_crtc_helper_funcs.atomic_disable hook. If that
+ * sequence is too simple drivers can just add their own driver private
+ * encoder hooks and call them from CRTC's callback by looping over all
+ * encoders connected to it using for_each_encoder_on_crtc().
+ *
+ * This callback is a variant of @disable that provides the atomic state
+ * to the driver. It takes priority over @disable during atomic commits.
+ *
+ * This hook is used only by atomic helpers. Atomic drivers don't need
+ * to implement it if there's no need to disable anything at the encoder
+ * level. To ensure that runtime PM handling (using either DPMS or the
+ * new "ACTIVE" property) works @atomic_disable must be the inverse of
+ * @atomic_enable.
+ */
+ void (*atomic_disable)(struct drm_encoder *encoder,
+ struct drm_atomic_state *state);
+
+ /**
+ * @atomic_enable:
+ *
+ * This callback should be used to enable the encoder. It is called
+ * after this encoder's CRTC has been enabled using their own
+ * &drm_crtc_helper_funcs.atomic_enable hook. If that sequence is
+ * too simple drivers can just add their own driver private encoder
+ * hooks and call them from CRTC's callback by looping over all encoders
+ * connected to it using for_each_encoder_on_crtc().
+ *
+ * This callback is a variant of @enable that provides the atomic state
+ * to the driver. It is called in place of @enable during atomic
+ * commits.
+ *
+ * This hook is used only by atomic helpers, for symmetry with @disable.
+ * Atomic drivers don't need to implement it if there's no need to
+ * enable anything at the encoder level. To ensure that runtime PM
+ * handling (using either DPMS or the new "ACTIVE" property) works
+ * @enable must be the inverse of @disable for atomic drivers.
+ */
+ void (*atomic_enable)(struct drm_encoder *encoder,
+ struct drm_atomic_state *state);
+
/**
* @disable:
*
@@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
* handling (using either DPMS or the new "ACTIVE" property) works
* @disable must be the inverse of @enable for atomic drivers.
*
+ * For atomic drivers also consider @atomic_disable and save yourself
+ * from having to read the NOTE below!
+ *
* NOTE:
*
* With legacy CRTC helpers there's a big semantic difference between
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs
2019-06-11 16:08 ` [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs Sean Paul
@ 2019-06-11 18:53 ` Sam Ravnborg
2019-06-11 20:22 ` Sean Paul
2019-06-11 20:49 ` [PATCH v6 " Sean Paul
0 siblings, 2 replies; 27+ messages in thread
From: Sam Ravnborg @ 2019-06-11 18:53 UTC (permalink / raw)
To: Sean Paul; +Cc: Maxime Ripard, David Airlie, Sean Paul, dri-devel
Hi Sean.
Nits below.
>
> + /**
> + * @atomic_disable:
> + *
...
> + *
> + * This callback is a variant of @disable that provides the atomic state
> + * to the driver. It takes priority over @disable during atomic commits.
> + *
> + * This hook is used only by atomic helpers. Atomic drivers don't need
> + * to implement it if there's no need to disable anything at the encoder
> + * level. To ensure that runtime PM handling (using either DPMS or the
> + * new "ACTIVE" property) works @atomic_disable must be the inverse of
> + * @atomic_enable.
> + */
> + void (*atomic_disable)(struct drm_encoder *encoder,
> + struct drm_atomic_state *state);
> +
> + /**
> + * @atomic_enable:
> + *
...
> + *
> + * This callback is a variant of @enable that provides the atomic state
> + * to the driver. It is called in place of @enable during atomic
> + * commits.
The wording in this paragrap is not the same as the similar paragraph
above.
One says "it takes priority over"
Another says "called in place of"
Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
define then {dis,en}able is not called?
> + *
> + * This hook is used only by atomic helpers, for symmetry with @disable.
I do not get the "for symmetry with @disable."?
> + * Atomic drivers don't need to implement it if there's no need to
> + * enable anything at the encoder level. To ensure that runtime PM
> + * handling (using either DPMS or the new "ACTIVE" property) works
> + * @enable must be the inverse of @disable for atomic drivers.
Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?
> + */
> + void (*atomic_enable)(struct drm_encoder *encoder,
> + struct drm_atomic_state *state);
> +
> /**
> * @disable:
> *
> @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
> * handling (using either DPMS or the new "ACTIVE" property) works
> * @disable must be the inverse of @enable for atomic drivers.
> *
> + * For atomic drivers also consider @atomic_disable and save yourself
> + * from having to read the NOTE below!
Maybe, if this is so, say that atomic drivers shall alwyas use the
atomic_* variants?
And then add a TODO entry to make it so for the other atomic drivers?
> + *
> * NOTE:
> *
> * With legacy CRTC helpers there's a big semantic difference between
Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs
2019-06-11 18:53 ` Sam Ravnborg
@ 2019-06-11 20:22 ` Sean Paul
2019-06-11 20:49 ` [PATCH v6 " Sean Paul
1 sibling, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 20:22 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: Maxime Ripard, David Airlie, Sean Paul, Sean Paul, dri-devel
On Tue, Jun 11, 2019 at 08:53:52PM +0200, Sam Ravnborg wrote:
> Hi Sean.
>
> Nits below.
>
> >
> > + /**
> > + * @atomic_disable:
> > + *
> ...
> > + *
> > + * This callback is a variant of @disable that provides the atomic state
> > + * to the driver. It takes priority over @disable during atomic commits.
> > + *
> > + * This hook is used only by atomic helpers. Atomic drivers don't need
> > + * to implement it if there's no need to disable anything at the encoder
> > + * level. To ensure that runtime PM handling (using either DPMS or the
> > + * new "ACTIVE" property) works @atomic_disable must be the inverse of
> > + * @atomic_enable.
> > + */
> > + void (*atomic_disable)(struct drm_encoder *encoder,
> > + struct drm_atomic_state *state);
>
>
>
> > +
> > + /**
> > + * @atomic_enable:
> > + *
> ...
> > + *
> > + * This callback is a variant of @enable that provides the atomic state
> > + * to the driver. It is called in place of @enable during atomic
> > + * commits.
>
> The wording in this paragrap is not the same as the similar paragraph
> above.
>
> One says "it takes priority over"
> Another says "called in place of"
>
> Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
> define then {dis,en}able is not called?
Sure, can make that change
>
>
> > + *
> > + * This hook is used only by atomic helpers, for symmetry with @disable.
> I do not get the "for symmetry with @disable."?
Copypasta from @enable kerneldoc. I'll change this up and submit a new patch to
fix up @enable to match.
>
> > + * Atomic drivers don't need to implement it if there's no need to
> > + * enable anything at the encoder level. To ensure that runtime PM
> > + * handling (using either DPMS or the new "ACTIVE" property) works
> > + * @enable must be the inverse of @disable for atomic drivers.
> Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?
Yes I did. I'll also clean up some of the surrounding text in the next version.
>
> > + */
> > + void (*atomic_enable)(struct drm_encoder *encoder,
> > + struct drm_atomic_state *state);
> > +
> > /**
> > * @disable:
> > *
> > @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
> > * handling (using either DPMS or the new "ACTIVE" property) works
> > * @disable must be the inverse of @enable for atomic drivers.
> > *
> > + * For atomic drivers also consider @atomic_disable and save yourself
> > + * from having to read the NOTE below!
> Maybe, if this is so, say that atomic drivers shall alwyas use the
> atomic_* variants?
> And then add a TODO entry to make it so for the other atomic drivers?
I don't think that's quite what we want. For atomic drivers it might be
preferable to use enable and disable if they don't need to inspect state.
The NOTE only applies to drivers using the legacy helpers, which isn't really
applicable for anyone making the decision between disable and atomic_disable.
Sean
> > + *
> > * NOTE:
> > *
> > * With legacy CRTC helpers there's a big semantic difference between
>
>
> Sam
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v6 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs
2019-06-11 18:53 ` Sam Ravnborg
2019-06-11 20:22 ` Sean Paul
@ 2019-06-11 20:49 ` Sean Paul
1 sibling, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 20:49 UTC (permalink / raw)
To: dri-devel; +Cc: Maxime Ripard, Sean Paul, David Airlie, Sean Paul, Sam Ravnborg
From: Sean Paul <seanpaul@chromium.org>
This patch adds atomic_enable and atomic_disable callbacks to the
encoder helpers. This will allow encoders to make informed decisions in
their start-up/shutdown based on the committed state.
Aside from the new hooks, this patch also introduces the new signature
for .atomic_* functions going forward. Instead of passing object state
(well, encoders don't have atomic state, but let's ignore that), we pass
the entire atomic state so the driver can inspect more than what's
happening locally.
This is particularly important for the upcoming self refresh helpers.
Changes in v3:
- Added patch to the set
Changes in v4:
- Move atomic_disable above prepare (Daniel)
- Add breadcrumb to .enable() docbook (Daniel)
Changes in v5:
- None
Changes in v6:
- Tweak kerneldoc some more (Sam)
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-2-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-2-sean@poorly.run
Link to v5: https://patchwork.freedesktop.org/patch/msgid/20190611160844.257498-2-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 8 +++-
include/drm/drm_modeset_helper_vtables.h | 49 ++++++++++++++++++++++++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0fc63d6822451..62e29b5ebb6e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1002,7 +1002,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
/* Right function depends upon target state. */
if (funcs) {
- if (new_conn_state->crtc && funcs->prepare)
+ if (funcs->atomic_disable)
+ funcs->atomic_disable(encoder, old_state);
+ else if (new_conn_state->crtc && funcs->prepare)
funcs->prepare(encoder);
else if (funcs->disable)
funcs->disable(encoder);
@@ -1311,7 +1313,9 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
drm_bridge_pre_enable(encoder->bridge);
if (funcs) {
- if (funcs->enable)
+ if (funcs->atomic_enable)
+ funcs->atomic_enable(encoder, old_state);
+ else if (funcs->enable)
funcs->enable(encoder);
else if (funcs->commit)
funcs->commit(encoder);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index f9c94c2a13646..4cf01cd64b0cf 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -679,6 +679,52 @@ struct drm_encoder_helper_funcs {
enum drm_connector_status (*detect)(struct drm_encoder *encoder,
struct drm_connector *connector);
+ /**
+ * @atomic_disable:
+ *
+ * This callback should be used to disable the encoder. With the atomic
+ * drivers it is called before this encoder's CRTC has been shut off
+ * using their own &drm_crtc_helper_funcs.atomic_disable hook. If that
+ * sequence is too simple drivers can just add their own driver private
+ * encoder hooks and call them from CRTC's callback by looping over all
+ * encoders connected to it using for_each_encoder_on_crtc().
+ *
+ * This callback is a variant of @disable that provides the atomic state
+ * to the driver. If @atomic_disable is implemented, @disable is not
+ * called by the helpers.
+ *
+ * This hook is only used by atomic helpers. Atomic drivers don't need
+ * to implement it if there's no need to disable anything at the encoder
+ * level. To ensure that runtime PM handling (using either DPMS or the
+ * new "ACTIVE" property) works @atomic_disable must be the inverse of
+ * @atomic_enable.
+ */
+ void (*atomic_disable)(struct drm_encoder *encoder,
+ struct drm_atomic_state *state);
+
+ /**
+ * @atomic_enable:
+ *
+ * This callback should be used to enable the encoder. It is called
+ * after this encoder's CRTC has been enabled using their own
+ * &drm_crtc_helper_funcs.atomic_enable hook. If that sequence is
+ * too simple drivers can just add their own driver private encoder
+ * hooks and call them from CRTC's callback by looping over all encoders
+ * connected to it using for_each_encoder_on_crtc().
+ *
+ * This callback is a variant of @enable that provides the atomic state
+ * to the driver. If @atomic_enable is implemented, @enable is not
+ * called by the helpers.
+ *
+ * This hook is only used by atomic helpers, it is the opposite of
+ * @atomic_disable. Atomic drivers don't need to implement it if there's
+ * no need to enable anything at the encoder level. To ensure that
+ * runtime PM handling works @atomic_enable must be the inverse of
+ * @atomic_disable.
+ */
+ void (*atomic_enable)(struct drm_encoder *encoder,
+ struct drm_atomic_state *state);
+
/**
* @disable:
*
@@ -695,6 +741,9 @@ struct drm_encoder_helper_funcs {
* handling (using either DPMS or the new "ACTIVE" property) works
* @disable must be the inverse of @enable for atomic drivers.
*
+ * For atomic drivers also consider @atomic_disable and save yourself
+ * from having to read the NOTE below!
+ *
* NOTE:
*
* With legacy CRTC helpers there's a big semantic difference between
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v5 02/11] drm: Add drm_atomic_get_(old|new_connector_for_encoder() helpers
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
2019-06-11 16:08 ` [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 18:57 ` Sam Ravnborg
2019-06-11 16:08 ` [PATCH v5 03/11] drm: Add atomic variants for bridge enable/disable Sean Paul
` (9 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Maxime Ripard, David Airlie, Sean Paul, Laurent Pinchart,
Sean Paul
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Add functions to the atomic core to retrieve the old and new connectors
associated with an encoder in a drm_atomic_state. This is useful for
encoders and bridges that need to access the connector, for instance for
the drm_display_info.
The CRTC associated with the encoder can also be retrieved through the
connector state, and from it, the old and new CRTC states.
Changed in v4:
- Added to the set
Changed in v5:
- Fix up docbook (Daniel & Laurent)
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-3-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[seanpaul removed WARNs from helpers and added docs to explain why
returning NULL might be valid]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_atomic.c | 69 ++++++++++++++++++++++++++++++++++++
include/drm/drm_atomic.h | 7 ++++
include/drm/drm_connector.h | 5 +++
3 files changed, 81 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 169f06d7c5ce4..a4e779deab0fb 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -846,6 +846,75 @@ drm_atomic_get_new_private_obj_state(struct drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_atomic_get_new_private_obj_state);
+/**
+ * drm_atomic_get_old_connector_for_encoder - Get old connector for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the connector state for
+ *
+ * This function finds and returns the connector that was connected to @encoder
+ * as specified by the @state.
+ *
+ * If there is no connector in @state which previously had @encoder connected to
+ * it, this function will return NULL. While this may seem like an invalid use
+ * case, it is sometimes useful to differentiate commits which had no prior
+ * connectors attached to @encoder vs ones that did (and to inspect their
+ * state). This is especially true in enable hooks because the pipeline has
+ * changed.
+ *
+ * Returns: The old connector connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_connector *
+drm_atomic_get_old_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+ unsigned int i;
+
+ for_each_old_connector_in_state(state, connector, conn_state, i) {
+ if (conn_state->best_encoder == encoder)
+ return connector;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(drm_atomic_get_old_connector_for_encoder);
+
+/**
+ * drm_atomic_get_new_connector_for_encoder - Get new connector for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the connector state for
+ *
+ * This function finds and returns the connector that will be connected to
+ * @encoder as specified by the @state.
+ *
+ * If there is no connector in @state which will have @encoder connected to it,
+ * this function will return NULL. While this may seem like an invalid use case,
+ * it is sometimes useful to differentiate commits which have no connectors
+ * attached to @encoder vs ones that do (and to inspect their state). This is
+ * especially true in disable hooks because the pipeline will change.
+ *
+ * Returns: The new connector connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_connector *
+drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+ unsigned int i;
+
+ for_each_new_connector_in_state(state, connector, conn_state, i) {
+ if (conn_state->best_encoder == encoder)
+ return connector;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
+
/**
* drm_atomic_get_connector_state - get connector state
* @state: global atomic state object
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e937ff2beb04f..f122156478010 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -459,6 +459,13 @@ struct drm_private_state *
drm_atomic_get_new_private_obj_state(struct drm_atomic_state *state,
struct drm_private_obj *obj);
+struct drm_connector *
+drm_atomic_get_old_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder);
+struct drm_connector *
+drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder);
+
/**
* drm_atomic_get_existing_crtc_state - get crtc state, if it exists
* @state: global atomic state object
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 47e749b74e5fc..071143bc0ebd1 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -518,6 +518,11 @@ struct drm_connector_state {
* &drm_connector_helper_funcs.atomic_best_encoder or
* &drm_connector_helper_funcs.best_encoder callbacks.
*
+ * This is also used in the atomic helpers to map encoders to their
+ * current and previous connectors, see
+ * &drm_atomic_get_old_connector_for_encoder() and
+ * &drm_atomic_get_new_connector_for_encoder().
+ *
* NOTE: Atomic drivers must fill this out (either themselves or through
* helpers), for otherwise the GETCONNECTOR and GETENCODER IOCTLs will
* not return correct data to userspace.
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 02/11] drm: Add drm_atomic_get_(old|new_connector_for_encoder() helpers
2019-06-11 16:08 ` [PATCH v5 02/11] drm: Add drm_atomic_get_(old|new_connector_for_encoder() helpers Sean Paul
@ 2019-06-11 18:57 ` Sam Ravnborg
2019-06-11 20:51 ` [PATCH v6 02/11] drm: Add drm_atomic_get_(old|new)_connector_for_encoder() helpers Sean Paul
0 siblings, 1 reply; 27+ messages in thread
From: Sam Ravnborg @ 2019-06-11 18:57 UTC (permalink / raw)
To: Sean Paul
Cc: Maxime Ripard, David Airlie, Sean Paul, Laurent Pinchart,
dri-devel
Hi Sean.
I got a parse error due to missing ')' in $subject :-)
The rest (changelog, patch) looked fine to me.
Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v6 02/11] drm: Add drm_atomic_get_(old|new)_connector_for_encoder() helpers
2019-06-11 18:57 ` Sam Ravnborg
@ 2019-06-11 20:51 ` Sean Paul
0 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 20:51 UTC (permalink / raw)
To: dri-devel
Cc: Maxime Ripard, Sean Paul, David Airlie, Sean Paul,
Laurent Pinchart, Sam Ravnborg
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Add functions to the atomic core to retrieve the old and new connectors
associated with an encoder in a drm_atomic_state. This is useful for
encoders and bridges that need to access the connector, for instance for
the drm_display_info.
The CRTC associated with the encoder can also be retrieved through the
connector state, and from it, the old and new CRTC states.
Changed in v4:
- Added to the set
Changed in v5:
- Fix up docbook (Daniel & Laurent)
Changed in v6:
- Updated commit subject (Sam)
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-3-sean@poorly.run
Link to v5: https://patchwork.freedesktop.org/patch/msgid/20190611160844.257498-3-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[seanpaul removed WARNs from helpers and added docs to explain why
returning NULL might be valid]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_atomic.c | 69 ++++++++++++++++++++++++++++++++++++
include/drm/drm_atomic.h | 7 ++++
include/drm/drm_connector.h | 5 +++
3 files changed, 81 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 169f06d7c5ce4..a4e779deab0fb 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -846,6 +846,75 @@ drm_atomic_get_new_private_obj_state(struct drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_atomic_get_new_private_obj_state);
+/**
+ * drm_atomic_get_old_connector_for_encoder - Get old connector for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the connector state for
+ *
+ * This function finds and returns the connector that was connected to @encoder
+ * as specified by the @state.
+ *
+ * If there is no connector in @state which previously had @encoder connected to
+ * it, this function will return NULL. While this may seem like an invalid use
+ * case, it is sometimes useful to differentiate commits which had no prior
+ * connectors attached to @encoder vs ones that did (and to inspect their
+ * state). This is especially true in enable hooks because the pipeline has
+ * changed.
+ *
+ * Returns: The old connector connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_connector *
+drm_atomic_get_old_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+ unsigned int i;
+
+ for_each_old_connector_in_state(state, connector, conn_state, i) {
+ if (conn_state->best_encoder == encoder)
+ return connector;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(drm_atomic_get_old_connector_for_encoder);
+
+/**
+ * drm_atomic_get_new_connector_for_encoder - Get new connector for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the connector state for
+ *
+ * This function finds and returns the connector that will be connected to
+ * @encoder as specified by the @state.
+ *
+ * If there is no connector in @state which will have @encoder connected to it,
+ * this function will return NULL. While this may seem like an invalid use case,
+ * it is sometimes useful to differentiate commits which have no connectors
+ * attached to @encoder vs ones that do (and to inspect their state). This is
+ * especially true in disable hooks because the pipeline will change.
+ *
+ * Returns: The new connector connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_connector *
+drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector_state *conn_state;
+ struct drm_connector *connector;
+ unsigned int i;
+
+ for_each_new_connector_in_state(state, connector, conn_state, i) {
+ if (conn_state->best_encoder == encoder)
+ return connector;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
+
/**
* drm_atomic_get_connector_state - get connector state
* @state: global atomic state object
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e937ff2beb04f..f122156478010 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -459,6 +459,13 @@ struct drm_private_state *
drm_atomic_get_new_private_obj_state(struct drm_atomic_state *state,
struct drm_private_obj *obj);
+struct drm_connector *
+drm_atomic_get_old_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder);
+struct drm_connector *
+drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder);
+
/**
* drm_atomic_get_existing_crtc_state - get crtc state, if it exists
* @state: global atomic state object
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 47e749b74e5fc..071143bc0ebd1 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -518,6 +518,11 @@ struct drm_connector_state {
* &drm_connector_helper_funcs.atomic_best_encoder or
* &drm_connector_helper_funcs.best_encoder callbacks.
*
+ * This is also used in the atomic helpers to map encoders to their
+ * current and previous connectors, see
+ * &drm_atomic_get_old_connector_for_encoder() and
+ * &drm_atomic_get_new_connector_for_encoder().
+ *
* NOTE: Atomic drivers must fill this out (either themselves or through
* helpers), for otherwise the GETCONNECTOR and GETENCODER IOCTLs will
* not return correct data to userspace.
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v5 03/11] drm: Add atomic variants for bridge enable/disable
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
2019-06-11 16:08 ` [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs Sean Paul
2019-06-11 16:08 ` [PATCH v5 02/11] drm: Add drm_atomic_get_(old|new_connector_for_encoder() helpers Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 16:08 ` [PATCH v5 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul
` (8 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Maxime Ripard, Daniel Vetter, David Airlie, Sean Paul, Sean Paul
From: Sean Paul <seanpaul@chromium.org>
This patch adds atomic variants for all of
pre_enable/enable/disable/post_disable bridge functions. These will be
called from the appropriate atomic helper functions. If the bridge
driver doesn't implement the atomic version of the function, we will
fall back to the vanilla implementation.
Note that some drivers call drm_bridge_disable directly, and these cases
are not covered. It's up to the driver to decide whether to implement
both atomic_disable and disable, or if it's not necessary.
Changes in v3:
- Added to the patchset
Changes in v4:
- Fix up docbook references (Daniel)
Changes in v5:
- None
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-4-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-4-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 8 +-
drivers/gpu/drm/drm_bridge.c | 110 ++++++++++++++++++++++++++++
include/drm/drm_bridge.h | 106 +++++++++++++++++++++++++++
3 files changed, 220 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 62e29b5ebb6e3..2133f62539176 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -998,7 +998,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
* Each encoder has at most one connector (since we always steal
* it away), so we won't call disable hooks twice.
*/
- drm_bridge_disable(encoder->bridge);
+ drm_atomic_bridge_disable(encoder->bridge, old_state);
/* Right function depends upon target state. */
if (funcs) {
@@ -1012,7 +1012,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
}
- drm_bridge_post_disable(encoder->bridge);
+ drm_atomic_bridge_post_disable(encoder->bridge, old_state);
}
for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1310,7 +1310,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
* Each encoder has at most one connector (since we always steal
* it away), so we won't call enable hooks twice.
*/
- drm_bridge_pre_enable(encoder->bridge);
+ drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
if (funcs) {
if (funcs->atomic_enable)
@@ -1321,7 +1321,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
funcs->commit(encoder);
}
- drm_bridge_enable(encoder->bridge);
+ drm_atomic_bridge_enable(encoder->bridge, old_state);
}
drm_atomic_helper_commit_writebacks(dev, old_state);
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 138b2711d389e..cba537c99e437 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -352,6 +352,116 @@ void drm_bridge_enable(struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_enable);
+/**
+ * drm_atomic_bridge_disable - disables all bridges in the encoder chain
+ * @bridge: bridge control structure
+ * @state: atomic state being committed
+ *
+ * Calls &drm_bridge_funcs.atomic_disable (falls back on
+ * &drm_bridge_funcs.disable) op for all the bridges in the encoder chain,
+ * starting from the last bridge to the first. These are called before calling
+ * &drm_encoder_helper_funcs.atomic_disable
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_atomic_bridge_disable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
+{
+ if (!bridge)
+ return;
+
+ drm_atomic_bridge_disable(bridge->next, state);
+
+ if (bridge->funcs->atomic_disable)
+ bridge->funcs->atomic_disable(bridge, state);
+ else if (bridge->funcs->disable)
+ bridge->funcs->disable(bridge);
+}
+EXPORT_SYMBOL(drm_atomic_bridge_disable);
+
+/**
+ * drm_atomic_bridge_post_disable - cleans up after disabling all bridges in the
+ * encoder chain
+ * @bridge: bridge control structure
+ * @state: atomic state being committed
+ *
+ * Calls &drm_bridge_funcs.atomic_post_disable (falls back on
+ * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
+ * starting from the first bridge to the last. These are called after completing
+ * &drm_encoder_helper_funcs.atomic_disable
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
+{
+ if (!bridge)
+ return;
+
+ if (bridge->funcs->atomic_post_disable)
+ bridge->funcs->atomic_post_disable(bridge, state);
+ else if (bridge->funcs->post_disable)
+ bridge->funcs->post_disable(bridge);
+
+ drm_atomic_bridge_post_disable(bridge->next, state);
+}
+EXPORT_SYMBOL(drm_atomic_bridge_post_disable);
+
+/**
+ * drm_atomic_bridge_pre_enable - prepares for enabling all bridges in the
+ * encoder chain
+ * @bridge: bridge control structure
+ * @state: atomic state being committed
+ *
+ * Calls &drm_bridge_funcs.atomic_pre_enable (falls back on
+ * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
+ * starting from the last bridge to the first. These are called before calling
+ * &drm_encoder_helper_funcs.atomic_enable
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
+{
+ if (!bridge)
+ return;
+
+ drm_atomic_bridge_pre_enable(bridge->next, state);
+
+ if (bridge->funcs->atomic_pre_enable)
+ bridge->funcs->atomic_pre_enable(bridge, state);
+ else if (bridge->funcs->pre_enable)
+ bridge->funcs->pre_enable(bridge);
+}
+EXPORT_SYMBOL(drm_atomic_bridge_pre_enable);
+
+/**
+ * drm_atomic_bridge_enable - enables all bridges in the encoder chain
+ * @bridge: bridge control structure
+ * @state: atomic state being committed
+ *
+ * Calls &drm_bridge_funcs.atomic_enable (falls back on
+ * &drm_bridge_funcs.enable) op for all the bridges in the encoder chain,
+ * starting from the first bridge to the last. These are called after completing
+ * &drm_encoder_helper_funcs.atomic_enable
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_atomic_bridge_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
+{
+ if (!bridge)
+ return;
+
+ if (bridge->funcs->atomic_enable)
+ bridge->funcs->atomic_enable(bridge, state);
+ else if (bridge->funcs->enable)
+ bridge->funcs->enable(bridge);
+
+ drm_atomic_bridge_enable(bridge->next, state);
+}
+EXPORT_SYMBOL(drm_atomic_bridge_enable);
+
#ifdef CONFIG_OF
/**
* of_drm_find_bridge - find the bridge corresponding to the device node in
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index d4428913a4e14..3228018848141 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -237,6 +237,103 @@ struct drm_bridge_funcs {
* The enable callback is optional.
*/
void (*enable)(struct drm_bridge *bridge);
+
+ /**
+ * @atomic_pre_enable:
+ *
+ * This callback should enable the bridge. It is called right before
+ * the preceding element in the display pipe is enabled. If the
+ * preceding element is a bridge this means it's called before that
+ * bridge's @atomic_pre_enable or @pre_enable function. If the preceding
+ * element is a &drm_encoder it's called right before the encoder's
+ * &drm_encoder_helper_funcs.atomic_enable hook.
+ *
+ * The display pipe (i.e. clocks and timing signals) feeding this bridge
+ * will not yet be running when this callback is called. The bridge must
+ * not enable the display link feeding the next bridge in the chain (if
+ * there is one) when this callback is called.
+ *
+ * Note that this function will only be invoked in the context of an
+ * atomic commit. It will not be invoked from &drm_bridge_pre_enable. It
+ * would be prudent to also provide an implementation of @pre_enable if
+ * you are expecting driver calls into &drm_bridge_pre_enable.
+ *
+ * The @atomic_pre_enable callback is optional.
+ */
+ void (*atomic_pre_enable)(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+
+ /**
+ * @atomic_enable:
+ *
+ * This callback should enable the bridge. It is called right after
+ * the preceding element in the display pipe is enabled. If the
+ * preceding element is a bridge this means it's called after that
+ * bridge's @atomic_enable or @enable function. If the preceding element
+ * is a &drm_encoder it's called right after the encoder's
+ * &drm_encoder_helper_funcs.atomic_enable hook.
+ *
+ * The bridge can assume that the display pipe (i.e. clocks and timing
+ * signals) feeding it is running when this callback is called. This
+ * callback must enable the display link feeding the next bridge in the
+ * chain if there is one.
+ *
+ * Note that this function will only be invoked in the context of an
+ * atomic commit. It will not be invoked from &drm_bridge_enable. It
+ * would be prudent to also provide an implementation of @enable if
+ * you are expecting driver calls into &drm_bridge_enable.
+ *
+ * The enable callback is optional.
+ */
+ void (*atomic_enable)(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+ /**
+ * @atomic_disable:
+ *
+ * This callback should disable the bridge. It is called right before
+ * the preceding element in the display pipe is disabled. If the
+ * preceding element is a bridge this means it's called before that
+ * bridge's @atomic_disable or @disable vfunc. If the preceding element
+ * is a &drm_encoder it's called right before the
+ * &drm_encoder_helper_funcs.atomic_disable hook.
+ *
+ * The bridge can assume that the display pipe (i.e. clocks and timing
+ * signals) feeding it is still running when this callback is called.
+ *
+ * Note that this function will only be invoked in the context of an
+ * atomic commit. It will not be invoked from &drm_bridge_disable. It
+ * would be prudent to also provide an implementation of @disable if
+ * you are expecting driver calls into &drm_bridge_disable.
+ *
+ * The disable callback is optional.
+ */
+ void (*atomic_disable)(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+
+ /**
+ * @atomic_post_disable:
+ *
+ * This callback should disable the bridge. It is called right after the
+ * preceding element in the display pipe is disabled. If the preceding
+ * element is a bridge this means it's called after that bridge's
+ * @atomic_post_disable or @post_disable function. If the preceding
+ * element is a &drm_encoder it's called right after the encoder's
+ * &drm_encoder_helper_funcs.atomic_disable hook.
+ *
+ * The bridge must assume that the display pipe (i.e. clocks and timing
+ * signals) feeding it is no longer running when this callback is
+ * called.
+ *
+ * Note that this function will only be invoked in the context of an
+ * atomic commit. It will not be invoked from &drm_bridge_post_disable.
+ * It would be prudent to also provide an implementation of
+ * @post_disable if you are expecting driver calls into
+ * &drm_bridge_post_disable.
+ *
+ * The post_disable callback is optional.
+ */
+ void (*atomic_post_disable)(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
};
/**
@@ -314,6 +411,15 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
void drm_bridge_pre_enable(struct drm_bridge *bridge);
void drm_bridge_enable(struct drm_bridge *bridge);
+void drm_atomic_bridge_disable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+void drm_atomic_bridge_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state);
+
#ifdef CONFIG_DRM_PANEL_BRIDGE
struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
u32 connector_type);
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (2 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 03/11] drm: Add atomic variants for bridge enable/disable Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 17:58 ` Laurent Pinchart
2019-06-11 16:08 ` [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers Sean Paul
` (7 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Maxime Ripard, nouveau, David Airlie, linux-renesas-soc,
Kieran Bingham, Sean Paul, Ben Skeggs, Rodrigo Vivi, Sean Paul,
intel-gfx, Laurent Pinchart
From: Sean Paul <seanpaul@chromium.org>
Everyone who implements connector_helper_funcs->atomic_check reaches
into the connector state to get the atomic state. Instead of continuing
this pattern, change the callback signature to just give atomic state
and let the driver determine what it does and does not need from it.
Eventually all atomic functions should do this, but that's just too much
busy work for me.
Changes in v3:
- Added to the set
Changes in v4:
- None
Changes in v5:
- intel_digital_connector_atomic_check declaration moved to i915_atomic.h
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-5-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-5-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Eric Anholt <eric@anholt.net>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 4 ++--
drivers/gpu/drm/i915/intel_atomic.c | 8 +++++---
drivers/gpu/drm/i915/intel_atomic.h | 2 +-
drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++---
drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++----
drivers/gpu/drm/i915/intel_tv.c | 8 +++++---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++--
drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++-----
drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++---
include/drm/drm_modeset_helper_vtables.h | 2 +-
10 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2133f62539176..e58be69960692 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -686,7 +686,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
if (funcs->atomic_check)
- ret = funcs->atomic_check(connector, new_connector_state);
+ ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
@@ -728,7 +728,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
continue;
if (funcs->atomic_check)
- ret = funcs->atomic_check(connector, new_connector_state);
+ ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 58b8049649a0f..ab40448a19d56 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -106,12 +106,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
}
int intel_digital_connector_atomic_check(struct drm_connector *conn,
- struct drm_connector_state *new_state)
+ struct drm_atomic_state *state)
{
+ struct drm_connector_state *new_state =
+ drm_atomic_get_new_connector_state(state, conn);
struct intel_digital_connector_state *new_conn_state =
to_intel_digital_connector_state(new_state);
struct drm_connector_state *old_state =
- drm_atomic_get_old_connector_state(new_state->state, conn);
+ drm_atomic_get_old_connector_state(state, conn);
struct intel_digital_connector_state *old_conn_state =
to_intel_digital_connector_state(old_state);
struct drm_crtc_state *crtc_state;
@@ -121,7 +123,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
if (!new_state->crtc)
return 0;
- crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);
+ crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
/*
* These properties are handled by fastset, and might not end
diff --git a/drivers/gpu/drm/i915/intel_atomic.h b/drivers/gpu/drm/i915/intel_atomic.h
index 1c8507da1a690..58065d3161a34 100644
--- a/drivers/gpu/drm/i915/intel_atomic.h
+++ b/drivers/gpu/drm/i915/intel_atomic.h
@@ -28,7 +28,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
struct drm_property *property,
u64 val);
int intel_digital_connector_atomic_check(struct drm_connector *conn,
- struct drm_connector_state *new_state);
+ struct drm_atomic_state *state);
struct drm_connector_state *
intel_digital_connector_duplicate_state(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 0caf645fbbb84..60652ebbdf610 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -151,9 +151,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
static int
intel_dp_mst_atomic_check(struct drm_connector *connector,
- struct drm_connector_state *new_conn_state)
+ struct drm_atomic_state *state)
{
- struct drm_atomic_state *state = new_conn_state->state;
+ struct drm_connector_state *new_conn_state =
+ drm_atomic_get_new_connector_state(state, connector);
struct drm_connector_state *old_conn_state =
drm_atomic_get_old_connector_state(state, connector);
struct intel_connector *intel_connector =
@@ -163,7 +164,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
struct drm_dp_mst_topology_mgr *mgr;
int ret;
- ret = intel_digital_connector_atomic_check(connector, new_conn_state);
+ ret = intel_digital_connector_atomic_check(connector, state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 6c98b35790db5..368b99f6f7fbd 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2353,9 +2353,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
};
static int intel_sdvo_atomic_check(struct drm_connector *conn,
- struct drm_connector_state *new_conn_state)
+ struct drm_atomic_state *state)
{
- struct drm_atomic_state *state = new_conn_state->state;
+ struct drm_connector_state *new_conn_state =
+ drm_atomic_get_new_connector_state(state, conn);
struct drm_connector_state *old_conn_state =
drm_atomic_get_old_connector_state(state, conn);
struct intel_sdvo_connector_state *old_state =
@@ -2367,13 +2368,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn,
(memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) ||
memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) {
struct drm_crtc_state *crtc_state =
- drm_atomic_get_new_crtc_state(new_conn_state->state,
+ drm_atomic_get_new_crtc_state(state,
new_conn_state->crtc);
crtc_state->connectors_changed = true;
}
- return intel_digital_connector_atomic_check(conn, new_conn_state);
+ return intel_digital_connector_atomic_check(conn, state);
}
static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 5dc594eafaf2f..0a95df6c6a57f 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1821,16 +1821,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
};
static int intel_tv_atomic_check(struct drm_connector *connector,
- struct drm_connector_state *new_state)
+ struct drm_atomic_state *state)
{
+ struct drm_connector_state *new_state;
struct drm_crtc_state *new_crtc_state;
struct drm_connector_state *old_state;
+ new_state = drm_atomic_get_new_connector_state(state, connector);
if (!new_state->crtc)
return 0;
- old_state = drm_atomic_get_old_connector_state(new_state->state, connector);
- new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);
+ old_state = drm_atomic_get_old_connector_state(state, connector);
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
if (old_state->tv.mode != new_state->tv.mode ||
old_state->tv.margins.left != new_state->tv.margins.left ||
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 4b1650f51955d..7ba373f493b29 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector)
static int
nv50_mstc_atomic_check(struct drm_connector *connector,
- struct drm_connector_state *new_conn_state)
+ struct drm_atomic_state *state)
{
- struct drm_atomic_state *state = new_conn_state->state;
struct nv50_mstc *mstc = nv50_mstc(connector);
struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr;
+ struct drm_connector_state *new_conn_state =
+ drm_atomic_get_new_connector_state(state, connector);
struct drm_connector_state *old_conn_state =
drm_atomic_get_old_connector_state(state, connector);
struct drm_crtc_state *crtc_state;
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 620b51aab291f..5b81ba2a7f272 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
}
static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
- struct drm_connector_state *state)
+ struct drm_atomic_state *state)
{
struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
const struct drm_display_mode *panel_mode;
+ struct drm_connector_state *conn_state;
struct drm_crtc_state *crtc_state;
- if (!state->crtc)
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (!conn_state->crtc)
return 0;
if (list_empty(&connector->modes)) {
@@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
struct drm_display_mode, head);
/* We're not allowed to modify the resolution. */
- crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
- if (IS_ERR(crtc_state))
- return PTR_ERR(crtc_state);
+ crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
+ if (!crtc_state)
+ return -EINVAL;
if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
crtc_state->mode.vdisplay != panel_mode->vdisplay)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index c8b89a78f9f4f..96f91c1b4b6e6 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -221,17 +221,18 @@ static const u32 txp_fmts[] = {
};
static int vc4_txp_connector_atomic_check(struct drm_connector *conn,
- struct drm_connector_state *conn_state)
+ struct drm_atomic_state *state)
{
+ struct drm_connector_state *conn_state;
struct drm_crtc_state *crtc_state;
struct drm_framebuffer *fb;
int i;
+ conn_state = drm_atomic_get_new_connector_state(state, conn);
if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
return 0;
- crtc_state = drm_atomic_get_new_crtc_state(conn_state->state,
- conn_state->crtc);
+ crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
fb = conn_state->writeback_job->fb;
if (fb->width != crtc_state->mode.hdisplay ||
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 0701b3a3108e5..0899f53180e59 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1027,7 +1027,7 @@ struct drm_connector_helper_funcs {
* deadlock.
*/
int (*atomic_check)(struct drm_connector *connector,
- struct drm_connector_state *state);
+ struct drm_atomic_state *state);
/**
* @atomic_commit:
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state
2019-06-11 16:08 ` [PATCH v5 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul
@ 2019-06-11 17:58 ` Laurent Pinchart
0 siblings, 0 replies; 27+ messages in thread
From: Laurent Pinchart @ 2019-06-11 17:58 UTC (permalink / raw)
To: Sean Paul
Cc: Maxime Ripard, nouveau, David Airlie, linux-renesas-soc,
Kieran Bingham, Sean Paul, dri-devel, Rodrigo Vivi, intel-gfx,
Ben Skeggs
Hi Sean,
Thank you for the patch.
On Tue, Jun 11, 2019 at 12:08:18PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> Everyone who implements connector_helper_funcs->atomic_check reaches
> into the connector state to get the atomic state. Instead of continuing
> this pattern, change the callback signature to just give atomic state
> and let the driver determine what it does and does not need from it.
>
> Eventually all atomic functions should do this, but that's just too much
> busy work for me.
>
> Changes in v3:
> - Added to the set
> Changes in v4:
> - None
> Changes in v5:
> - intel_digital_connector_atomic_check declaration moved to i915_atomic.h
>
> Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-5-sean@poorly.run
> Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-5-sean@poorly.run
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: Eric Anholt <eric@anholt.net>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Acked-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 4 ++--
> drivers/gpu/drm/i915/intel_atomic.c | 8 +++++---
> drivers/gpu/drm/i915/intel_atomic.h | 2 +-
> drivers/gpu/drm/i915/intel_dp_mst.c | 7 ++++---
> drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++----
> drivers/gpu/drm/i915/intel_tv.c | 8 +++++---
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++--
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++-----
For the R-Car LVDS driver,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> drivers/gpu/drm/vc4/vc4_txp.c | 7 ++++---
> include/drm/drm_modeset_helper_vtables.h | 2 +-
> 10 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2133f62539176..e58be69960692 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -686,7 +686,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> }
>
> if (funcs->atomic_check)
> - ret = funcs->atomic_check(connector, new_connector_state);
> + ret = funcs->atomic_check(connector, state);
> if (ret)
> return ret;
>
> @@ -728,7 +728,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> continue;
>
> if (funcs->atomic_check)
> - ret = funcs->atomic_check(connector, new_connector_state);
> + ret = funcs->atomic_check(connector, state);
> if (ret)
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 58b8049649a0f..ab40448a19d56 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -106,12 +106,14 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> }
>
> int intel_digital_connector_atomic_check(struct drm_connector *conn,
> - struct drm_connector_state *new_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *new_state =
> + drm_atomic_get_new_connector_state(state, conn);
> struct intel_digital_connector_state *new_conn_state =
> to_intel_digital_connector_state(new_state);
> struct drm_connector_state *old_state =
> - drm_atomic_get_old_connector_state(new_state->state, conn);
> + drm_atomic_get_old_connector_state(state, conn);
> struct intel_digital_connector_state *old_conn_state =
> to_intel_digital_connector_state(old_state);
> struct drm_crtc_state *crtc_state;
> @@ -121,7 +123,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
> if (!new_state->crtc)
> return 0;
>
> - crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);
> + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
>
> /*
> * These properties are handled by fastset, and might not end
> diff --git a/drivers/gpu/drm/i915/intel_atomic.h b/drivers/gpu/drm/i915/intel_atomic.h
> index 1c8507da1a690..58065d3161a34 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/intel_atomic.h
> @@ -28,7 +28,7 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> struct drm_property *property,
> u64 val);
> int intel_digital_connector_atomic_check(struct drm_connector *conn,
> - struct drm_connector_state *new_state);
> + struct drm_atomic_state *state);
> struct drm_connector_state *
> intel_digital_connector_duplicate_state(struct drm_connector *connector);
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 0caf645fbbb84..60652ebbdf610 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -151,9 +151,10 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>
> static int
> intel_dp_mst_atomic_check(struct drm_connector *connector,
> - struct drm_connector_state *new_conn_state)
> + struct drm_atomic_state *state)
> {
> - struct drm_atomic_state *state = new_conn_state->state;
> + struct drm_connector_state *new_conn_state =
> + drm_atomic_get_new_connector_state(state, connector);
> struct drm_connector_state *old_conn_state =
> drm_atomic_get_old_connector_state(state, connector);
> struct intel_connector *intel_connector =
> @@ -163,7 +164,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
> struct drm_dp_mst_topology_mgr *mgr;
> int ret;
>
> - ret = intel_digital_connector_atomic_check(connector, new_conn_state);
> + ret = intel_digital_connector_atomic_check(connector, state);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 6c98b35790db5..368b99f6f7fbd 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2353,9 +2353,10 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
> };
>
> static int intel_sdvo_atomic_check(struct drm_connector *conn,
> - struct drm_connector_state *new_conn_state)
> + struct drm_atomic_state *state)
> {
> - struct drm_atomic_state *state = new_conn_state->state;
> + struct drm_connector_state *new_conn_state =
> + drm_atomic_get_new_connector_state(state, conn);
> struct drm_connector_state *old_conn_state =
> drm_atomic_get_old_connector_state(state, conn);
> struct intel_sdvo_connector_state *old_state =
> @@ -2367,13 +2368,13 @@ static int intel_sdvo_atomic_check(struct drm_connector *conn,
> (memcmp(&old_state->tv, &new_state->tv, sizeof(old_state->tv)) ||
> memcmp(&old_conn_state->tv, &new_conn_state->tv, sizeof(old_conn_state->tv)))) {
> struct drm_crtc_state *crtc_state =
> - drm_atomic_get_new_crtc_state(new_conn_state->state,
> + drm_atomic_get_new_crtc_state(state,
> new_conn_state->crtc);
>
> crtc_state->connectors_changed = true;
> }
>
> - return intel_digital_connector_atomic_check(conn, new_conn_state);
> + return intel_digital_connector_atomic_check(conn, state);
> }
>
> static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = {
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 5dc594eafaf2f..0a95df6c6a57f 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1821,16 +1821,18 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
> };
>
> static int intel_tv_atomic_check(struct drm_connector *connector,
> - struct drm_connector_state *new_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *new_state;
> struct drm_crtc_state *new_crtc_state;
> struct drm_connector_state *old_state;
>
> + new_state = drm_atomic_get_new_connector_state(state, connector);
> if (!new_state->crtc)
> return 0;
>
> - old_state = drm_atomic_get_old_connector_state(new_state->state, connector);
> - new_crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);
> + old_state = drm_atomic_get_old_connector_state(state, connector);
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
>
> if (old_state->tv.mode != new_state->tv.mode ||
> old_state->tv.margins.left != new_state->tv.margins.left ||
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 4b1650f51955d..7ba373f493b29 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -948,11 +948,12 @@ nv50_mstc_get_modes(struct drm_connector *connector)
>
> static int
> nv50_mstc_atomic_check(struct drm_connector *connector,
> - struct drm_connector_state *new_conn_state)
> + struct drm_atomic_state *state)
> {
> - struct drm_atomic_state *state = new_conn_state->state;
> struct nv50_mstc *mstc = nv50_mstc(connector);
> struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr;
> + struct drm_connector_state *new_conn_state =
> + drm_atomic_get_new_connector_state(state, connector);
> struct drm_connector_state *old_conn_state =
> drm_atomic_get_old_connector_state(state, connector);
> struct drm_crtc_state *crtc_state;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 620b51aab291f..5b81ba2a7f272 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -92,13 +92,15 @@ static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
> }
>
> static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
> - struct drm_connector_state *state)
> + struct drm_atomic_state *state)
> {
> struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> const struct drm_display_mode *panel_mode;
> + struct drm_connector_state *conn_state;
> struct drm_crtc_state *crtc_state;
>
> - if (!state->crtc)
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (!conn_state->crtc)
> return 0;
>
> if (list_empty(&connector->modes)) {
> @@ -110,9 +112,9 @@ static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
> struct drm_display_mode, head);
>
> /* We're not allowed to modify the resolution. */
> - crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> - if (IS_ERR(crtc_state))
> - return PTR_ERR(crtc_state);
> + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> + if (!crtc_state)
> + return -EINVAL;
>
> if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
> crtc_state->mode.vdisplay != panel_mode->vdisplay)
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index c8b89a78f9f4f..96f91c1b4b6e6 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -221,17 +221,18 @@ static const u32 txp_fmts[] = {
> };
>
> static int vc4_txp_connector_atomic_check(struct drm_connector *conn,
> - struct drm_connector_state *conn_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_connector_state *conn_state;
> struct drm_crtc_state *crtc_state;
> struct drm_framebuffer *fb;
> int i;
>
> + conn_state = drm_atomic_get_new_connector_state(state, conn);
> if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> return 0;
>
> - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state,
> - conn_state->crtc);
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
>
> fb = conn_state->writeback_job->fb;
> if (fb->width != crtc_state->mode.hdisplay ||
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 0701b3a3108e5..0899f53180e59 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1027,7 +1027,7 @@ struct drm_connector_helper_funcs {
> * deadlock.
> */
> int (*atomic_check)(struct drm_connector *connector,
> - struct drm_connector_state *state);
> + struct drm_atomic_state *state);
>
> /**
> * @atomic_commit:
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (3 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 20:56 ` Sam Ravnborg
2019-06-11 16:08 ` [PATCH v5 06/11] drm/rockchip: Use dirtyfb helper Sean Paul
` (6 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Zain Wang, Jonathan Corbet, David Airlie, linux-doc, Jose Souza,
Tomasz Figa, Maxime Ripard, Sean Paul, Sean Paul
From: Sean Paul <seanpaul@chromium.org>
This patch adds a new drm helper library to help drivers implement
self refresh. Drivers choosing to use it will register crtcs and
will receive callbacks when it's time to enter or exit self refresh
mode.
In its current form, it has a timer which will trigger after a
driver-specified amount of inactivity. When the timer triggers, the
helpers will submit a new atomic commit to shut the refreshing pipe
off. On the next atomic commit, the drm core will revert the self
refresh state and bring everything back up to be actively driven.
From the driver's perspective, this works like a regular disable/enable
cycle. The driver need only check the 'self_refresh_active' state in
crtc_state. It should initiate self refresh mode on the panel and enter
an off or low-power state.
Changes in v2:
- s/psr/self_refresh/ (Daniel)
- integrated the psr exit into the commit that wakes it up (Jose/Daniel)
- made the psr state per-crtc (Jose/Daniel)
Changes in v3:
- Remove the self_refresh_(active|changed) from connector state (Daniel)
- Simplify loop in drm_self_refresh_helper_alter_state (Daniel)
- Improve self_refresh_aware comment (Daniel)
- s/self_refresh_state/self_refresh_data/ (Daniel)
Changes in v4:
- Move docbook location below panel (Daniel)
- Improve docbook with references and more detailed explanation (Daniel)
- Instead of register/unregister, use init/cleanup (Daniel)
Changes in v5:
- Resolved conflict in drm_atomic_helper.c #include block
- Resolved conflict in rst with HDCP helper docs
Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-2-sean@poorly.run
Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-1-sean@poorly.run
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-6-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-6-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jose Souza <jose.souza@intel.com>
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
Documentation/gpu/drm-kms-helpers.rst | 9 +
drivers/gpu/drm/Makefile | 2 +-
drivers/gpu/drm/drm_atomic.c | 2 +
drivers/gpu/drm/drm_atomic_helper.c | 35 +++-
drivers/gpu/drm/drm_atomic_state_helper.c | 4 +
drivers/gpu/drm/drm_atomic_uapi.c | 7 +-
drivers/gpu/drm/drm_self_refresh_helper.c | 213 ++++++++++++++++++++++
include/drm/drm_atomic.h | 15 ++
include/drm/drm_connector.h | 14 ++
include/drm/drm_crtc.h | 19 ++
include/drm/drm_self_refresh_helper.h | 22 +++
11 files changed, 337 insertions(+), 5 deletions(-)
create mode 100644 drivers/gpu/drm/drm_self_refresh_helper.c
create mode 100644 include/drm/drm_self_refresh_helper.h
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 0fe726a6ee678..b327bbc111821 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -181,6 +181,15 @@ Panel Helper Reference
.. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
:export:
+Panel Self Refresh Helper Reference
+===================================
+
+.. kernel-doc:: drivers/gpu/drm/drm_self_refresh_helper.c
+ :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_self_refresh_helper.c
+ :export:
+
HDCP Helper Functions Reference
===============================
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index d36feb4a62330..9d630a28a7880 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -43,7 +43,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper
drm_simple_kms_helper.o drm_modeset_helper.o \
drm_scdc_helper.o drm_gem_framebuffer_helper.o \
drm_atomic_state_helper.o drm_damage_helper.o \
- drm_format_helper.o
+ drm_format_helper.o drm_self_refresh_helper.o
drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a4e779deab0fb..419381abbdd16 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -384,6 +384,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
drm_printf(p, "crtc[%u]: %s\n", crtc->base.id, crtc->name);
drm_printf(p, "\tenable=%d\n", state->enable);
drm_printf(p, "\tactive=%d\n", state->active);
+ drm_printf(p, "\tself_refresh_active=%d\n", state->self_refresh_active);
drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
@@ -999,6 +1000,7 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name);
drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
+ drm_printf(p, "\tself_refresh_aware=%d\n", state->self_refresh_aware);
if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
if (state->writeback_job && state->writeback_job->fb)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index e58be69960692..bc43cc08cdc11 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -33,6 +33,7 @@
#include <drm/drm_damage_helper.h>
#include <drm/drm_device.h>
#include <drm/drm_plane_helper.h>
+#include <drm/drm_self_refresh_helper.h>
#include <drm/drm_print.h>
#include <drm/drm_vblank.h>
#include <drm/drm_writeback.h>
@@ -953,10 +954,33 @@ int drm_atomic_helper_check(struct drm_device *dev,
if (state->legacy_cursor_update)
state->async_update = !drm_atomic_helper_async_check(dev, state);
+ drm_self_refresh_helper_alter_state(state);
+
return ret;
}
EXPORT_SYMBOL(drm_atomic_helper_check);
+static bool
+crtc_needs_disable(struct drm_crtc_state *old_state,
+ struct drm_crtc_state *new_state)
+{
+ /*
+ * No new_state means the crtc is off, so the only criteria is whether
+ * it's currently active or in self refresh mode.
+ */
+ if (!new_state)
+ return drm_atomic_crtc_effectively_active(old_state);
+
+ /*
+ * We need to run through the crtc_funcs->disable() function if the crtc
+ * is currently on, if it's transitioning to self refresh mode, or if
+ * it's in self refresh mode and needs to be fully disabled.
+ */
+ return old_state->active ||
+ (old_state->self_refresh_active && !new_state->enable) ||
+ new_state->self_refresh_active;
+}
+
static void
disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
{
@@ -977,7 +1001,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
- if (!old_crtc_state->active ||
+ if (new_conn_state->crtc)
+ new_crtc_state = drm_atomic_get_new_crtc_state(
+ old_state,
+ new_conn_state->crtc);
+ else
+ new_crtc_state = NULL;
+
+ if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
!drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
continue;
@@ -1023,7 +1054,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
continue;
- if (!old_crtc_state->active)
+ if (!crtc_needs_disable(old_crtc_state, new_crtc_state))
continue;
funcs = crtc->helper_private;
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 97ab26679b969..7d7347a6f1943 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -129,6 +129,10 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
state->commit = NULL;
state->event = NULL;
state->pageflip_flags = 0;
+
+ /* Self refresh should be canceled when a new update is available */
+ state->active = drm_atomic_crtc_effectively_active(state);
+ state->self_refresh_active = false;
}
EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index eb22e8bdd8531..abe38bdf85ae6 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -490,7 +490,7 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
struct drm_mode_config *config = &dev->mode_config;
if (property == config->prop_active)
- *val = state->active;
+ *val = drm_atomic_crtc_effectively_active(state);
else if (property == config->prop_mode_id)
*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
else if (property == config->prop_vrr_enabled)
@@ -788,7 +788,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
if (property == config->prop_crtc_id) {
*val = (state->crtc) ? state->crtc->base.id : 0;
} else if (property == config->dpms_property) {
- *val = connector->dpms;
+ if (state->crtc && state->crtc->state->self_refresh_active)
+ *val = DRM_MODE_DPMS_ON;
+ else
+ *val = connector->dpms;
} else if (property == config->tv_select_subconnector_property) {
*val = state->tv.subconnector;
} else if (property == config->tv_left_margin_property) {
diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c
new file mode 100644
index 0000000000000..c7083c5694fee
--- /dev/null
+++ b/drivers/gpu/drm/drm_self_refresh_helper.c
@@ -0,0 +1,213 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Authors:
+ * Sean Paul <seanpaul@chromium.org>
+ */
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_mode_config.h>
+#include <drm/drm_modeset_lock.h>
+#include <drm/drm_print.h>
+#include <drm/drm_self_refresh_helper.h>
+#include <linux/bitops.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+/**
+ * DOC: overview
+ *
+ * This helper library provides an easy way for drivers to leverage the atomic
+ * framework to implement panel self refresh (SR) support. Drivers are
+ * responsible for initializing and cleaning up the SR helpers on load/unload
+ * (see &drm_self_refresh_helper_init/&drm_self_refresh_helper_cleanup).
+ * The connector is responsible for setting
+ * &drm_connector_state.self_refresh_aware to true at runtime if it is SR-aware
+ * (meaning it knows how to initiate self refresh on the panel).
+ *
+ * Once a crtc has enabled SR using &drm_self_refresh_helper_init, the
+ * helpers will monitor activity and call back into the driver to enable/disable
+ * SR as appropriate. The best way to think about this is that it's a DPMS
+ * on/off request with &drm_crtc_state.self_refresh_active set in crtc state
+ * that tells you to disable/enable SR on the panel instead of power-cycling it.
+ *
+ * During SR, drivers may choose to fully disable their crtc/encoder/bridge
+ * hardware (in which case no driver changes are necessary), or they can inspect
+ * &drm_crtc_state.self_refresh_active if they want to enter low power mode
+ * without full disable (in case full disable/enable is too slow).
+ *
+ * SR will be deactivated if there are any atomic updates affecting the
+ * pipe that is in SR mode. If a crtc is driving multiple connectors, all
+ * connectors must be SR aware and all will enter/exit SR mode at the same time.
+ *
+ * If the crtc and connector are SR aware, but the panel connected does not
+ * support it (or is otherwise unable to enter SR), the driver should fail
+ * atomic_check when &drm_crtc_state.self_refresh_active is true.
+ */
+
+struct drm_self_refresh_data {
+ struct drm_crtc *crtc;
+ struct delayed_work entry_work;
+ struct drm_atomic_state *save_state;
+ unsigned int entry_delay_ms;
+};
+
+static void drm_self_refresh_helper_entry_work(struct work_struct *work)
+{
+ struct drm_self_refresh_data *sr_data = container_of(
+ to_delayed_work(work),
+ struct drm_self_refresh_data, entry_work);
+ struct drm_crtc *crtc = sr_data->crtc;
+ struct drm_device *dev = crtc->dev;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_state *state;
+ struct drm_connector *conn;
+ struct drm_connector_state *conn_state;
+ struct drm_crtc_state *crtc_state;
+ int i, ret;
+
+ drm_modeset_acquire_init(&ctx, 0);
+
+ state = drm_atomic_state_alloc(dev);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+retry:
+ state->acquire_ctx = &ctx;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state)) {
+ ret = PTR_ERR(crtc_state);
+ goto out;
+ }
+
+ if (!crtc_state->enable)
+ goto out;
+
+ ret = drm_atomic_add_affected_connectors(state, crtc);
+ if (ret)
+ goto out;
+
+ for_each_new_connector_in_state(state, conn, conn_state, i) {
+ if (!conn_state->self_refresh_aware)
+ goto out;
+ }
+
+ crtc_state->active = false;
+ crtc_state->self_refresh_active = true;
+
+ ret = drm_atomic_commit(state);
+ if (ret)
+ goto out;
+
+out:
+ if (ret == -EDEADLK) {
+ drm_atomic_state_clear(state);
+ ret = drm_modeset_backoff(&ctx);
+ if (!ret)
+ goto retry;
+ }
+
+ drm_atomic_state_put(state);
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+}
+
+/**
+ * drm_self_refresh_helper_alter_state - Alters the atomic state for SR exit
+ * @state: the state currently being checked
+ *
+ * Called at the end of atomic check. This function checks the state for flags
+ * incompatible with self refresh exit and changes them. This is a bit
+ * disingenuous since userspace is expecting one thing and we're giving it
+ * another. However in order to keep self refresh entirely hidden from
+ * userspace, this is required.
+ *
+ * At the end, we queue up the self refresh entry work so we can enter PSR after
+ * the desired delay.
+ */
+void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state)
+{
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+ int i;
+
+ if (state->async_update || !state->allow_modeset) {
+ for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
+ if (crtc_state->self_refresh_active) {
+ state->async_update = false;
+ state->allow_modeset = true;
+ break;
+ }
+ }
+ }
+
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+ struct drm_self_refresh_data *sr_data;
+
+ /* Don't trigger the entry timer when we're already in SR */
+ if (crtc_state->self_refresh_active)
+ continue;
+
+ sr_data = crtc->self_refresh_data;
+ if (!sr_data)
+ continue;
+
+ mod_delayed_work(system_wq, &sr_data->entry_work,
+ msecs_to_jiffies(sr_data->entry_delay_ms));
+ }
+}
+EXPORT_SYMBOL(drm_self_refresh_helper_alter_state);
+
+/**
+ * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
+ * @crtc: the crtc which supports self refresh supported displays
+ * @entry_delay_ms: amount of inactivity to wait before entering self refresh
+ */
+int drm_self_refresh_helper_init(struct drm_crtc *crtc,
+ unsigned int entry_delay_ms)
+{
+ struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;
+
+ /* Helper is already initialized */
+ if (WARN_ON(sr_data))
+ return -EINVAL;
+
+ sr_data = kzalloc(sizeof(*sr_data), GFP_KERNEL);
+ if (!sr_data)
+ return -ENOMEM;
+
+ INIT_DELAYED_WORK(&sr_data->entry_work,
+ drm_self_refresh_helper_entry_work);
+ sr_data->entry_delay_ms = entry_delay_ms;
+ sr_data->crtc = crtc;
+
+ crtc->self_refresh_data = sr_data;
+ return 0;
+}
+EXPORT_SYMBOL(drm_self_refresh_helper_init);
+
+/**
+ * drm_self_refresh_helper_cleanup - Cleans up self refresh helpers for a crtc
+ * @crtc: the crtc to cleanup
+ */
+void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc)
+{
+ struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;
+
+ /* Helper is already uninitialized */
+ if (sr_data)
+ return;
+
+ crtc->self_refresh_data = NULL;
+
+ cancel_delayed_work_sync(&sr_data->entry_work);
+ kfree(sr_data);
+}
+EXPORT_SYMBOL(drm_self_refresh_helper_cleanup);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f122156478010..927e1205d7aa4 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -957,4 +957,19 @@ drm_atomic_crtc_needs_modeset(const struct drm_crtc_state *state)
state->connectors_changed;
}
+/**
+ * drm_atomic_crtc_effectively_active - compute whether crtc is actually active
+ * @state: &drm_crtc_state for the CRTC
+ *
+ * When in self refresh mode, the crtc_state->active value will be false, since
+ * the crtc is off. However in some cases we're interested in whether the crtc
+ * is active, or effectively active (ie: it's connected to an active display).
+ * In these cases, use this function instead of just checking active.
+ */
+static inline bool
+drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state)
+{
+ return state->active || state->self_refresh_active;
+}
+
#endif /* DRM_ATOMIC_H_ */
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 071143bc0ebd1..c6f8486d8b8f3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -548,6 +548,20 @@ struct drm_connector_state {
/** @tv: TV connector state */
struct drm_tv_connector_state tv;
+ /**
+ * @self_refresh_aware:
+ *
+ * This tracks whether a connector is aware of the self refresh state.
+ * It should be set to true for those connector implementations which
+ * understand the self refresh state. This is needed since the crtc
+ * registers the self refresh helpers and it doesn't know if the
+ * connectors downstream have implemented self refresh entry/exit.
+ *
+ * Drivers should set this to true in atomic_check if they know how to
+ * handle self_refresh requests.
+ */
+ bool self_refresh_aware;
+
/**
* @picture_aspect_ratio: Connector property to control the
* HDMI infoframe aspect ratio setting.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index dc42b9e353333..4fda610d76f26 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -54,6 +54,7 @@ struct drm_mode_set;
struct drm_file;
struct drm_clip_rect;
struct drm_printer;
+struct drm_self_refresh_data;
struct device_node;
struct dma_fence;
struct edid;
@@ -300,6 +301,17 @@ struct drm_crtc_state {
*/
bool vrr_enabled;
+ /**
+ * @self_refresh_active:
+ *
+ * Used by the self refresh helpers to denote when a self refresh
+ * transition is occuring. This will be set on enable/disable callbacks
+ * when self refresh is being enabled or disabled. In some cases, it may
+ * not be desirable to fully shut off the crtc during self refresh.
+ * CRTC's can inspect this flag and determine the best course of action.
+ */
+ bool self_refresh_active;
+
/**
* @event:
*
@@ -1088,6 +1100,13 @@ struct drm_crtc {
* The name of the CRTC's fence timeline.
*/
char timeline_name[32];
+
+ /**
+ * @self_refresh_data: Holds the state for the self refresh helpers
+ *
+ * Initialized via drm_self_refresh_helper_register().
+ */
+ struct drm_self_refresh_data *self_refresh_data;
};
/**
diff --git a/include/drm/drm_self_refresh_helper.h b/include/drm/drm_self_refresh_helper.h
new file mode 100644
index 0000000000000..405e86fb8461e
--- /dev/null
+++ b/include/drm/drm_self_refresh_helper.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Authors:
+ * Sean Paul <seanpaul@chromium.org>
+ */
+#ifndef DRM_SELF_REFRESH_HELPER_H_
+#define DRM_SELF_REFRESH_HELPER_H_
+
+struct drm_atomic_state;
+struct drm_connector;
+struct drm_device;
+struct drm_modeset_acquire_ctx;
+
+void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state);
+
+int drm_self_refresh_helper_init(struct drm_crtc *crtc,
+ unsigned int entry_delay_ms);
+
+void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc);
+#endif
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers
2019-06-11 16:08 ` [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers Sean Paul
@ 2019-06-11 20:56 ` Sam Ravnborg
2019-06-12 14:19 ` Sean Paul
2019-06-12 14:50 ` [PATCH v6] " Sean Paul
0 siblings, 2 replies; 27+ messages in thread
From: Sam Ravnborg @ 2019-06-11 20:56 UTC (permalink / raw)
To: Sean Paul
Cc: Zain Wang, linux-doc, David Airlie, Jonathan Corbet, Jose Souza,
Tomasz Figa, Maxime Ripard, Sean Paul, dri-devel
Hi Sean.
Small things here and there. Did not stare at this long enough to
understand the code, but added some feedback anyway.
Sam
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index d36feb4a62330..9d630a28a7880 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -43,7 +43,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper
> drm_simple_kms_helper.o drm_modeset_helper.o \
> drm_scdc_helper.o drm_gem_framebuffer_helper.o \
> drm_atomic_state_helper.o drm_damage_helper.o \
> - drm_format_helper.o
> + drm_format_helper.o drm_self_refresh_helper.o
We really should have only one .o file per line and sort them.
The current way to do things is asking for conflicts.
Anyway, that battle is for another day.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e58be69960692..bc43cc08cdc11 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -33,6 +33,7 @@
> #include <drm/drm_damage_helper.h>
> #include <drm/drm_device.h>
> #include <drm/drm_plane_helper.h>
> +#include <drm/drm_self_refresh_helper.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
> #include <drm/drm_writeback.h>
Please add header file in alphabetic order.
It goes below drm_print.h
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_self_refresh_helper.c
> @@ -0,0 +1,213 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@chromium.org>
> + */
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_mode_config.h>
> +#include <drm/drm_modeset_lock.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_self_refresh_helper.h>
> +#include <linux/bitops.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
Nit:
Typical include order is:
#include <linux/*>
#include <drm/*>
#include "*"
With an empty lines between the blocks.
And sorted within the block.
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides an easy way for drivers to leverage the atomic
> + * framework to implement panel self refresh (SR) support. Drivers are
> + * responsible for initializing and cleaning up the SR helpers on load/unload
> + * (see &drm_self_refresh_helper_init/&drm_self_refresh_helper_cleanup).
> + * The connector is responsible for setting
> + * &drm_connector_state.self_refresh_aware to true at runtime if it is SR-aware
> + * (meaning it knows how to initiate self refresh on the panel).
> + *
> + * Once a crtc has enabled SR using &drm_self_refresh_helper_init, the
> + * helpers will monitor activity and call back into the driver to enable/disable
> + * SR as appropriate. The best way to think about this is that it's a DPMS
> + * on/off request with &drm_crtc_state.self_refresh_active set in crtc state
> + * that tells you to disable/enable SR on the panel instead of power-cycling it.
> + *
> + * During SR, drivers may choose to fully disable their crtc/encoder/bridge
> + * hardware (in which case no driver changes are necessary), or they can inspect
> + * &drm_crtc_state.self_refresh_active if they want to enter low power mode
> + * without full disable (in case full disable/enable is too slow).
> + *
> + * SR will be deactivated if there are any atomic updates affecting the
> + * pipe that is in SR mode. If a crtc is driving multiple connectors, all
> + * connectors must be SR aware and all will enter/exit SR mode at the same time.
> + *
> + * If the crtc and connector are SR aware, but the panel connected does not
> + * support it (or is otherwise unable to enter SR), the driver should fail
> + * atomic_check when &drm_crtc_state.self_refresh_active is true.
> + */
> +
> +struct drm_self_refresh_data {
> + struct drm_crtc *crtc;
> + struct delayed_work entry_work;
> + struct drm_atomic_state *save_state;
> + unsigned int entry_delay_ms;
> +};
> +
> +static void drm_self_refresh_helper_entry_work(struct work_struct *work)
> +{
> + struct drm_self_refresh_data *sr_data = container_of(
> + to_delayed_work(work),
> + struct drm_self_refresh_data, entry_work);
> + struct drm_crtc *crtc = sr_data->crtc;
> + struct drm_device *dev = crtc->dev;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_connector *conn;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc_state *crtc_state;
> + int i, ret;
This function is called from a workqueue.
Just wondering if this require any locking?
(Maybe I missed it, browsed the code without a detailed review)
> +
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> +retry:
> + state->acquire_ctx = &ctx;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state)) {
> + ret = PTR_ERR(crtc_state);
> + goto out;
> + }
> +
> + if (!crtc_state->enable)
> + goto out;
> +
> + ret = drm_atomic_add_affected_connectors(state, crtc);
> + if (ret)
> + goto out;
> +
> + for_each_new_connector_in_state(state, conn, conn_state, i) {
> + if (!conn_state->self_refresh_aware)
> + goto out;
> + }
> +
> + crtc_state->active = false;
> + crtc_state->self_refresh_active = true;
> +
> + ret = drm_atomic_commit(state);
> + if (ret)
> + goto out;
> +
> +out:
> + if (ret == -EDEADLK) {
> + drm_atomic_state_clear(state);
> + ret = drm_modeset_backoff(&ctx);
> + if (!ret)
> + goto retry;
> + }
> +
> + drm_atomic_state_put(state);
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +}
> +
> +
> +/**
> + * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
> + * @crtc: the crtc which supports self refresh supported displays
> + * @entry_delay_ms: amount of inactivity to wait before entering self refresh
> + */
> +int drm_self_refresh_helper_init(struct drm_crtc *crtc,
> + unsigned int entry_delay_ms)
Missing documentation of return code.
> +{
> + struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;
> +
> + /* Helper is already initialized */
> + if (WARN_ON(sr_data))
> + return -EINVAL;
> +
> + sr_data = kzalloc(sizeof(*sr_data), GFP_KERNEL);
> + if (!sr_data)
> + return -ENOMEM;
> +
> + INIT_DELAYED_WORK(&sr_data->entry_work,
> + drm_self_refresh_helper_entry_work);
> + sr_data->entry_delay_ms = entry_delay_ms;
> + sr_data->crtc = crtc;
> +
> + crtc->self_refresh_data = sr_data;
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_self_refresh_helper_init);
> +
>
> /**
> diff --git a/include/drm/drm_self_refresh_helper.h b/include/drm/drm_self_refresh_helper.h
> new file mode 100644
> index 0000000000000..405e86fb8461e
> --- /dev/null
> +++ b/include/drm/drm_self_refresh_helper.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@chromium.org>
> + */
> +#ifndef DRM_SELF_REFRESH_HELPER_H_
> +#define DRM_SELF_REFRESH_HELPER_H_
> +
> +struct drm_atomic_state;
OK
> +struct drm_connector;
> +struct drm_device;
> +struct drm_modeset_acquire_ctx;
The three above are not needed.
struct drm_crtc;
Missing
> +
> +void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state);
> +
> +int drm_self_refresh_helper_init(struct drm_crtc *crtc,
> + unsigned int entry_delay_ms);
> +
> +void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc);
> +#endif
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers
2019-06-11 20:56 ` Sam Ravnborg
@ 2019-06-12 14:19 ` Sean Paul
2019-06-12 14:50 ` [PATCH v6] " Sean Paul
1 sibling, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-12 14:19 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Zain Wang, linux-doc, David Airlie, Jonathan Corbet, dri-devel,
Tomasz Figa, Maxime Ripard, Sean Paul, Jose Souza, Sean Paul
On Tue, Jun 11, 2019 at 10:56:45PM +0200, Sam Ravnborg wrote:
> Hi Sean.
>
> Small things here and there. Did not stare at this long enough to
> understand the code, but added some feedback anyway.
Thanks for the comments, Sam, I'll send a revision shortly
>
> Sam
> >
/snip
> > +static void drm_self_refresh_helper_entry_work(struct work_struct *work)
> > +{
> > + struct drm_self_refresh_data *sr_data = container_of(
> > + to_delayed_work(work),
> > + struct drm_self_refresh_data, entry_work);
> > + struct drm_crtc *crtc = sr_data->crtc;
> > + struct drm_device *dev = crtc->dev;
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_atomic_state *state;
> > + struct drm_connector *conn;
> > + struct drm_connector_state *conn_state;
> > + struct drm_crtc_state *crtc_state;
> > + int i, ret;
> This function is called from a workqueue.
> Just wondering if this require any locking?
Yes, it does. The locks are acquired in the various drm_atomic_get_*_state()
function calls and dropped below in the out label (drm_modeset_drop_locks).
> (Maybe I missed it, browsed the code without a detailed review)
>
> > +
> > + drm_modeset_acquire_init(&ctx, 0);
> > +
> > + state = drm_atomic_state_alloc(dev);
> > + if (!state) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > +retry:
> > + state->acquire_ctx = &ctx;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > + if (IS_ERR(crtc_state)) {
> > + ret = PTR_ERR(crtc_state);
> > + goto out;
> > + }
> > +
> > + if (!crtc_state->enable)
> > + goto out;
> > +
> > + ret = drm_atomic_add_affected_connectors(state, crtc);
> > + if (ret)
> > + goto out;
> > +
> > + for_each_new_connector_in_state(state, conn, conn_state, i) {
> > + if (!conn_state->self_refresh_aware)
> > + goto out;
> > + }
> > +
> > + crtc_state->active = false;
> > + crtc_state->self_refresh_active = true;
> > +
> > + ret = drm_atomic_commit(state);
> > + if (ret)
> > + goto out;
> > +
> > +out:
> > + if (ret == -EDEADLK) {
> > + drm_atomic_state_clear(state);
> > + ret = drm_modeset_backoff(&ctx);
> > + if (!ret)
> > + goto retry;
> > + }
> > +
> > + drm_atomic_state_put(state);
> > + drm_modeset_drop_locks(&ctx);
> > + drm_modeset_acquire_fini(&ctx);
> > +}
> > +
/snip
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v6] drm: Add helpers to kick off self refresh mode in drivers
2019-06-11 20:56 ` Sam Ravnborg
2019-06-12 14:19 ` Sean Paul
@ 2019-06-12 14:50 ` Sean Paul
1 sibling, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-12 14:50 UTC (permalink / raw)
To: dri-devel
Cc: Zain Wang, Jonathan Corbet, Maxime Ripard, Sean Paul, linux-doc,
Jose Souza, Tomasz Figa, David Airlie, Sean Paul, Sam Ravnborg
From: Sean Paul <seanpaul@chromium.org>
This patch adds a new drm helper library to help drivers implement
self refresh. Drivers choosing to use it will register crtcs and
will receive callbacks when it's time to enter or exit self refresh
mode.
In its current form, it has a timer which will trigger after a
driver-specified amount of inactivity. When the timer triggers, the
helpers will submit a new atomic commit to shut the refreshing pipe
off. On the next atomic commit, the drm core will revert the self
refresh state and bring everything back up to be actively driven.
From the driver's perspective, this works like a regular disable/enable
cycle. The driver need only check the 'self_refresh_active' state in
crtc_state. It should initiate self refresh mode on the panel and enter
an off or low-power state.
Changes in v2:
- s/psr/self_refresh/ (Daniel)
- integrated the psr exit into the commit that wakes it up (Jose/Daniel)
- made the psr state per-crtc (Jose/Daniel)
Changes in v3:
- Remove the self_refresh_(active|changed) from connector state (Daniel)
- Simplify loop in drm_self_refresh_helper_alter_state (Daniel)
- Improve self_refresh_aware comment (Daniel)
- s/self_refresh_state/self_refresh_data/ (Daniel)
Changes in v4:
- Move docbook location below panel (Daniel)
- Improve docbook with references and more detailed explanation (Daniel)
- Instead of register/unregister, use init/cleanup (Daniel)
Changes in v5:
- Resolved conflict in drm_atomic_helper.c #include block
- Resolved conflict in rst with HDCP helper docs
Changes in v6:
- Fix include ordering, clean up forward declarations (Sam)
Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-2-sean@poorly.run
Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-1-sean@poorly.run
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-6-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-6-sean@poorly.run
Link to v5: https://patchwork.freedesktop.org/patch/msgid/20190611160844.257498-6-sean@poorly.run
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jose Souza <jose.souza@intel.com>
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
Documentation/gpu/drm-kms-helpers.rst | 9 +
drivers/gpu/drm/Makefile | 2 +-
drivers/gpu/drm/drm_atomic.c | 2 +
drivers/gpu/drm/drm_atomic_helper.c | 35 +++-
drivers/gpu/drm/drm_atomic_state_helper.c | 4 +
drivers/gpu/drm/drm_atomic_uapi.c | 7 +-
drivers/gpu/drm/drm_self_refresh_helper.c | 216 ++++++++++++++++++++++
include/drm/drm_atomic.h | 15 ++
include/drm/drm_connector.h | 14 ++
include/drm/drm_crtc.h | 19 ++
include/drm/drm_self_refresh_helper.h | 20 ++
11 files changed, 338 insertions(+), 5 deletions(-)
create mode 100644 drivers/gpu/drm/drm_self_refresh_helper.c
create mode 100644 include/drm/drm_self_refresh_helper.h
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 0fe726a6ee678..b327bbc111821 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -181,6 +181,15 @@ Panel Helper Reference
.. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
:export:
+Panel Self Refresh Helper Reference
+===================================
+
+.. kernel-doc:: drivers/gpu/drm/drm_self_refresh_helper.c
+ :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_self_refresh_helper.c
+ :export:
+
HDCP Helper Functions Reference
===============================
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index d36feb4a62330..9d630a28a7880 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -43,7 +43,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper
drm_simple_kms_helper.o drm_modeset_helper.o \
drm_scdc_helper.o drm_gem_framebuffer_helper.o \
drm_atomic_state_helper.o drm_damage_helper.o \
- drm_format_helper.o
+ drm_format_helper.o drm_self_refresh_helper.o
drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a4e779deab0fb..419381abbdd16 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -384,6 +384,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
drm_printf(p, "crtc[%u]: %s\n", crtc->base.id, crtc->name);
drm_printf(p, "\tenable=%d\n", state->enable);
drm_printf(p, "\tactive=%d\n", state->active);
+ drm_printf(p, "\tself_refresh_active=%d\n", state->self_refresh_active);
drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
@@ -999,6 +1000,7 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name);
drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
+ drm_printf(p, "\tself_refresh_aware=%d\n", state->self_refresh_aware);
if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
if (state->writeback_job && state->writeback_job->fb)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index e58be69960692..d9ea7e51ffdc0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -34,6 +34,7 @@
#include <drm/drm_device.h>
#include <drm/drm_plane_helper.h>
#include <drm/drm_print.h>
+#include <drm/drm_self_refresh_helper.h>
#include <drm/drm_vblank.h>
#include <drm/drm_writeback.h>
@@ -953,10 +954,33 @@ int drm_atomic_helper_check(struct drm_device *dev,
if (state->legacy_cursor_update)
state->async_update = !drm_atomic_helper_async_check(dev, state);
+ drm_self_refresh_helper_alter_state(state);
+
return ret;
}
EXPORT_SYMBOL(drm_atomic_helper_check);
+static bool
+crtc_needs_disable(struct drm_crtc_state *old_state,
+ struct drm_crtc_state *new_state)
+{
+ /*
+ * No new_state means the crtc is off, so the only criteria is whether
+ * it's currently active or in self refresh mode.
+ */
+ if (!new_state)
+ return drm_atomic_crtc_effectively_active(old_state);
+
+ /*
+ * We need to run through the crtc_funcs->disable() function if the crtc
+ * is currently on, if it's transitioning to self refresh mode, or if
+ * it's in self refresh mode and needs to be fully disabled.
+ */
+ return old_state->active ||
+ (old_state->self_refresh_active && !new_state->enable) ||
+ new_state->self_refresh_active;
+}
+
static void
disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
{
@@ -977,7 +1001,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
- if (!old_crtc_state->active ||
+ if (new_conn_state->crtc)
+ new_crtc_state = drm_atomic_get_new_crtc_state(
+ old_state,
+ new_conn_state->crtc);
+ else
+ new_crtc_state = NULL;
+
+ if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
!drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
continue;
@@ -1023,7 +1054,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
continue;
- if (!old_crtc_state->active)
+ if (!crtc_needs_disable(old_crtc_state, new_crtc_state))
continue;
funcs = crtc->helper_private;
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 97ab26679b969..7d7347a6f1943 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -129,6 +129,10 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
state->commit = NULL;
state->event = NULL;
state->pageflip_flags = 0;
+
+ /* Self refresh should be canceled when a new update is available */
+ state->active = drm_atomic_crtc_effectively_active(state);
+ state->self_refresh_active = false;
}
EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index eb22e8bdd8531..abe38bdf85ae6 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -490,7 +490,7 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
struct drm_mode_config *config = &dev->mode_config;
if (property == config->prop_active)
- *val = state->active;
+ *val = drm_atomic_crtc_effectively_active(state);
else if (property == config->prop_mode_id)
*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
else if (property == config->prop_vrr_enabled)
@@ -788,7 +788,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
if (property == config->prop_crtc_id) {
*val = (state->crtc) ? state->crtc->base.id : 0;
} else if (property == config->dpms_property) {
- *val = connector->dpms;
+ if (state->crtc && state->crtc->state->self_refresh_active)
+ *val = DRM_MODE_DPMS_ON;
+ else
+ *val = connector->dpms;
} else if (property == config->tv_select_subconnector_property) {
*val = state->tv.subconnector;
} else if (property == config->tv_left_margin_property) {
diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c
new file mode 100644
index 0000000000000..ebd907c6dbb63
--- /dev/null
+++ b/drivers/gpu/drm/drm_self_refresh_helper.c
@@ -0,0 +1,216 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Authors:
+ * Sean Paul <seanpaul@chromium.org>
+ */
+#include <linux/bitops.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_mode_config.h>
+#include <drm/drm_modeset_lock.h>
+#include <drm/drm_print.h>
+#include <drm/drm_self_refresh_helper.h>
+
+/**
+ * DOC: overview
+ *
+ * This helper library provides an easy way for drivers to leverage the atomic
+ * framework to implement panel self refresh (SR) support. Drivers are
+ * responsible for initializing and cleaning up the SR helpers on load/unload
+ * (see &drm_self_refresh_helper_init/&drm_self_refresh_helper_cleanup).
+ * The connector is responsible for setting
+ * &drm_connector_state.self_refresh_aware to true at runtime if it is SR-aware
+ * (meaning it knows how to initiate self refresh on the panel).
+ *
+ * Once a crtc has enabled SR using &drm_self_refresh_helper_init, the
+ * helpers will monitor activity and call back into the driver to enable/disable
+ * SR as appropriate. The best way to think about this is that it's a DPMS
+ * on/off request with &drm_crtc_state.self_refresh_active set in crtc state
+ * that tells you to disable/enable SR on the panel instead of power-cycling it.
+ *
+ * During SR, drivers may choose to fully disable their crtc/encoder/bridge
+ * hardware (in which case no driver changes are necessary), or they can inspect
+ * &drm_crtc_state.self_refresh_active if they want to enter low power mode
+ * without full disable (in case full disable/enable is too slow).
+ *
+ * SR will be deactivated if there are any atomic updates affecting the
+ * pipe that is in SR mode. If a crtc is driving multiple connectors, all
+ * connectors must be SR aware and all will enter/exit SR mode at the same time.
+ *
+ * If the crtc and connector are SR aware, but the panel connected does not
+ * support it (or is otherwise unable to enter SR), the driver should fail
+ * atomic_check when &drm_crtc_state.self_refresh_active is true.
+ */
+
+struct drm_self_refresh_data {
+ struct drm_crtc *crtc;
+ struct delayed_work entry_work;
+ struct drm_atomic_state *save_state;
+ unsigned int entry_delay_ms;
+};
+
+static void drm_self_refresh_helper_entry_work(struct work_struct *work)
+{
+ struct drm_self_refresh_data *sr_data = container_of(
+ to_delayed_work(work),
+ struct drm_self_refresh_data, entry_work);
+ struct drm_crtc *crtc = sr_data->crtc;
+ struct drm_device *dev = crtc->dev;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_state *state;
+ struct drm_connector *conn;
+ struct drm_connector_state *conn_state;
+ struct drm_crtc_state *crtc_state;
+ int i, ret;
+
+ drm_modeset_acquire_init(&ctx, 0);
+
+ state = drm_atomic_state_alloc(dev);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+retry:
+ state->acquire_ctx = &ctx;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state)) {
+ ret = PTR_ERR(crtc_state);
+ goto out;
+ }
+
+ if (!crtc_state->enable)
+ goto out;
+
+ ret = drm_atomic_add_affected_connectors(state, crtc);
+ if (ret)
+ goto out;
+
+ for_each_new_connector_in_state(state, conn, conn_state, i) {
+ if (!conn_state->self_refresh_aware)
+ goto out;
+ }
+
+ crtc_state->active = false;
+ crtc_state->self_refresh_active = true;
+
+ ret = drm_atomic_commit(state);
+ if (ret)
+ goto out;
+
+out:
+ if (ret == -EDEADLK) {
+ drm_atomic_state_clear(state);
+ ret = drm_modeset_backoff(&ctx);
+ if (!ret)
+ goto retry;
+ }
+
+ drm_atomic_state_put(state);
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+}
+
+/**
+ * drm_self_refresh_helper_alter_state - Alters the atomic state for SR exit
+ * @state: the state currently being checked
+ *
+ * Called at the end of atomic check. This function checks the state for flags
+ * incompatible with self refresh exit and changes them. This is a bit
+ * disingenuous since userspace is expecting one thing and we're giving it
+ * another. However in order to keep self refresh entirely hidden from
+ * userspace, this is required.
+ *
+ * At the end, we queue up the self refresh entry work so we can enter PSR after
+ * the desired delay.
+ */
+void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state)
+{
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+ int i;
+
+ if (state->async_update || !state->allow_modeset) {
+ for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
+ if (crtc_state->self_refresh_active) {
+ state->async_update = false;
+ state->allow_modeset = true;
+ break;
+ }
+ }
+ }
+
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+ struct drm_self_refresh_data *sr_data;
+
+ /* Don't trigger the entry timer when we're already in SR */
+ if (crtc_state->self_refresh_active)
+ continue;
+
+ sr_data = crtc->self_refresh_data;
+ if (!sr_data)
+ continue;
+
+ mod_delayed_work(system_wq, &sr_data->entry_work,
+ msecs_to_jiffies(sr_data->entry_delay_ms));
+ }
+}
+EXPORT_SYMBOL(drm_self_refresh_helper_alter_state);
+
+/**
+ * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
+ * @crtc: the crtc which supports self refresh supported displays
+ * @entry_delay_ms: amount of inactivity to wait before entering self refresh
+ *
+ * Returns zero if successful or -errno on failure
+ */
+int drm_self_refresh_helper_init(struct drm_crtc *crtc,
+ unsigned int entry_delay_ms)
+{
+ struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;
+
+ /* Helper is already initialized */
+ if (WARN_ON(sr_data))
+ return -EINVAL;
+
+ sr_data = kzalloc(sizeof(*sr_data), GFP_KERNEL);
+ if (!sr_data)
+ return -ENOMEM;
+
+ INIT_DELAYED_WORK(&sr_data->entry_work,
+ drm_self_refresh_helper_entry_work);
+ sr_data->entry_delay_ms = entry_delay_ms;
+ sr_data->crtc = crtc;
+
+ crtc->self_refresh_data = sr_data;
+ return 0;
+}
+EXPORT_SYMBOL(drm_self_refresh_helper_init);
+
+/**
+ * drm_self_refresh_helper_cleanup - Cleans up self refresh helpers for a crtc
+ * @crtc: the crtc to cleanup
+ */
+void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc)
+{
+ struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;
+
+ /* Helper is already uninitialized */
+ if (sr_data)
+ return;
+
+ crtc->self_refresh_data = NULL;
+
+ cancel_delayed_work_sync(&sr_data->entry_work);
+ kfree(sr_data);
+}
+EXPORT_SYMBOL(drm_self_refresh_helper_cleanup);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f122156478010..927e1205d7aa4 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -957,4 +957,19 @@ drm_atomic_crtc_needs_modeset(const struct drm_crtc_state *state)
state->connectors_changed;
}
+/**
+ * drm_atomic_crtc_effectively_active - compute whether crtc is actually active
+ * @state: &drm_crtc_state for the CRTC
+ *
+ * When in self refresh mode, the crtc_state->active value will be false, since
+ * the crtc is off. However in some cases we're interested in whether the crtc
+ * is active, or effectively active (ie: it's connected to an active display).
+ * In these cases, use this function instead of just checking active.
+ */
+static inline bool
+drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state)
+{
+ return state->active || state->self_refresh_active;
+}
+
#endif /* DRM_ATOMIC_H_ */
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 071143bc0ebd1..c6f8486d8b8f3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -548,6 +548,20 @@ struct drm_connector_state {
/** @tv: TV connector state */
struct drm_tv_connector_state tv;
+ /**
+ * @self_refresh_aware:
+ *
+ * This tracks whether a connector is aware of the self refresh state.
+ * It should be set to true for those connector implementations which
+ * understand the self refresh state. This is needed since the crtc
+ * registers the self refresh helpers and it doesn't know if the
+ * connectors downstream have implemented self refresh entry/exit.
+ *
+ * Drivers should set this to true in atomic_check if they know how to
+ * handle self_refresh requests.
+ */
+ bool self_refresh_aware;
+
/**
* @picture_aspect_ratio: Connector property to control the
* HDMI infoframe aspect ratio setting.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index dc42b9e353333..4fda610d76f26 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -54,6 +54,7 @@ struct drm_mode_set;
struct drm_file;
struct drm_clip_rect;
struct drm_printer;
+struct drm_self_refresh_data;
struct device_node;
struct dma_fence;
struct edid;
@@ -300,6 +301,17 @@ struct drm_crtc_state {
*/
bool vrr_enabled;
+ /**
+ * @self_refresh_active:
+ *
+ * Used by the self refresh helpers to denote when a self refresh
+ * transition is occuring. This will be set on enable/disable callbacks
+ * when self refresh is being enabled or disabled. In some cases, it may
+ * not be desirable to fully shut off the crtc during self refresh.
+ * CRTC's can inspect this flag and determine the best course of action.
+ */
+ bool self_refresh_active;
+
/**
* @event:
*
@@ -1088,6 +1100,13 @@ struct drm_crtc {
* The name of the CRTC's fence timeline.
*/
char timeline_name[32];
+
+ /**
+ * @self_refresh_data: Holds the state for the self refresh helpers
+ *
+ * Initialized via drm_self_refresh_helper_register().
+ */
+ struct drm_self_refresh_data *self_refresh_data;
};
/**
diff --git a/include/drm/drm_self_refresh_helper.h b/include/drm/drm_self_refresh_helper.h
new file mode 100644
index 0000000000000..45c106fbb9b40
--- /dev/null
+++ b/include/drm/drm_self_refresh_helper.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Authors:
+ * Sean Paul <seanpaul@chromium.org>
+ */
+#ifndef DRM_SELF_REFRESH_HELPER_H_
+#define DRM_SELF_REFRESH_HELPER_H_
+
+struct drm_atomic_state;
+struct drm_crtc;
+
+void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state);
+
+int drm_self_refresh_helper_init(struct drm_crtc *crtc,
+ unsigned int entry_delay_ms);
+
+void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc);
+#endif
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v5 06/11] drm/rockchip: Use dirtyfb helper
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (4 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 17:39 ` Daniel Vetter
2019-06-11 16:08 ` [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr Sean Paul
` (5 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel; +Cc: David Airlie, linux-rockchip, Sean Paul, linux-arm-kernel
From: Sean Paul <seanpaul@chromium.org>
Instead of flushing all vops every time we get a dirtyfb call, use the
damage helper to kick off an atomic commit. Even though we don't use
damage clips, the helper commit will force us through the normal
psr_inhibit_get/put sequence.
Changes in v3:
- Added to the set
Changes in v4:
- None
Changes in v5:
- None
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-7-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-7-sean@poorly.run
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 31030cf81bc97..a888e31aa545a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -16,6 +16,7 @@
#include <drm/drm.h>
#include <drm/drmP.h>
#include <drm/drm_atomic.h>
+#include <drm/drm_damage_helper.h>
#include <drm/drm_fb_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_probe_helper.h>
@@ -25,20 +26,10 @@
#include "rockchip_drm_gem.h"
#include "rockchip_drm_psr.h"
-static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
- struct drm_file *file,
- unsigned int flags, unsigned int color,
- struct drm_clip_rect *clips,
- unsigned int num_clips)
-{
- rockchip_drm_psr_flush_all(fb->dev);
- return 0;
-}
-
static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
.destroy = drm_gem_fb_destroy,
.create_handle = drm_gem_fb_create_handle,
- .dirty = rockchip_drm_fb_dirty,
+ .dirty = drm_atomic_helper_dirtyfb,
};
static struct drm_framebuffer *
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 06/11] drm/rockchip: Use dirtyfb helper
2019-06-11 16:08 ` [PATCH v5 06/11] drm/rockchip: Use dirtyfb helper Sean Paul
@ 2019-06-11 17:39 ` Daniel Vetter
2019-06-11 18:09 ` Sean Paul
0 siblings, 1 reply; 27+ messages in thread
From: Daniel Vetter @ 2019-06-11 17:39 UTC (permalink / raw)
To: Sean Paul
Cc: David Airlie, dri-devel, linux-rockchip, Sean Paul,
linux-arm-kernel
On Tue, Jun 11, 2019 at 12:08:20PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> Instead of flushing all vops every time we get a dirtyfb call, use the
> damage helper to kick off an atomic commit. Even though we don't use
> damage clips, the helper commit will force us through the normal
> psr_inhibit_get/put sequence.
>
> Changes in v3:
> - Added to the set
> Changes in v4:
> - None
> Changes in v5:
> - None
>
> Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-7-sean@poorly.run
> Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-7-sean@poorly.run
>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
btw somewhat bigger fish to fry, but imo also very much worth it:
Switching over to the generic fbdev emulation.
Also, since everything up to this one is reviewed I think would be good to
land that part already. So I can go around and haggle this to other
drivers :-)
-Daniel
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 31030cf81bc97..a888e31aa545a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -16,6 +16,7 @@
> #include <drm/drm.h>
> #include <drm/drmP.h>
> #include <drm/drm_atomic.h>
> +#include <drm/drm_damage_helper.h>
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> #include <drm/drm_probe_helper.h>
> @@ -25,20 +26,10 @@
> #include "rockchip_drm_gem.h"
> #include "rockchip_drm_psr.h"
>
> -static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> - struct drm_file *file,
> - unsigned int flags, unsigned int color,
> - struct drm_clip_rect *clips,
> - unsigned int num_clips)
> -{
> - rockchip_drm_psr_flush_all(fb->dev);
> - return 0;
> -}
> -
> static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> .destroy = drm_gem_fb_destroy,
> .create_handle = drm_gem_fb_create_handle,
> - .dirty = rockchip_drm_fb_dirty,
> + .dirty = drm_atomic_helper_dirtyfb,
> };
>
> static struct drm_framebuffer *
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
--
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] 27+ messages in thread* Re: [PATCH v5 06/11] drm/rockchip: Use dirtyfb helper
2019-06-11 17:39 ` Daniel Vetter
@ 2019-06-11 18:09 ` Sean Paul
0 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 18:09 UTC (permalink / raw)
To: Daniel Vetter
Cc: David Airlie, dri-devel, linux-rockchip, Sean Paul, Sean Paul,
linux-arm-kernel
On Tue, Jun 11, 2019 at 07:39:12PM +0200, Daniel Vetter wrote:
> On Tue, Jun 11, 2019 at 12:08:20PM -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > Instead of flushing all vops every time we get a dirtyfb call, use the
> > damage helper to kick off an atomic commit. Even though we don't use
> > damage clips, the helper commit will force us through the normal
> > psr_inhibit_get/put sequence.
> >
> > Changes in v3:
> > - Added to the set
> > Changes in v4:
> > - None
> > Changes in v5:
> > - None
> >
> > Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-7-sean@poorly.run
> > Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-7-sean@poorly.run
> >
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Thanks!
>
> btw somewhat bigger fish to fry, but imo also very much worth it:
> Switching over to the generic fbdev emulation.
Agreed, this tends to get swept under the rug on CrOS devices since we don't use
fbdev :/
>
> Also, since everything up to this one is reviewed I think would be good to
> land that part already. So I can go around and haggle this to other
> drivers :-)
Sounds good to me, it'll be nice to offload some of the patches I've been
carrying.
Thanks again for the reviews,
Sean
> -Daniel
>
> > ---
> > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 13 ++-----------
> > 1 file changed, 2 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 31030cf81bc97..a888e31aa545a 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -16,6 +16,7 @@
> > #include <drm/drm.h>
> > #include <drm/drmP.h>
> > #include <drm/drm_atomic.h>
> > +#include <drm/drm_damage_helper.h>
> > #include <drm/drm_fb_helper.h>
> > #include <drm/drm_gem_framebuffer_helper.h>
> > #include <drm/drm_probe_helper.h>
> > @@ -25,20 +26,10 @@
> > #include "rockchip_drm_gem.h"
> > #include "rockchip_drm_psr.h"
> >
> > -static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> > - struct drm_file *file,
> > - unsigned int flags, unsigned int color,
> > - struct drm_clip_rect *clips,
> > - unsigned int num_clips)
> > -{
> > - rockchip_drm_psr_flush_all(fb->dev);
> > - return 0;
> > -}
> > -
> > static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> > .destroy = drm_gem_fb_destroy,
> > .create_handle = drm_gem_fb_create_handle,
> > - .dirty = rockchip_drm_fb_dirty,
> > + .dirty = drm_atomic_helper_dirtyfb,
> > };
> >
> > static struct drm_framebuffer *
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (5 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 06/11] drm/rockchip: Use dirtyfb helper Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-07-25 18:08 ` Sean Paul
2019-06-11 16:08 ` [PATCH v5 08/11] drm/rockchip: Use the helpers for PSR Sean Paul
` (4 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Zain Wang, David Airlie, Tomasz Figa, Sean Paul, Laurent Pinchart
From: Sean Paul <seanpaul@chromium.org>
Once we start shutting off the link during PSR, we're going to want fast
training to work. If the display doesn't support fast training, don't
enable psr.
Changes in v2:
- None
Changes in v3:
- None
Changes in v4:
- None
Changes in v5:
- None
Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-3-sean@poorly.run
Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-2-sean@poorly.run
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-9-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-8-sean@poorly.run
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index d2de98d441845..a07d19e08b803 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1040,16 +1040,17 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
if (ret)
return ret;
+ /* Check whether panel supports fast training */
+ ret = analogix_dp_fast_link_train_detection(dp);
+ if (ret)
+ dp->psr_enable = false;
+
if (dp->psr_enable) {
ret = analogix_dp_enable_sink_psr(dp);
if (ret)
return ret;
}
- /* Check whether panel supports fast training */
- ret = analogix_dp_fast_link_train_detection(dp);
- if (ret)
- dp->psr_enable = false;
return ret;
}
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr
2019-06-11 16:08 ` [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr Sean Paul
@ 2019-07-25 18:08 ` Sean Paul
2019-07-25 18:39 ` Heiko Stuebner
0 siblings, 1 reply; 27+ messages in thread
From: Sean Paul @ 2019-07-25 18:08 UTC (permalink / raw)
To: Heiko Stuebner
Cc: Zain Wang, David Airlie, dri-devel, Tomasz Figa, Sean Paul,
Laurent Pinchart
On Tue, Jun 11, 2019 at 12:08:21PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> Once we start shutting off the link during PSR, we're going to want fast
> training to work. If the display doesn't support fast training, don't
> enable psr.
>
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - None
> Changes in v5:
> - None
>
> Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-3-sean@poorly.run
> Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-2-sean@poorly.run
> Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-9-sean@poorly.run
> Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-8-sean@poorly.run
>
> Cc: Zain Wang <wzz@rock-chips.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
@Heiko, could you please review patches 7-11? I'm worried they're going to start
to rot and the helpers have already landed in Linus' tree.
Thanks,
Sean
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index d2de98d441845..a07d19e08b803 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1040,16 +1040,17 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
> if (ret)
> return ret;
>
> + /* Check whether panel supports fast training */
> + ret = analogix_dp_fast_link_train_detection(dp);
> + if (ret)
> + dp->psr_enable = false;
> +
> if (dp->psr_enable) {
> ret = analogix_dp_enable_sink_psr(dp);
> if (ret)
> return ret;
> }
>
> - /* Check whether panel supports fast training */
> - ret = analogix_dp_fast_link_train_detection(dp);
> - if (ret)
> - dp->psr_enable = false;
>
> return ret;
> }
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr
2019-07-25 18:08 ` Sean Paul
@ 2019-07-25 18:39 ` Heiko Stuebner
2019-07-26 18:52 ` Sean Paul
0 siblings, 1 reply; 27+ messages in thread
From: Heiko Stuebner @ 2019-07-25 18:39 UTC (permalink / raw)
To: Sean Paul
Cc: Zain Wang, David Airlie, dri-devel, Tomasz Figa, Sean Paul,
Laurent Pinchart
Hi Sean,
Am Donnerstag, 25. Juli 2019, 20:08:12 CEST schrieb Sean Paul:
> On Tue, Jun 11, 2019 at 12:08:21PM -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > Once we start shutting off the link during PSR, we're going to want fast
> > training to work. If the display doesn't support fast training, don't
> > enable psr.
> >
> > Changes in v2:
> > - None
> > Changes in v3:
> > - None
> > Changes in v4:
> > - None
> > Changes in v5:
> > - None
> >
> > Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-3-sean@poorly.run
> > Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-2-sean@poorly.run
> > Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-9-sean@poorly.run
> > Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-8-sean@poorly.run
> >
> > Cc: Zain Wang <wzz@rock-chips.com>
> > Cc: Tomasz Figa <tfiga@chromium.org>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
>
> @Heiko, could you please review patches 7-11? I'm worried they're going to start
> to rot and the helpers have already landed in Linus' tree.
I did look through the patches and nothing out of the ordinary sprang
out to my (somewhat untrained) eye and judging by my tested-tags I also
gave them a spin at some point - although it seems I never was actually on
cc for them.
In any case they look good to me, so if it helps you, you can add my
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
though should take the technical expertise on drm-internals with a
grain of salt.
Heiko
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index d2de98d441845..a07d19e08b803 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -1040,16 +1040,17 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
> > if (ret)
> > return ret;
> >
> > + /* Check whether panel supports fast training */
> > + ret = analogix_dp_fast_link_train_detection(dp);
> > + if (ret)
> > + dp->psr_enable = false;
> > +
> > if (dp->psr_enable) {
> > ret = analogix_dp_enable_sink_psr(dp);
> > if (ret)
> > return ret;
> > }
> >
> > - /* Check whether panel supports fast training */
> > - ret = analogix_dp_fast_link_train_detection(dp);
> > - if (ret)
> > - dp->psr_enable = false;
> >
> > return ret;
> > }
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr
2019-07-25 18:39 ` Heiko Stuebner
@ 2019-07-26 18:52 ` Sean Paul
0 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-07-26 18:52 UTC (permalink / raw)
To: Heiko Stuebner
Cc: Zain Wang, David Airlie, dri-devel, Tomasz Figa, Sean Paul,
Laurent Pinchart, Sean Paul
On Thu, Jul 25, 2019 at 08:39:42PM +0200, Heiko Stuebner wrote:
> Hi Sean,
>
> Am Donnerstag, 25. Juli 2019, 20:08:12 CEST schrieb Sean Paul:
> > On Tue, Jun 11, 2019 at 12:08:21PM -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > Once we start shutting off the link during PSR, we're going to want fast
> > > training to work. If the display doesn't support fast training, don't
> > > enable psr.
> > >
> > > Changes in v2:
> > > - None
> > > Changes in v3:
> > > - None
> > > Changes in v4:
> > > - None
> > > Changes in v5:
> > > - None
> > >
> > > Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-3-sean@poorly.run
> > > Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-2-sean@poorly.run
> > > Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-9-sean@poorly.run
> > > Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-8-sean@poorly.run
> > >
> > > Cc: Zain Wang <wzz@rock-chips.com>
> > > Cc: Tomasz Figa <tfiga@chromium.org>
> > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> >
> > @Heiko, could you please review patches 7-11? I'm worried they're going to start
> > to rot and the helpers have already landed in Linus' tree.
>
> I did look through the patches and nothing out of the ordinary sprang
> out to my (somewhat untrained) eye and judging by my tested-tags I also
> gave them a spin at some point - although it seems I never was actually on
> cc for them.
>
> In any case they look good to me, so if it helps you, you can add my
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> though should take the technical expertise on drm-internals with a
> grain of salt.
Thanks Heiko! Given how long this has been on the list and that both you and I
have tested it, I've pushed it to drm-misc-next with your R-b. I'll keep an eye
on things as it rolls out and hopefully everything will go smoothly.
Sean
>
>
> Heiko
>
>
>
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > index d2de98d441845..a07d19e08b803 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > @@ -1040,16 +1040,17 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
> > > if (ret)
> > > return ret;
> > >
> > > + /* Check whether panel supports fast training */
> > > + ret = analogix_dp_fast_link_train_detection(dp);
> > > + if (ret)
> > > + dp->psr_enable = false;
> > > +
> > > if (dp->psr_enable) {
> > > ret = analogix_dp_enable_sink_psr(dp);
> > > if (ret)
> > > return ret;
> > > }
> > >
> > > - /* Check whether panel supports fast training */
> > > - ret = analogix_dp_fast_link_train_detection(dp);
> > > - if (ret)
> > > - dp->psr_enable = false;
> > >
> > > return ret;
> > > }
> >
> >
>
>
>
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 08/11] drm/rockchip: Use the helpers for PSR
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (6 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 07/11] drm/rockchip: Check for fast link training before enabling psr Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 16:08 ` [PATCH v5 09/11] drm/rockchip: Use vop_win in vop_win_disable instead of vop_win_data Sean Paul
` (3 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Zain Wang, linux-rockchip, David Airlie, Tomasz Figa, Sean Paul,
Laurent Pinchart, linux-arm-kernel
From: Sean Paul <seanpaul@chromium.org>
Instead of rolling our own implementation for tracking when PSR should
be [in]active, use the new self refresh helpers to do the heavy lifting.
Changes in v2:
- updated to reflect changes made in the helpers
Changes in v3:
- use the new atomic hooks to inspect crtc state instead of needing conn state (Daniel)
Changes in v4:
- Use Laurent's get_new_connector_for_encoder helper (Daniel)
- Exit vop disable early if it's already off
Changes in v5:
- Rebase on latest drm-misc-next
- Resolve conflict with s/edp_vsc_psr/dp_sdp/ rename
- Resolve conflict with drm_atomic.h header inclusion
Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-4-sean@poorly.run
Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-3-sean@poorly.run
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-9-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-9-sean@poorly.run
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
.../drm/bridge/analogix/analogix_dp_core.c | 291 +++++++++++++-----
.../drm/bridge/analogix/analogix_dp_core.h | 2 +-
drivers/gpu/drm/rockchip/Makefile | 3 +-
.../gpu/drm/rockchip/analogix_dp-rockchip.c | 106 ++++---
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 5 -
drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 290 -----------------
drivers/gpu/drm/rockchip/rockchip_drm_psr.h | 30 --
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 39 ++-
include/drm/bridge/analogix_dp.h | 4 -
9 files changed, 295 insertions(+), 475 deletions(-)
delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index a07d19e08b803..feed08ef386e3 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -24,6 +24,7 @@
#include <linux/platform_device.h>
#include <drm/bridge/analogix_dp.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_crtc.h>
#include <drm/drm_device.h>
@@ -106,63 +107,7 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
return 0;
}
-int analogix_dp_psr_enabled(struct analogix_dp_device *dp)
-{
-
- return dp->psr_enable;
-}
-EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
-
-int analogix_dp_enable_psr(struct analogix_dp_device *dp)
-{
- struct dp_sdp psr_vsc;
-
- if (!dp->psr_enable)
- return 0;
-
- /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
- memset(&psr_vsc, 0, sizeof(psr_vsc));
- psr_vsc.sdp_header.HB0 = 0;
- psr_vsc.sdp_header.HB1 = 0x7;
- psr_vsc.sdp_header.HB2 = 0x2;
- psr_vsc.sdp_header.HB3 = 0x8;
-
- psr_vsc.db[0] = 0;
- psr_vsc.db[1] = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
-
- return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
-}
-EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
-
-int analogix_dp_disable_psr(struct analogix_dp_device *dp)
-{
- struct dp_sdp psr_vsc;
- int ret;
-
- if (!dp->psr_enable)
- return 0;
-
- /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
- memset(&psr_vsc, 0, sizeof(psr_vsc));
- psr_vsc.sdp_header.HB0 = 0;
- psr_vsc.sdp_header.HB1 = 0x7;
- psr_vsc.sdp_header.HB2 = 0x2;
- psr_vsc.sdp_header.HB3 = 0x8;
-
- psr_vsc.db[0] = 0;
- psr_vsc.db[1] = 0;
-
- ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
- if (ret != 1) {
- dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
- return ret;
- }
-
- return analogix_dp_send_psr_spd(dp, &psr_vsc, false);
-}
-EXPORT_SYMBOL_GPL(analogix_dp_disable_psr);
-
-static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
{
unsigned char psr_version;
int ret;
@@ -170,14 +115,11 @@ static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_SUPPORT, &psr_version);
if (ret != 1) {
dev_err(dp->dev, "failed to get PSR version, disable it\n");
- return ret;
+ return false;
}
dev_dbg(dp->dev, "Panel PSR version : %x\n", psr_version);
-
- dp->psr_enable = (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
-
- return 0;
+ return psr_version & DP_PSR_IS_SUPPORTED;
}
static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
@@ -200,7 +142,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
}
/* Main-Link transmitter remains active during PSR active states */
- psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
+ psr_en = DP_PSR_CRC_VERIFICATION;
ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
if (ret != 1) {
dev_err(dp->dev, "failed to set panel psr\n");
@@ -208,8 +150,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
}
/* Enable psr function */
- psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
- DP_PSR_CRC_VERIFICATION;
+ psr_en = DP_PSR_ENABLE | DP_PSR_CRC_VERIFICATION;
ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
if (ret != 1) {
dev_err(dp->dev, "failed to set panel psr\n");
@@ -218,10 +159,11 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
analogix_dp_enable_psr_crc(dp);
+ dp->psr_supported = true;
+
return 0;
end:
dev_err(dp->dev, "enable psr fail, force to disable psr\n");
- dp->psr_enable = false;
return ret;
}
@@ -1036,25 +978,90 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
}
}
- ret = analogix_dp_detect_sink_psr(dp);
- if (ret)
- return ret;
-
/* Check whether panel supports fast training */
ret = analogix_dp_fast_link_train_detection(dp);
if (ret)
- dp->psr_enable = false;
+ return ret;
- if (dp->psr_enable) {
+ if (analogix_dp_detect_sink_psr(dp)) {
ret = analogix_dp_enable_sink_psr(dp);
if (ret)
return ret;
}
+ return ret;
+}
+
+static int analogix_dp_enable_psr(struct analogix_dp_device *dp)
+{
+ struct dp_sdp psr_vsc;
+ int ret;
+ u8 sink;
+
+ ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
+ if (ret != 1)
+ DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
+ else if (sink == DP_PSR_SINK_ACTIVE_RFB)
+ return 0;
+
+ /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
+ memset(&psr_vsc, 0, sizeof(psr_vsc));
+ psr_vsc.sdp_header.HB0 = 0;
+ psr_vsc.sdp_header.HB1 = 0x7;
+ psr_vsc.sdp_header.HB2 = 0x2;
+ psr_vsc.sdp_header.HB3 = 0x8;
+ psr_vsc.db[0] = 0;
+ psr_vsc.db[1] = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
+
+ ret = analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+ if (!ret)
+ analogix_dp_set_analog_power_down(dp, POWER_ALL, true);
return ret;
}
+static int analogix_dp_disable_psr(struct analogix_dp_device *dp)
+{
+ struct dp_sdp psr_vsc;
+ int ret;
+ u8 sink;
+
+ analogix_dp_set_analog_power_down(dp, POWER_ALL, false);
+
+ ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+ if (ret != 1) {
+ DRM_DEV_ERROR(dp->dev, "Failed to set DP Power0 %d\n", ret);
+ return ret;
+ }
+
+ ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
+ if (ret != 1) {
+ DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
+ return ret;
+ } else if (sink == DP_PSR_SINK_INACTIVE) {
+ DRM_DEV_ERROR(dp->dev, "sink inactive, skip disable psr");
+ return 0;
+ }
+
+ ret = analogix_dp_train_link(dp);
+ if (ret) {
+ DRM_DEV_ERROR(dp->dev, "Failed to train the link %d\n", ret);
+ return ret;
+ }
+
+ /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
+ memset(&psr_vsc, 0, sizeof(psr_vsc));
+ psr_vsc.sdp_header.HB0 = 0;
+ psr_vsc.sdp_header.HB1 = 0x7;
+ psr_vsc.sdp_header.HB2 = 0x2;
+ psr_vsc.sdp_header.HB3 = 0x8;
+
+ psr_vsc.db[0] = 0;
+ psr_vsc.db[1] = 0;
+
+ return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+}
+
/*
* This function is a bit of a catch-all for panel preparation, hopefully
* simplifying the logic of functions that need to prepare/unprepare the panel
@@ -1145,9 +1152,37 @@ analogix_dp_best_encoder(struct drm_connector *connector)
return dp->encoder;
}
+
+int analogix_dp_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct analogix_dp_device *dp = to_dp(connector);
+ struct drm_connector_state *conn_state;
+ struct drm_crtc_state *crtc_state;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (WARN_ON(!conn_state))
+ return -ENODEV;
+
+ conn_state->self_refresh_aware = true;
+
+ if (!conn_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+ if (!crtc_state)
+ return 0;
+
+ if (crtc_state->self_refresh_active && !dp->psr_supported)
+ return -EINVAL;
+
+ return 0;
+}
+
static const struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
.get_modes = analogix_dp_get_modes,
.best_encoder = analogix_dp_best_encoder,
+ .atomic_check = analogix_dp_atomic_check,
};
static enum drm_connector_status
@@ -1239,11 +1274,42 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
return 0;
}
-static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
+static
+struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
+ struct drm_atomic_state *state)
+{
+ struct drm_encoder *encoder = dp->encoder;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+
+static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
{
struct analogix_dp_device *dp = bridge->driver_private;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
int ret;
+ crtc = analogix_dp_get_new_crtc(dp, state);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+ /* Don't touch the panel if we're coming back from PSR */
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
+
ret = analogix_dp_prepare_panel(dp, true, true);
if (ret)
DRM_ERROR("failed to setup the panel ret = %d\n", ret);
@@ -1304,10 +1370,27 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
return ret;
}
-static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
+static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
{
struct analogix_dp_device *dp = bridge->driver_private;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
int timeout_loop = 0;
+ int ret;
+
+ crtc = analogix_dp_get_new_crtc(dp, state);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+ /* Not a full enable, just disable PSR and continue */
+ if (old_crtc_state && old_crtc_state->self_refresh_active) {
+ ret = analogix_dp_disable_psr(dp);
+ if (ret)
+ DRM_ERROR("Failed to disable psr %d\n", ret);
+ return;
+ }
if (dp->dpms_mode == DRM_MODE_DPMS_ON)
return;
@@ -1356,11 +1439,56 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
if (ret)
DRM_ERROR("failed to setup the panel ret = %d\n", ret);
- dp->psr_enable = false;
dp->fast_train_enable = false;
+ dp->psr_supported = false;
dp->dpms_mode = DRM_MODE_DPMS_OFF;
}
+static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
+{
+ struct analogix_dp_device *dp = bridge->driver_private;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state = NULL;
+
+ crtc = analogix_dp_get_new_crtc(dp, state);
+ if (!crtc)
+ goto out;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ if (!new_crtc_state)
+ goto out;
+
+ /* Don't do a full disable on PSR transitions */
+ if (new_crtc_state->self_refresh_active)
+ return;
+
+out:
+ analogix_dp_bridge_disable(bridge);
+}
+
+static
+void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
+{
+ struct analogix_dp_device *dp = bridge->driver_private;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+ int ret;
+
+ crtc = analogix_dp_get_new_crtc(dp, state);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ if (!new_crtc_state || !new_crtc_state->self_refresh_active)
+ return;
+
+ ret = analogix_dp_enable_psr(dp);
+ if (ret)
+ DRM_ERROR("Failed to enable psr (%d)\n", ret);
+}
+
static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
const struct drm_display_mode *orig_mode,
const struct drm_display_mode *mode)
@@ -1440,16 +1568,11 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
video->interlaced = true;
}
-static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
-{
- /* do nothing */
-}
-
static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
- .pre_enable = analogix_dp_bridge_pre_enable,
- .enable = analogix_dp_bridge_enable,
- .disable = analogix_dp_bridge_disable,
- .post_disable = analogix_dp_bridge_nop,
+ .atomic_pre_enable = analogix_dp_bridge_atomic_pre_enable,
+ .atomic_enable = analogix_dp_bridge_atomic_enable,
+ .atomic_disable = analogix_dp_bridge_atomic_disable,
+ .atomic_post_disable = analogix_dp_bridge_atomic_post_disable,
.mode_set = analogix_dp_bridge_mode_set,
.attach = analogix_dp_bridge_attach,
};
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 3e5fe90edf71a..141bec67142d2 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -173,8 +173,8 @@ struct analogix_dp_device {
int dpms_mode;
int hpd_gpio;
bool force_hpd;
- bool psr_enable;
bool fast_train_enable;
+ bool psr_supported;
struct mutex panel_lock;
bool panel_is_modeset;
diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
index 524684ba7f6a5..17a9e7eb2130d 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -4,8 +4,7 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
- rockchip_drm_gem.o rockchip_drm_psr.o \
- rockchip_drm_vop.o rockchip_vop_reg.o
+ rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index bc44236242098..031185929a227 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -21,6 +21,8 @@
#include <linux/clk.h>
#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_dp_helper.h>
#include <drm/drm_of.h>
#include <drm/drm_panel.h>
@@ -32,7 +34,6 @@
#include <drm/bridge/analogix_dp.h>
#include "rockchip_drm_drv.h"
-#include "rockchip_drm_psr.h"
#include "rockchip_drm_vop.h"
#define RK3288_GRF_SOC_CON6 0x25c
@@ -77,29 +78,6 @@ struct rockchip_dp_device {
struct analogix_dp_plat_data plat_data;
};
-static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
-{
- struct rockchip_dp_device *dp = to_dp(encoder);
- int ret;
-
- if (!analogix_dp_psr_enabled(dp->adp))
- return 0;
-
- DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
-
- ret = rockchip_drm_wait_vact_end(dp->encoder.crtc,
- PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
- if (ret) {
- DRM_DEV_ERROR(dp->dev, "line flag interrupt did not arrive\n");
- return -ETIMEDOUT;
- }
-
- if (enabled)
- return analogix_dp_enable_psr(dp->adp);
- else
- return analogix_dp_disable_psr(dp->adp);
-}
-
static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
{
reset_control_assert(dp->rst);
@@ -130,21 +108,9 @@ static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
return ret;
}
-static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
-{
- struct rockchip_dp_device *dp = to_dp(plat_data);
-
- return rockchip_drm_psr_inhibit_put(&dp->encoder);
-}
-
static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
{
struct rockchip_dp_device *dp = to_dp(plat_data);
- int ret;
-
- ret = rockchip_drm_psr_inhibit_get(&dp->encoder);
- if (ret != 0)
- return ret;
clk_disable_unprepare(dp->pclk);
@@ -184,12 +150,42 @@ static void rockchip_dp_drm_encoder_mode_set(struct drm_encoder *encoder,
/* do nothing */
}
-static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
+static
+struct drm_crtc *rockchip_dp_drm_get_new_crtc(struct drm_encoder *encoder,
+ struct drm_atomic_state *state)
+{
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+
+static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
+ struct drm_atomic_state *state)
{
struct rockchip_dp_device *dp = to_dp(encoder);
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
int ret;
u32 val;
+ crtc = rockchip_dp_drm_get_new_crtc(encoder, state);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+ /* Coming back from self refresh, nothing to do */
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
+
ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
if (ret < 0)
return;
@@ -214,9 +210,27 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
clk_disable_unprepare(dp->grfclk);
}
-static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
+static void rockchip_dp_drm_encoder_disable(struct drm_encoder *encoder,
+ struct drm_atomic_state *state)
{
- /* do nothing */
+ struct rockchip_dp_device *dp = to_dp(encoder);
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state = NULL;
+ int ret;
+
+ crtc = rockchip_dp_drm_get_new_crtc(encoder, state);
+ /* No crtc means we're doing a full shutdown */
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ /* If we're not entering self-refresh, no need to wait for vact */
+ if (!new_crtc_state || !new_crtc_state->self_refresh_active)
+ return;
+
+ ret = rockchip_drm_wait_vact_end(crtc, PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
+ if (ret)
+ DRM_DEV_ERROR(dp->dev, "line flag irq timed out\n");
}
static int
@@ -245,8 +259,8 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
.mode_fixup = rockchip_dp_drm_encoder_mode_fixup,
.mode_set = rockchip_dp_drm_encoder_mode_set,
- .enable = rockchip_dp_drm_encoder_enable,
- .disable = rockchip_dp_drm_encoder_nop,
+ .atomic_enable = rockchip_dp_drm_encoder_enable,
+ .atomic_disable = rockchip_dp_drm_encoder_disable,
.atomic_check = rockchip_dp_drm_encoder_atomic_check,
};
@@ -338,23 +352,16 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
dp->plat_data.dev_type = dp->data->chip_type;
dp->plat_data.power_on_start = rockchip_dp_poweron_start;
- dp->plat_data.power_on_end = rockchip_dp_poweron_end;
dp->plat_data.power_off = rockchip_dp_powerdown;
dp->plat_data.get_modes = rockchip_dp_get_modes;
- ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
- if (ret < 0)
- goto err_cleanup_encoder;
-
dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
if (IS_ERR(dp->adp)) {
ret = PTR_ERR(dp->adp);
- goto err_unreg_psr;
+ goto err_cleanup_encoder;
}
return 0;
-err_unreg_psr:
- rockchip_drm_psr_unregister(&dp->encoder);
err_cleanup_encoder:
dp->encoder.funcs->destroy(&dp->encoder);
return ret;
@@ -366,7 +373,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
struct rockchip_dp_device *dp = dev_get_drvdata(dev);
analogix_dp_unbind(dp->adp);
- rockchip_drm_psr_unregister(&dp->encoder);
dp->encoder.funcs->destroy(&dp->encoder);
dp->adp = ERR_PTR(-ENODEV);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index a888e31aa545a..e305b8a586de2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -24,7 +24,6 @@
#include "rockchip_drm_drv.h"
#include "rockchip_drm_fb.h"
#include "rockchip_drm_gem.h"
-#include "rockchip_drm_psr.h"
static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
.destroy = drm_gem_fb_destroy,
@@ -118,8 +117,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
{
struct drm_device *dev = old_state->dev;
- rockchip_drm_psr_inhibit_get_state(old_state);
-
drm_atomic_helper_commit_modeset_disables(dev, old_state);
drm_atomic_helper_commit_modeset_enables(dev, old_state);
@@ -127,8 +124,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_planes(dev, old_state,
DRM_PLANE_COMMIT_ACTIVE_ONLY);
- rockchip_drm_psr_inhibit_put_state(old_state);
-
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
deleted file mode 100644
index a0c8bd235b675..0000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ /dev/null
@@ -1,290 +0,0 @@
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author: Yakir Yang <ykk@rock-chips.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- */
-
-#include <drm/drmP.h>
-#include <drm/drm_atomic.h>
-#include <drm/drm_probe_helper.h>
-
-#include "rockchip_drm_drv.h"
-#include "rockchip_drm_psr.h"
-
-#define PSR_FLUSH_TIMEOUT_MS 100
-
-struct psr_drv {
- struct list_head list;
- struct drm_encoder *encoder;
-
- struct mutex lock;
- int inhibit_count;
- bool enabled;
-
- struct delayed_work flush_work;
-
- int (*set)(struct drm_encoder *encoder, bool enable);
-};
-
-static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
-{
- struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
- struct psr_drv *psr;
-
- mutex_lock(&drm_drv->psr_list_lock);
- list_for_each_entry(psr, &drm_drv->psr_list, list) {
- if (psr->encoder == encoder)
- goto out;
- }
- psr = ERR_PTR(-ENODEV);
-
-out:
- mutex_unlock(&drm_drv->psr_list_lock);
- return psr;
-}
-
-static int psr_set_state_locked(struct psr_drv *psr, bool enable)
-{
- int ret;
-
- if (psr->inhibit_count > 0)
- return -EINVAL;
-
- if (enable == psr->enabled)
- return 0;
-
- ret = psr->set(psr->encoder, enable);
- if (ret)
- return ret;
-
- psr->enabled = enable;
- return 0;
-}
-
-static void psr_flush_handler(struct work_struct *work)
-{
- struct psr_drv *psr = container_of(to_delayed_work(work),
- struct psr_drv, flush_work);
-
- mutex_lock(&psr->lock);
- psr_set_state_locked(psr, true);
- mutex_unlock(&psr->lock);
-}
-
-/**
- * rockchip_drm_psr_inhibit_put - release PSR inhibit on given encoder
- * @encoder: encoder to obtain the PSR encoder
- *
- * Decrements PSR inhibit count on given encoder. Should be called only
- * for a PSR inhibit count increment done before. If PSR inhibit counter
- * reaches zero, PSR flush work is scheduled to make the hardware enter
- * PSR mode in PSR_FLUSH_TIMEOUT_MS.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder)
-{
- struct psr_drv *psr = find_psr_by_encoder(encoder);
-
- if (IS_ERR(psr))
- return PTR_ERR(psr);
-
- mutex_lock(&psr->lock);
- --psr->inhibit_count;
- WARN_ON(psr->inhibit_count < 0);
- if (!psr->inhibit_count)
- mod_delayed_work(system_wq, &psr->flush_work,
- PSR_FLUSH_TIMEOUT_MS);
- mutex_unlock(&psr->lock);
-
- return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put);
-
-void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder;
- u32 encoder_mask = 0;
- int i;
-
- for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
- encoder_mask |= crtc_state->encoder_mask;
- encoder_mask |= crtc->state->encoder_mask;
- }
-
- drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
- rockchip_drm_psr_inhibit_get(encoder);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get_state);
-
-void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder;
- u32 encoder_mask = 0;
- int i;
-
- for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
- encoder_mask |= crtc_state->encoder_mask;
- encoder_mask |= crtc->state->encoder_mask;
- }
-
- drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
- rockchip_drm_psr_inhibit_put(encoder);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put_state);
-
-/**
- * rockchip_drm_psr_inhibit_get - acquire PSR inhibit on given encoder
- * @encoder: encoder to obtain the PSR encoder
- *
- * Increments PSR inhibit count on given encoder. This function guarantees
- * that after it returns PSR is turned off on given encoder and no PSR-related
- * hardware state change occurs at least until a matching call to
- * rockchip_drm_psr_inhibit_put() is done.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder)
-{
- struct psr_drv *psr = find_psr_by_encoder(encoder);
-
- if (IS_ERR(psr))
- return PTR_ERR(psr);
-
- mutex_lock(&psr->lock);
- psr_set_state_locked(psr, false);
- ++psr->inhibit_count;
- mutex_unlock(&psr->lock);
- cancel_delayed_work_sync(&psr->flush_work);
-
- return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get);
-
-static void rockchip_drm_do_flush(struct psr_drv *psr)
-{
- cancel_delayed_work_sync(&psr->flush_work);
-
- mutex_lock(&psr->lock);
- if (!psr_set_state_locked(psr, false))
- mod_delayed_work(system_wq, &psr->flush_work,
- PSR_FLUSH_TIMEOUT_MS);
- mutex_unlock(&psr->lock);
-}
-
-/**
- * rockchip_drm_psr_flush_all - force to flush all registered PSR encoders
- * @dev: drm device
- *
- * Disable the PSR function for all registered encoders, and then enable the
- * PSR function back after PSR_FLUSH_TIMEOUT. If encoder PSR state have been
- * changed during flush time, then keep the state no change after flush
- * timeout.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-void rockchip_drm_psr_flush_all(struct drm_device *dev)
-{
- struct rockchip_drm_private *drm_drv = dev->dev_private;
- struct psr_drv *psr;
-
- mutex_lock(&drm_drv->psr_list_lock);
- list_for_each_entry(psr, &drm_drv->psr_list, list)
- rockchip_drm_do_flush(psr);
- mutex_unlock(&drm_drv->psr_list_lock);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
-
-/**
- * rockchip_drm_psr_register - register encoder to psr driver
- * @encoder: encoder that obtain the PSR function
- * @psr_set: call back to set PSR state
- *
- * The function returns with PSR inhibit counter initialized with one
- * and the caller (typically encoder driver) needs to call
- * rockchip_drm_psr_inhibit_put() when it becomes ready to accept PSR
- * enable request.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_register(struct drm_encoder *encoder,
- int (*psr_set)(struct drm_encoder *, bool enable))
-{
- struct rockchip_drm_private *drm_drv;
- struct psr_drv *psr;
-
- if (!encoder || !psr_set)
- return -EINVAL;
-
- drm_drv = encoder->dev->dev_private;
-
- psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
- if (!psr)
- return -ENOMEM;
-
- INIT_DELAYED_WORK(&psr->flush_work, psr_flush_handler);
- mutex_init(&psr->lock);
-
- psr->inhibit_count = 1;
- psr->enabled = false;
- psr->encoder = encoder;
- psr->set = psr_set;
-
- mutex_lock(&drm_drv->psr_list_lock);
- list_add_tail(&psr->list, &drm_drv->psr_list);
- mutex_unlock(&drm_drv->psr_list_lock);
-
- return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_register);
-
-/**
- * rockchip_drm_psr_unregister - unregister encoder to psr driver
- * @encoder: encoder that obtain the PSR function
- * @psr_set: call back to set PSR state
- *
- * It is expected that the PSR inhibit counter is 1 when this function is
- * called, which corresponds to a state when related encoder has been
- * disconnected from any CRTCs and its driver called
- * rockchip_drm_psr_inhibit_get() to stop the PSR logic.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
-{
- struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
- struct psr_drv *psr, *n;
-
- mutex_lock(&drm_drv->psr_list_lock);
- list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) {
- if (psr->encoder == encoder) {
- /*
- * Any other value would mean that the encoder
- * is still in use.
- */
- WARN_ON(psr->inhibit_count != 1);
-
- list_del(&psr->list);
- kfree(psr);
- }
- }
- mutex_unlock(&drm_drv->psr_list_lock);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_unregister);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
deleted file mode 100644
index 25350ba3237b6..0000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author: Yakir Yang <ykk@rock-chips.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- */
-
-#ifndef __ROCKCHIP_DRM_PSR___
-#define __ROCKCHIP_DRM_PSR___
-
-void rockchip_drm_psr_flush_all(struct drm_device *dev);
-
-int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder);
-int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder);
-
-void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state);
-void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state);
-
-int rockchip_drm_psr_register(struct drm_encoder *encoder,
- int (*psr_set)(struct drm_encoder *, bool enable));
-void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
-
-#endif /* __ROCKCHIP_DRM_PSR__ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 4189ca17f3818..1ff79b2f9afa8 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -21,6 +21,7 @@
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_plane_helper.h>
#include <drm/drm_probe_helper.h>
+#include <drm/drm_self_refresh_helper.h>
#ifdef CONFIG_DRM_ANALOGIX_DP
#include <drm/bridge/analogix_dp.h>
#endif
@@ -42,10 +43,11 @@
#include "rockchip_drm_drv.h"
#include "rockchip_drm_gem.h"
#include "rockchip_drm_fb.h"
-#include "rockchip_drm_psr.h"
#include "rockchip_drm_vop.h"
#include "rockchip_rgb.h"
+#define VOP_SELF_REFRESH_ENTRY_DELAY_MS 100
+
#define VOP_WIN_SET(vop, win, name, v) \
vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name)
#define VOP_SCL_SET(vop, win, name, v) \
@@ -548,7 +550,7 @@ static void vop_win_disable(struct vop *vop, const struct vop_win_data *win)
VOP_WIN_SET(vop, win, enable, 0);
}
-static int vop_enable(struct drm_crtc *crtc)
+static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
{
struct vop *vop = to_vop(crtc);
int ret, i;
@@ -588,12 +590,18 @@ static int vop_enable(struct drm_crtc *crtc)
* We need to make sure that all windows are disabled before we
* enable the crtc. Otherwise we might try to scan from a destroyed
* buffer later.
+ *
+ * In the case of enable-after-PSR, we don't need to worry about this
+ * case since the buffer is guaranteed to be valid and disabling the
+ * window will result in screen glitches on PSR exit.
*/
- for (i = 0; i < vop->data->win_size; i++) {
- struct vop_win *vop_win = &vop->win[i];
- const struct vop_win_data *win = vop_win->data;
+ if (!old_state || !old_state->self_refresh_active) {
+ for (i = 0; i < vop->data->win_size; i++) {
+ struct vop_win *vop_win = &vop->win[i];
+ const struct vop_win_data *win = vop_win->data;
- vop_win_disable(vop, win);
+ vop_win_disable(vop, win);
+ }
}
spin_unlock(&vop->reg_lock);
@@ -631,6 +639,12 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
WARN_ON(vop->event);
mutex_lock(&vop->vop_lock);
+
+ if (!vop->is_enabled) {
+ mutex_unlock(&vop->vop_lock);
+ return;
+ }
+
drm_crtc_vblank_off(crtc);
/*
@@ -944,12 +958,10 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
}
if (vop->is_enabled) {
- rockchip_drm_psr_inhibit_get_state(new_state->state);
vop_plane_atomic_update(plane, plane->state);
spin_lock(&vop->reg_lock);
vop_cfg_done(vop);
spin_unlock(&vop->reg_lock);
- rockchip_drm_psr_inhibit_put_state(new_state->state);
}
plane->funcs->atomic_destroy_state(plane, plane_state);
@@ -1043,7 +1055,7 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
WARN_ON(vop->event);
- ret = vop_enable(crtc);
+ ret = vop_enable(crtc, old_state);
if (ret) {
mutex_unlock(&vop->vop_lock);
DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
@@ -1525,6 +1537,13 @@ static int vop_create_crtc(struct vop *vop)
init_completion(&vop->line_flag_completion);
crtc->port = port;
+ ret = drm_self_refresh_helper_init(crtc,
+ VOP_SELF_REFRESH_ENTRY_DELAY_MS);
+ if (ret)
+ DRM_DEV_DEBUG_KMS(vop->dev,
+ "Failed to init %s with SR helpers %d, ignoring\n",
+ crtc->name, ret);
+
return 0;
err_cleanup_crtc:
@@ -1542,6 +1561,8 @@ static void vop_destroy_crtc(struct vop *vop)
struct drm_device *drm_dev = vop->drm_dev;
struct drm_plane *plane, *tmp;
+ drm_self_refresh_helper_cleanup(crtc);
+
of_node_put(crtc->port);
/*
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 475b706b49de2..7c5c138036732 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -42,10 +42,6 @@ struct analogix_dp_plat_data {
struct drm_connector *);
};
-int analogix_dp_psr_enabled(struct analogix_dp_device *dp);
-int analogix_dp_enable_psr(struct analogix_dp_device *dp);
-int analogix_dp_disable_psr(struct analogix_dp_device *dp);
-
int analogix_dp_resume(struct analogix_dp_device *dp);
int analogix_dp_suspend(struct analogix_dp_device *dp);
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 09/11] drm/rockchip: Use vop_win in vop_win_disable instead of vop_win_data
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (7 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 08/11] drm/rockchip: Use the helpers for PSR Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 16:08 ` [PATCH v5 10/11] drm/rockchip: Don't fully disable vop on self refresh Sean Paul
` (2 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel; +Cc: David Airlie, linux-rockchip, Sean Paul, linux-arm-kernel
From: Sean Paul <seanpaul@chromium.org>
Change the argument to vop_win_disable to vop_win to accomodate future
changes to the function.
Changes in v4:
- Added to the patchset
Changes in v5:
- None
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-10-sean@poorly.run
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 1ff79b2f9afa8..83c6884f558d5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -538,8 +538,10 @@ static void vop_core_clks_disable(struct vop *vop)
clk_disable(vop->hclk);
}
-static void vop_win_disable(struct vop *vop, const struct vop_win_data *win)
+static void vop_win_disable(struct vop *vop, const struct vop_win *vop_win)
{
+ const struct vop_win_data *win = vop_win->data;
+
if (win->phy->scl && win->phy->scl->ext) {
VOP_SCL_SET_EXT(vop, win, yrgb_hor_scl_mode, SCALE_NONE);
VOP_SCL_SET_EXT(vop, win, yrgb_ver_scl_mode, SCALE_NONE);
@@ -598,9 +600,8 @@ static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
if (!old_state || !old_state->self_refresh_active) {
for (i = 0; i < vop->data->win_size; i++) {
struct vop_win *vop_win = &vop->win[i];
- const struct vop_win_data *win = vop_win->data;
- vop_win_disable(vop, win);
+ vop_win_disable(vop, vop_win);
}
}
spin_unlock(&vop->reg_lock);
@@ -748,7 +749,6 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
struct drm_plane_state *old_state)
{
struct vop_win *vop_win = to_vop_win(plane);
- const struct vop_win_data *win = vop_win->data;
struct vop *vop = to_vop(old_state->crtc);
if (!old_state->crtc)
@@ -756,7 +756,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
spin_lock(&vop->reg_lock);
- vop_win_disable(vop, win);
+ vop_win_disable(vop, vop_win);
spin_unlock(&vop->reg_lock);
}
@@ -1587,7 +1587,6 @@ static void vop_destroy_crtc(struct vop *vop)
static int vop_initial(struct vop *vop)
{
- const struct vop_data *vop_data = vop->data;
struct reset_control *ahb_rst;
int i, ret;
@@ -1654,12 +1653,13 @@ static int vop_initial(struct vop *vop)
VOP_REG_SET(vop, misc, global_regdone_en, 1);
VOP_REG_SET(vop, common, dsp_blank, 0);
- for (i = 0; i < vop_data->win_size; i++) {
- const struct vop_win_data *win = &vop_data->win[i];
+ for (i = 0; i < vop->data->win_size; i++) {
+ struct vop_win *vop_win = &vop->win[i];
+ const struct vop_win_data *win = vop_win->data;
int channel = i * 2 + 1;
VOP_WIN_SET(vop, win, channel, (channel + 1) << 4 | channel);
- vop_win_disable(vop, win);
+ vop_win_disable(vop, vop_win);
VOP_WIN_SET(vop, win, gate, 1);
}
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 10/11] drm/rockchip: Don't fully disable vop on self refresh
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (8 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 09/11] drm/rockchip: Use vop_win in vop_win_disable instead of vop_win_data Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-11 16:08 ` [PATCH v5 11/11] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul
2019-06-13 19:05 ` [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
11 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Zain Wang, David Airlie, Tomasz Figa, linux-rockchip, Sean Paul,
Kristian H . Kristensen, linux-arm-kernel
From: Sean Paul <seanpaul@chromium.org>
Instead of fully disabling and re-enabling the vop on self refresh
transitions, only disable the active windows. This will speed up
self refresh exits substantially and is still a power-savings win.
This patch integrates portions of Zain's patch from here:
https://patchwork.kernel.org/patch/9615063/
Changes in v2:
- None
Changes in v3:
- None
Changes in v4:
- Adjust for preceding vop_win_disable changes
Changes in v5:
- None
Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-5-sean@poorly.run
Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-4-sean@poorly.run
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-10-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-11-sean@poorly.run
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 ++++++++++++++++++---
1 file changed, 36 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 83c6884f558d5..12ca67284811e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -134,6 +134,7 @@ struct vop {
bool is_enabled;
struct completion dsp_hold_completion;
+ unsigned int win_enabled;
/* protected by dev->event_lock */
struct drm_pending_vblank_event *event;
@@ -550,6 +551,7 @@ static void vop_win_disable(struct vop *vop, const struct vop_win *vop_win)
}
VOP_WIN_SET(vop, win, enable, 0);
+ vop->win_enabled &= ~BIT(VOP_WIN_TO_INDEX(vop_win));
}
static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
@@ -632,6 +634,25 @@ static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
return ret;
}
+static void rockchip_drm_set_win_enabled(struct drm_crtc *crtc, bool enabled)
+{
+ struct vop *vop = to_vop(crtc);
+ int i;
+
+ spin_lock(&vop->reg_lock);
+
+ for (i = 0; i < vop->data->win_size; i++) {
+ struct vop_win *vop_win = &vop->win[i];
+ const struct vop_win_data *win = vop_win->data;
+
+ VOP_WIN_SET(vop, win, enable,
+ enabled && (vop->win_enabled & BIT(i)));
+ }
+ vop_cfg_done(vop);
+
+ spin_unlock(&vop->reg_lock);
+}
+
static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_crtc_state *old_state)
{
@@ -639,15 +660,16 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
WARN_ON(vop->event);
- mutex_lock(&vop->vop_lock);
+ if (crtc->state->self_refresh_active)
+ rockchip_drm_set_win_enabled(crtc, false);
- if (!vop->is_enabled) {
- mutex_unlock(&vop->vop_lock);
- return;
- }
+ mutex_lock(&vop->vop_lock);
drm_crtc_vblank_off(crtc);
+ if (crtc->state->self_refresh_active)
+ goto out;
+
/*
* Vop standby will take effect at end of current frame,
* if dsp hold valid irq happen, it means standby complete.
@@ -678,6 +700,8 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
clk_disable(vop->dclk);
vop_core_clks_disable(vop);
pm_runtime_put(vop->dev);
+
+out:
mutex_unlock(&vop->vop_lock);
if (crtc->state->event && !crtc->state->active) {
@@ -895,6 +919,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
}
VOP_WIN_SET(vop, win, enable, 1);
+ vop->win_enabled |= BIT(win_index);
spin_unlock(&vop->reg_lock);
}
@@ -1051,6 +1076,12 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
int dither_bpc = s->output_bpc ? s->output_bpc : 10;
int ret;
+ if (old_state && old_state->self_refresh_active) {
+ drm_crtc_vblank_on(crtc);
+ rockchip_drm_set_win_enabled(crtc, true);
+ return;
+ }
+
mutex_lock(&vop->vop_lock);
WARN_ON(vop->event);
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 11/11] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (9 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 10/11] drm/rockchip: Don't fully disable vop on self refresh Sean Paul
@ 2019-06-11 16:08 ` Sean Paul
2019-06-13 19:05 ` [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
11 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-11 16:08 UTC (permalink / raw)
To: dri-devel
Cc: Zain Wang, David Airlie, Tomasz Figa, linux-rockchip, Sean Paul,
linux-arm-kernel
From: Sean Paul <seanpaul@chromium.org>
Now that we use the drm psr helpers, we no longer need to hand-roll our
atomic_commit_tail implementation. So use the helper
Changes in v2:
- None
Changes in v3:
- None
Changes in v4:
- None
Changes in v5:
- None
Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-6-sean@poorly.run
Link to v2: https://patchwork.freedesktop.org/patch/msgid/20190326204509.96515-5-sean@poorly.run
Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-11-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-12-sean@poorly.run
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index e305b8a586de2..7d1c5d8e74487 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -112,27 +112,8 @@ rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
return ERR_PTR(ret);
}
-static void
-rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
-{
- struct drm_device *dev = old_state->dev;
-
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
-
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
-
- drm_atomic_helper_commit_planes(dev, old_state,
- DRM_PLANE_COMMIT_ACTIVE_ONLY);
-
- drm_atomic_helper_commit_hw_done(old_state);
-
- drm_atomic_helper_wait_for_vblanks(dev, old_state);
-
- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
-
static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
- .atomic_commit_tail = rockchip_atomic_helper_commit_tail_rpm,
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
};
static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 00/11] drm: Add self refresh helpers
2019-06-11 16:08 [PATCH v5 00/11] drm: Add self refresh helpers Sean Paul
` (10 preceding siblings ...)
2019-06-11 16:08 ` [PATCH v5 11/11] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul
@ 2019-06-13 19:05 ` Sean Paul
11 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2019-06-13 19:05 UTC (permalink / raw)
To: dri-devel; +Cc: Sean Paul
On Tue, Jun 11, 2019 at 12:08:14PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
I've pushed the first 6 (all reviewed) patches to drm-misc-next, I'll wait for a
rockchip review on the last 5 and apply them when that comes through.
Thanks all for the comments!
Sean
>
> Back again with a new version. Very little has changed from v5, in fact
> I was just going to resend patch 2, but I ran into some collisions while
> rebasing. I figured I'd take the opportunity to collect and apply the
> tags I've accumulated so far and send the whole thing again.
>
> I need reviews on patch 2 and patches 6 through 11 (the rockchip bits).
>
> Link to v4 cover: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-1-sean@poorly.run
>
> Thanks,
>
> Sean
>
>
> Laurent Pinchart (1):
> drm: Add drm_atomic_get_(old|new_connector_for_encoder() helpers
>
> Sean Paul (10):
> drm: Add atomic variants of enable/disable to encoder helper funcs
> drm: Add atomic variants for bridge enable/disable
> drm: Convert connector_helper_funcs->atomic_check to accept
> drm_atomic_state
> drm: Add helpers to kick off self refresh mode in drivers
> drm/rockchip: Use dirtyfb helper
> drm/rockchip: Check for fast link training before enabling psr
> drm/rockchip: Use the helpers for PSR
> drm/rockchip: Use vop_win in vop_win_disable instead of vop_win_data
> drm/rockchip: Don't fully disable vop on self refresh
> drm/rockchip: Use drm_atomic_helper_commit_tail_rpm
>
> Documentation/gpu/drm-kms-helpers.rst | 9 +
> drivers/gpu/drm/Makefile | 2 +-
> .../drm/bridge/analogix/analogix_dp_core.c | 292 +++++++++++++-----
> .../drm/bridge/analogix/analogix_dp_core.h | 2 +-
> drivers/gpu/drm/drm_atomic.c | 71 +++++
> drivers/gpu/drm/drm_atomic_helper.c | 55 +++-
> drivers/gpu/drm/drm_atomic_state_helper.c | 4 +
> drivers/gpu/drm/drm_atomic_uapi.c | 7 +-
> drivers/gpu/drm/drm_bridge.c | 110 +++++++
> drivers/gpu/drm/drm_self_refresh_helper.c | 213 +++++++++++++
> drivers/gpu/drm/i915/intel_atomic.c | 8 +-
> drivers/gpu/drm/i915/intel_atomic.h | 2 +-
> drivers/gpu/drm/i915/intel_dp_mst.c | 7 +-
> drivers/gpu/drm/i915/intel_sdvo.c | 9 +-
> drivers/gpu/drm/i915/intel_tv.c | 8 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +-
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +-
> drivers/gpu/drm/rockchip/Makefile | 3 +-
> .../gpu/drm/rockchip/analogix_dp-rockchip.c | 106 ++++---
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 39 +--
> drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 290 -----------------
> drivers/gpu/drm/rockchip/rockchip_drm_psr.h | 30 --
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 84 ++++-
> drivers/gpu/drm/vc4/vc4_txp.c | 7 +-
> include/drm/bridge/analogix_dp.h | 4 -
> include/drm/drm_atomic.h | 22 ++
> include/drm/drm_bridge.h | 106 +++++++
> include/drm/drm_connector.h | 19 ++
> include/drm/drm_crtc.h | 19 ++
> include/drm/drm_modeset_helper_vtables.h | 50 ++-
> include/drm/drm_self_refresh_helper.h | 22 ++
> 31 files changed, 1066 insertions(+), 551 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_self_refresh_helper.c
> delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> create mode 100644 include/drm/drm_self_refresh_helper.h
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 27+ messages in thread