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
Attachment:
pgpJqCeTL1RQz.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
