[PATCH 1/2] drm/atomic-helper: Add atomic_mode_set version to extend encoder mode_set

[PATCH 1/2] drm/atomic-helper: Add atomic_mode_set version to extend encoder mode_set

[Philipp Zabel]

Some encoders need more information from crtc and connector state
than just the mode. Add an atomic encoder mode setting variant
that passes the crtc state (which contains the modes) and the
connector state.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/drm_atomic_helper.c      |  6 +++++-
 include/drm/drm_modeset_helper_vtables.h | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index de7fddc..a3c033f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -880,8 +880,12 @@ crtc_set_mode(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 mode_set hooks twice.
 		 */
-		if (funcs && funcs->mode_set)
+		if (funcs && funcs->atomic_mode_set) {
+			funcs->atomic_mode_set(encoder, new_crtc_state,
+					       connector->state);
+		} else if (funcs && funcs->mode_set) {
 			funcs->mode_set(encoder, mode, adjusted_mode);
+		}
 
 		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
 	}
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index b55f218..1b15c1f 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -529,6 +529,26 @@ struct drm_encoder_helper_funcs {
 			 struct drm_display_mode *adjusted_mode);
 
 	/**
+	 * @atomic_mode_set:
+	 *
+	 * This callback is used to update the display mode of an encoder.
+	 *
+	 * Note that the display pipe is completely off when this function is
+	 * called. Drivers which need hardware to be running before they program
+	 * the new display mode (because they implement runtime PM) should not
+	 * use this hook, because the helper library calls it only once and not
+	 * every time the display pipeline is suspended using either DPMS or the
+	 * new "ACTIVE" property. Such drivers should instead move all their
+	 * encoder setup into the ->enable() callback.
+	 *
+	 * This callback is used by the atomic modeset helpers instead of the
+	 * mode_set callback. It is optional in the atomic helpers.
+	 */
+	void (*atomic_mode_set)(struct drm_encoder *encoder,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *conn_state);
+
+	/**
 	 * @get_crtc:
 	 *
 	 * This callback is used by the legacy CRTC helpers to work around
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/imx: imx-ldb: use encoder atomic_mode_set callback

[Philipp Zabel]

Using atomic_mode_set instead of mode_set allows to access crtc
and connector states in addition to the modes. This allows to
remove the connector list walk.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 3ed2d50..7b588b4 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -252,11 +252,13 @@ static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
 	drm_panel_enable(imx_ldb_ch->panel);
 }
 
-static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *orig_mode,
-			 struct drm_display_mode *mode)
+static void
+imx_ldb_encoder_atomic_mode_set(struct drm_encoder *encoder,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *connector_state)
 {
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
 	unsigned long serial_clk;
@@ -298,17 +300,11 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 	}
 
 	if (!bus_format) {
-		struct drm_connector *connector;
+		struct drm_connector *connector = connector_state->connector;
+		struct drm_display_info *di = &connector->display_info;
 
-		drm_for_each_connector(connector, encoder->dev) {
-			struct drm_display_info *di = &connector->display_info;
-
-			if (connector->encoder == encoder &&
-			    di->num_bus_formats) {
-				bus_format = di->bus_formats[0];
-				break;
-			}
-		}
+		if (di->num_bus_formats)
+			bus_format = di->bus_formats[0];
 	}
 	imx_ldb_ch_set_bus_format(imx_ldb_ch, bus_format);
 }
@@ -426,7 +422,7 @@ static const struct drm_encoder_funcs imx_ldb_encoder_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs imx_ldb_encoder_helper_funcs = {
-	.mode_set = imx_ldb_encoder_mode_set,
+	.atomic_mode_set = imx_ldb_encoder_atomic_mode_set,
 	.enable = imx_ldb_encoder_enable,
 	.disable = imx_ldb_encoder_disable,
 	.atomic_check = imx_ldb_encoder_atomic_check,
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/atomic-helper: Add atomic_mode_set version to extend encoder mode_set

[Daniel Vetter]

On Fri, Jul 22, 2016 at 02:09:12PM +0200, Philipp Zabel wrote:
> Some encoders need more information from crtc and connector state
> than just the mode. Add an atomic encoder mode setting variant
> that passes the crtc state (which contains the modes) and the
> connector state.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++++-
>  include/drm/drm_modeset_helper_vtables.h | 20 ++++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index de7fddc..a3c033f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -880,8 +880,12 @@ crtc_set_mode(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 mode_set hooks twice.
>  		 */
> -		if (funcs && funcs->mode_set)
> +		if (funcs && funcs->atomic_mode_set) {
> +			funcs->atomic_mode_set(encoder, new_crtc_state,
> +					       connector->state);
> +		} else if (funcs && funcs->mode_set) {
>  			funcs->mode_set(encoder, mode, adjusted_mode);
> +		}
>  
>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>  	}
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index b55f218..1b15c1f 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -529,6 +529,26 @@ struct drm_encoder_helper_funcs {
>  			 struct drm_display_mode *adjusted_mode);
>  
>  	/**
> +	 * @atomic_mode_set:
> +	 *
> +	 * This callback is used to update the display mode of an encoder.
> +	 *
> +	 * Note that the display pipe is completely off when this function is
> +	 * called. Drivers which need hardware to be running before they program
> +	 * the new display mode (because they implement runtime PM) should not
> +	 * use this hook, because the helper library calls it only once and not
> +	 * every time the display pipeline is suspended using either DPMS or the
> +	 * new "ACTIVE" property. Such drivers should instead move all their
> +	 * encoder setup into the ->enable() callback.
> +	 *
> +	 * This callback is used by the atomic modeset helpers instead of the
> +	 * mode_set callback. It is optional in the atomic helpers.

s/mode_set/@mode_set/ Also pls a bit of text when this should be used (any
time you need to inspect connector state, since there's no direct way to
go from the encoder to the current connector). And pls add a note to
@mode_set that drivers should use @atomic_mode_set in such a case, for
cross linking.

Great docs matter ;-)

While at it I noticed that the kernel-doc for @mode_set is wrong, it says
it's only used by CRTC helpers. Can you pls fix that too?

Thanks, Daniel
> +	 */
> +	void (*atomic_mode_set)(struct drm_encoder *encoder,
> +				struct drm_crtc_state *crtc_state,
> +				struct drm_connector_state *conn_state);
> +
> +	/**
>  	 * @get_crtc:
>  	 *
>  	 * This callback is used by the legacy CRTC helpers to work around
> -- 
> 2.8.1
> 

-- 
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

Re: [PATCH 1/2] drm/atomic-helper: Add atomic_mode_set version to extend encoder mode_set

[Daniel Vetter]

On Fri, Jul 22, 2016 at 06:53:41PM +0200, Daniel Vetter wrote:
> On Fri, Jul 22, 2016 at 02:09:12PM +0200, Philipp Zabel wrote:
> > Some encoders need more information from crtc and connector state
> > than just the mode. Add an atomic encoder mode setting variant
> > that passes the crtc state (which contains the modes) and the
> > connector state.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++++-
> >  include/drm/drm_modeset_helper_vtables.h | 20 ++++++++++++++++++++
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index de7fddc..a3c033f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -880,8 +880,12 @@ crtc_set_mode(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 mode_set hooks twice.
> >  		 */
> > -		if (funcs && funcs->mode_set)
> > +		if (funcs && funcs->atomic_mode_set) {
> > +			funcs->atomic_mode_set(encoder, new_crtc_state,
> > +					       connector->state);
> > +		} else if (funcs && funcs->mode_set) {
> >  			funcs->mode_set(encoder, mode, adjusted_mode);
> > +		}
> >  
> >  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> >  	}
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index b55f218..1b15c1f 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -529,6 +529,26 @@ struct drm_encoder_helper_funcs {
> >  			 struct drm_display_mode *adjusted_mode);
> >  
> >  	/**
> > +	 * @atomic_mode_set:
> > +	 *
> > +	 * This callback is used to update the display mode of an encoder.
> > +	 *
> > +	 * Note that the display pipe is completely off when this function is
> > +	 * called. Drivers which need hardware to be running before they program
> > +	 * the new display mode (because they implement runtime PM) should not
> > +	 * use this hook, because the helper library calls it only once and not
> > +	 * every time the display pipeline is suspended using either DPMS or the
> > +	 * new "ACTIVE" property. Such drivers should instead move all their
> > +	 * encoder setup into the ->enable() callback.
> > +	 *
> > +	 * This callback is used by the atomic modeset helpers instead of the
> > +	 * mode_set callback. It is optional in the atomic helpers.
> 
> s/mode_set/@mode_set/ Also pls a bit of text when this should be used (any
> time you need to inspect connector state, since there's no direct way to
> go from the encoder to the current connector). And pls add a note to
> @mode_set that drivers should use @atomic_mode_set in such a case, for
> cross linking.
> 
> Great docs matter ;-)
> 
> While at it I noticed that the kernel-doc for @mode_set is wrong, it says
> it's only used by CRTC helpers. Can you pls fix that too?

Strike that part, I accidentally read the kerneldoc for crtc->mode_fixup.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel