Re: [PATCH] drm: Update docs for legacy kms state

[Manasi Navare <manasi.d.navare@intel.com>]

On Wed, Nov 08, 2017 at 09:30:07PM +0100, Daniel Vetter wrote:
> Point at the equivalent atomic state and explain that atomic drivers
> shouldn't really depend upon legacy state.
> 
> Motivated by questions from Manasi about how this all is supposed to
> work.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Thanks for the patch, clarifies the usage of the atomic state
objects in place of legacy mode objects.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  6 ++++++
>  include/drm/drm_connector.h         |  9 +++++++--
>  include/drm/drm_encoder.h           |  6 +++++-
>  include/drm/drm_plane.h             | 12 ++++++++++--
>  4 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ea781d55f2c1..09512a64c336 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -907,6 +907,12 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>   *
>   * Drivers can use this for building their own atomic commit if they don't have
>   * a pure helper-based modeset implementation.
> + *
> + * Since these updates are not synchronized with lockings, only code paths
> + * called from &drm_mode_config_helper_funcs.atomic_commit_tail can look at the
> + * legacy state filled out by this helper. Defacto this means this helper and
> + * the legacy state pointers are only really useful for transitioning an
> + * existing driver to the atomic world.
>   */
>  void
>  drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b4285c40e1e4..bb6629b5c55c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -699,7 +699,6 @@ struct drm_cmdline_mode {
>   * @force: a DRM_FORCE_<foo> state for forced mode sets
>   * @override_edid: has the EDID been overwritten through debugfs for testing?
>   * @encoder_ids: valid encoders for this connector
> - * @encoder: encoder driving this connector, if any
>   * @eld: EDID-like data, if present
>   * @latency_present: AV delay info from ELD, if found
>   * @video_latency: video latency info from ELD, if found
> @@ -869,7 +868,13 @@ struct drm_connector {
>  
>  #define DRM_CONNECTOR_MAX_ENCODER 3
>  	uint32_t encoder_ids[DRM_CONNECTOR_MAX_ENCODER];
> -	struct drm_encoder *encoder; /* currently active encoder */
> +	/**
> +	 * @encoder: Currently bound encoder driving this connector, if any.
> +	 * Only really meaningful for non-atomic drivers. Atomic drivers should
> +	 * instead look at &drm_connector_state.best_encoder, and in case they
> +	 * need the CRTC driving this output, &drm_connector_state.crtc.
> +	 */
> +	struct drm_encoder *encoder;
>  
>  #define MAX_ELD_BYTES	128
>  	/* EDID bits */
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 86db0da8bdcb..67e1bccfb49d 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -88,7 +88,6 @@ struct drm_encoder_funcs {
>   * @head: list management
>   * @base: base KMS object
>   * @name: human readable name, can be overwritten by the driver
> - * @crtc: currently bound CRTC
>   * @bridge: bridge associated to the encoder
>   * @funcs: control functions
>   * @helper_private: mid-layer private data
> @@ -166,6 +165,11 @@ struct drm_encoder {
>  	 */
>  	uint32_t possible_clones;
>  
> +	/**
> +	 * @crtc: Currently bound CRTC, only really meaningful for non-atomic
> +	 * drivers.  Atomic drivers should instead check
> +	 * &drm_connector_state.crtc.
> +	 */
>  	struct drm_crtc *crtc;
>  	struct drm_bridge *bridge;
>  	const struct drm_encoder_funcs *funcs;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 069c4c8ce360..0efd38cd990b 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -474,8 +474,6 @@ enum drm_plane_type {
>   * @format_types: array of formats supported by this plane
>   * @format_count: number of formats supported
>   * @format_default: driver hasn't supplied supported formats for the plane
> - * @crtc: currently bound CRTC
> - * @fb: currently bound fb
>   * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
>   * 	drm_mode_set_config_internal() to implement correct refcounting.
>   * @funcs: helper functions
> @@ -512,7 +510,17 @@ struct drm_plane {
>  	uint64_t *modifiers;
>  	unsigned int modifier_count;
>  
> +	/**
> +	 * @crtc: Currently bound CRTC, only really meaningful for non-atomic
> +	 * drivers.  Atomic drivers should instead check &drm_plane_state.crtc.
> +	 */
>  	struct drm_crtc *crtc;
> +
> +	/**
> +	 * @fb: Currently bound framebuffer, only really meaningful for
> +	 * non-atomic drivers.  Atomic drivers should instead check
> +	 * &drm_plane_state.fb.
> +	 */
>  	struct drm_framebuffer *fb;
>  
>  	struct drm_framebuffer *old_fb;
> -- 
> 2.15.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel