On Sun, Nov 02, 2014 at 02:19:18PM +0100, Daniel Vetter wrote:
[...]
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a68e02be7e37..9847009ad451 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -224,6 +224,25 @@ struct drm_encoder;
>  struct drm_pending_vblank_event;
>  struct drm_plane;
>  struct drm_bridge;
> +struct drm_atomic_state;
> +
> +/**
> + * struct drm_crtc_state - mutable crtc state
> + * @enable: whether the CRTC should be enabled, gates all other state

You use a mix of "crtc" and "CRTC" here. Since it's an abbreviation it
should really be "CRTC" consistently. The commit message also suffers
from this.

> + * @mode: current mode timings
> + * @event: optional pointer to a DRM event to signal upon completion of the
> + * 	state update
> + * @state: backpointer to global drm_atomic_state
> + */
> +struct drm_crtc_state {
> +	bool enable        : 1;

I thought there had been a recent thread about bitfields being
discouraged.

> @@ -288,6 +310,15 @@ struct drm_crtc_funcs {
>  
>  	int (*set_property)(struct drm_crtc *crtc,
>  			    struct drm_property *property, uint64_t val);
> +
> +	/* atomic update handling */
> +	struct drm_crtc_state *(*atomic_duplicate_state)(struct drm_crtc *crtc);
> +	void (*atomic_destroy_state)(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *cstate);

Why is the second parameter named "cstate"? Other functions use simply
"state".

> +/**
> + * struct drm_connector_state - mutable connector state
> + * @crtc: crtc to connect connector to, NULL if disabled

s/crtc/CRTC/

> + * @state: backpointer to global drm_atomic_state
> + */
> +struct drm_connector_state {
> +	struct drm_crtc *crtc;
> +
> +	struct drm_atomic_state *state;
> +};
>  
>  /**
>   * struct drm_connector_funcs - control connectors on a given device
> @@ -393,6 +437,10 @@ struct drm_crtc {
>   * @set_property: property for this connector may need an update
>   * @destroy: make object go away
>   * @force: notify the driver that the connector is forced on
> + * @atomic_duplicate_state: duplicate the atomic state for this connector
> + * @atomic_destroy_state: destroy an atomic state for this connector
> + * @atomic_set_property: set a property on an atomic state for this connector
> + *

The last line here seems gratuituous.

> +/**
> + * struct drm_plane_state - mutable plane state
> + * @crtc: currently bound CRTC, NULL if disabled
> + * @fb: currently bound fb

Nit: For consistency with the spelling for CRTC this should be FB.

> + * @crtc_x: left position of visible portion of plane on crtc
> + * @crtc_y: upper position of visible portion of plane on crtc
> + * @crtc_w: width of visible portion of plane on crtc
> + * @crtc_h: height of visible portion of plane on crtc
> + * @src_x: left position of visible portion of plane within
> + *	plane (in 16.16)
> + * @src_y: upper position of visible portion of plane within
> + *	plane (in 16.16)
> + * @src_w: width of visible portion of plane (in 16.16)
> + * @src_h: height of visible portion of plane (in 16.16)
> + * @state: backpointer to global drm_atomic_state
> + */
> +struct drm_plane_state {
> +	struct drm_crtc *crtc;
> +	struct drm_framebuffer *fb;
> +
> +	/* Signed dest location allows it to be partially off screen */
> +	int32_t crtc_x, crtc_y;
> +	uint32_t crtc_w, crtc_h;

Should these perhaps be unsized types? For the same reasons you argued
the other day.

> +
> +	/* Source values are 16.16 fixed point */
> +	uint32_t src_x, src_y;
> +	uint32_t src_h, src_w;

Do we really use the 16.16 fixed point format for these? Maybe now would
be a good time to get rid of that if we don't need it. If they're not a
16.16 fixed point format they could also be unsized.

Thierry