From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 05/17] drm: Add atomic driver interface definitions for objects Date: Wed, 5 Nov 2014 17:26:45 +0100 Message-ID: <20141105162644.GB25607@ulmo> References: <1414934370-11924-1-git-send-email-daniel.vetter@ffwll.ch> <1414934370-11924-6-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0482945856==" Return-path: In-Reply-To: <1414934370-11924-6-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Intel Graphics Development , DRI Development List-Id: dri-devel@lists.freedesktop.org --===============0482945856== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0eh6TmSyL6TZE2Uz" Content-Disposition: inline --0eh6TmSyL6TZE2Uz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =66rom 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 { > =20 > 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; > +}; > =20 > /** > * 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 conn= ector > + * 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 --0eh6TmSyL6TZE2Uz Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUWk/EAAoJEN0jrNd/PrOhKFEQAIL4Q8estbmoBiX3+bgTvVKK gto3l/dBkkAq86/YaR1Ds7fj2DWfffYY4JpuJJzEXr5bmPuTb+QGdbw+zTlGxGoH t/cU3HKUpqbCNrbT+bX/37qf9OQ789Wyt94hwMuYyQwDfpyI+LR6eNhs7TvJQPna ZzY3yj98j0couDJaWMhE7yTyURkAPs4C/KQfaqicLRlgLfK03Mer6mqxtDCajbEm CBrAjbsnZ+wutQ+pzBbp6OhjeKAUeCLnpJudw28ORNvSe2s4L5jcPsWAp6JVtKMt RoB6Q3C8QO0pZ/arP0hAgnRuNIC6dRnaY5aY+JFU7v1zrgi7XsAYCRkaF52+do6b ppG9ZCpAKJtt8mkOZnBWZa+LIBMKuVxDNeKGe0JCFhQKDwPpEm3MrG5ZPFpd1Cly gQPaOZk9UAjpTarOKV0uqDqEDnqBrCweuOlsJlTfCCq0e+5uqvPWzdzurKDPmguj BwwehSrEriUXRvfdQFnkPK3ZBppbqf+d2MfsVrdxNzT8+t6CRVJpMQIhE+XNmicr 3O3w+R+/c5YjoLNmm3lBKfBUy6owr7GXRHiqPEGv2Z6BCEzf13ni/vTjCQ/tW8I3 GUs1piXL1UmPMTiyDwc1KCwqcbn9zWzpP0D5siTiT47KgDzkA+VobfEoQu2kMbQo AMT7HUztdqL4e4LPhBnx =xZNo -----END PGP SIGNATURE----- --0eh6TmSyL6TZE2Uz-- --===============0482945856== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0482945856==--