From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 06/28] drm/bridge: Improve kerneldoc Date: Mon, 7 Dec 2015 12:31:48 +0100 Message-ID: <20151207113148.GI13177@ulmo> References: <1449218769-16577-1-git-send-email-daniel.vetter@ffwll.ch> <1449218769-16577-7-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1154304477==" Return-path: In-Reply-To: <1449218769-16577-7-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: intel-gfx@lists.freedesktop.org --===============1154304477== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lqaZmxkhekPBfBzr" Content-Disposition: inline --lqaZmxkhekPBfBzr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Dec 04, 2015 at 09:45:47AM +0100, Daniel Vetter wrote: > Especially document the assumptions and semantics of the callbacks > carefully. Just a warm-up excercise really. >=20 > v2: Spelling fixes (Eric). >=20 > v3: Consolidate more with existing docs: >=20 > - Remove the overview section explaining the bridge funcs, that's > now all in the drm_bridge_funcs kerneldoc in much more detail. >=20 > - Use & to reference structs so that kerneldoc automatically inserts > hyperlinks. >=20 > Cc: Eric Anholt > Cc: Archit Taneja > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_bridge.c | 69 +++++++++++------------------ > include/drm/drm_crtc.h | 102 +++++++++++++++++++++++++++++++++++++= +++--- > 2 files changed, 122 insertions(+), 49 deletions(-) >=20 > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 6b8f7211e543..26dd1cfb6078 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -31,14 +31,14 @@ > /** > * DOC: overview > * > - * drm_bridge represents a device that hangs on to an encoder. These are= handy > - * when a regular drm_encoder entity isn't enough to represent the entire > + * struct &drm_bridge represents a device that hangs on to an encoder. T= hese are > + * handy when a regular &drm_encoder entity isn't enough to represent th= e entire > * encoder chain. > * > - * A bridge is always associated to a single drm_encoder at a time, but = can be > + * A bridge is always associated to a single &drm_encoder at a time, but= can be I think I've seen either "associated with" or "attached to", but the above sounds strange to me. Anyway, it's nothing you've introduced in this patch, so might as well stay the way it is. > * either connected to it directly, or through an intermediate bridge: > * > - * encoder ---> bridge B ---> bridge A > + * encoder ---> bridge B ---> bridge A > * > * Here, the output of the encoder feeds to bridge B, and that furthers = feeds to > * bridge A. > @@ -46,11 +46,16 @@ > * The driver using the bridge is responsible to make the associations b= etween > * the encoder and bridges. Once these links are made, the bridges will > * participate along with encoder functions to perform mode_set/enable/d= isable > - * through the ops provided in drm_bridge_funcs. > + * through the ops provided in &drm_bridge_funcs. > * > * drm_bridge, like drm_panel, aren't drm_mode_object entities like plan= es, > - * crtcs, encoders or connectors. They just provide additional hooks to = get the > - * desired output at the end of the encoder chain. > + * crtcs, encoders or connectors and hence are not visible to userspace.= They s/crtcs/CRTCs/ > @@ -122,34 +127,12 @@ EXPORT_SYMBOL(drm_bridge_attach); > /** > * DOC: bridge callbacks > * > - * The drm_bridge_funcs ops are populated by the bridge driver. The drm > - * internals(atomic and crtc helpers) use the helpers defined in drm_bri= dge.c > - * These helpers call a specific drm_bridge_funcs op for all the bridges > + * The &drm_bridge_funcs ops are populated by the bridge driver. The drm s/drm/DRM/ > + * internals (atomic and crtc helpers) use the helpers defined in drm_br= idge.c s/crtc/CRTC/ > /** > @@ -159,7 +142,7 @@ EXPORT_SYMBOL(drm_bridge_attach); > * @mode: desired mode to be set for the bridge > * @adjusted_mode: updated mode that works for this bridge > * > - * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the > + * Calls 'mode_fixup' &drm_bridge_funcs op for all the bridges in the "->mode_fixup()"? > @@ -186,11 +169,11 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridg= e, > EXPORT_SYMBOL(drm_bridge_mode_fixup); > =20 > /** > - * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all > + * drm_bridge_disable - calls 'disable' &drm_bridge_funcs op for all > * bridges in the encoder chain. > * @bridge: bridge control structure > * > - * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder > + * Calls 'disable' &drm_bridge_funcs op for all the bridges in the encod= er "->disable()"? Perhaps not worth it because there's a bunch of these in this file. > struct drm_bridge_funcs { > int (*attach)(struct drm_bridge *bridge); > + > + /** > + * @mode_fixup: > + * > + * This callback is used to validate and adjust a mode. The paramater > + * mode is the display mode that should be fed to the next element in > + * the display chain, either the final &drm_connector or the next > + * &drm_bridge. The parameter adjusted_mode is the input mode the bridge > + * requires. It can be modified by this callback and does not need to > + * match mode. > + * > + * This is the only hook that allows a bridge to reject a modeset. If > + * this function passes all other callbacks must succeed for this > + * configuration. > + * > + * NOTE: > + * > + * This function is called in the check phase of atomic modesets, which > + * can be aborted for any reason (including on userspace's request to > + * just check whether a configuration would be possible). Drivers MUST > + * NOT touch any persistent state (hardware or software) or data > + * structures except the passed in adjusted_mode parameter. > + * > + * RETURNS: > + * > + * True if an acceptable configuration is possible, false if the modeset > + * operation should be rejected. > + */ > bool (*mode_fixup)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > + /** > + * @disable: > + * > + * This callback should disable the bridge. It is called right before > + * the preceding element in the display pipe is disabled. If the > + * preceding element is a bridge this means it's called before that > + * bridge's ->disaable function. If the preceding element is a s/->disaable/->disable()/? > + * &drm_encoder it's called right before the encoder's ->disable, > + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. ->disable(), ->prepare() or ->dpms()? > + * > + * The bridge can assume that the display pipe (i.e. clocks and timing > + * signals) feeding it is still running when this callback is called. > + */ > void (*disable)(struct drm_bridge *bridge); > + > + /** > + * @post_disable: > + * > + * This callback should disable the bridge. It is called right after > + * the preceding element in the display pipe is disabled. If the > + * preceding element is a bridge this means it's called after that > + * bridge's ->post_disaable function. If the preceding element is a > + * &drm_encoder it's called right after the encoder's ->disable, > + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. Same comments as for ->disable(). Perhaps this should also say what the difference is to ->disable()? But maybe that's more suitable for a follow-up patch. > + * > + * The bridge must assume that the display pipe (i.e. clocks and timing > + * singals) feeding it is no longer running when this callback is "signals". I guess this is the difference. Perhaps mention in the above paragraph that ->post_disable() is called after ->disable(), though that much should be obvious. Thierry --lqaZmxkhekPBfBzr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWZW4hAAoJEN0jrNd/PrOhZycP/Rsypt+khr1TO1gVUN0pwF13 hmCppxcsPkLaDtt9/1zuW5N3rvwwEQio5+CUCAVgVBzNZl5ShcvO+vZ8vZ/jQbvv lTbQTF5ptBxeKVwN4IItcANfS0qNmqDcsDfqN+nc4nxc3DLpxicTx2j2LqeeuI1H F8GH7JizRXSWIJ5Lf+7GCNsFDwyKPDSeE6Cs7UYooC7Z/gAgLEWG0n25HAidvCJU WNhVUR6/l0W5mLJnoa37+HB/fm7iu+z6L85RWgWDLzdDLap73Tqmo08M8dItqZBl GiZc24xpzQaORa8FK5ixYoN0/+3oBxZ/7HfKa5Mrw3d7SZeOf2HK1YsAJlC0b0Z6 oqMiwo2XoIiqN2zn4ICLM2se0gPbSQ42pmx5eD4/OTEfgY17f63QV9qKMIk3SX6k mkEh8LdQUocPe50Zo/5YwC5WYC8FOAXzfgoic3PCbGd4mcQ57Hm2frz1FxpzVNF2 rZHP40BWT75S4uoiqffsqfOpDADcmFSgXyj4onZMqHzCAs1Hh6MkNbecVetEz4rQ 1Xg/dZ7FrQQxFoGaOA5fc8nnj4S+KuE/JAcrwXDVakxPxwRdqstwYSEfHM548qoB ClyNZ+e19A3jDrnXewgSSZ54lz5jiSxaBIGnJaI7zD2mw+xcayaW8kg0Nz6Fxbux +ua4bf2qRwdJbtfHUc0l =7JC6 -----END PGP SIGNATURE----- --lqaZmxkhekPBfBzr-- --===============1154304477== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1154304477==--