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

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