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