From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile Date: Mon, 28 Dec 2015 11:22:52 +0100 Message-ID: <20151228102251.GC29195@ulmo.nvidia.com> References: <1450286305-8980-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0518031534==" Return-path: In-Reply-To: <1450286305-8980-1-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: Daniel Vetter , Intel Graphics Development , Laurent Pinchart , DRI Development List-Id: intel-gfx@lists.freedesktop.org --===============0518031534== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="+nBD6E3TurpgldQp" Content-Disposition: inline --+nBD6E3TurpgldQp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 16, 2015 at 06:18:25PM +0100, Daniel Vetter wrote: > Another pile of vfuncs from the old gpu.tmpl xml documentation that > I've forgotten to delete. I spotted a few more things to > clarify/extend in the new kerneldoc while going through this once > more. >=20 > Cc: Laurent Pinchart > Cc: Thierry Reding > Signed-off-by: Daniel Vetter > --- > Documentation/DocBook/gpu.tmpl | 188 -------------------------= ------ > include/drm/drm_modeset_helper_vtables.h | 28 ++++- > 2 files changed, 25 insertions(+), 191 deletions(-) >=20 > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.t= mpl > index a3764291c826..c0fa21c797fe 100644 > --- a/Documentation/DocBook/gpu.tmpl > +++ b/Documentation/DocBook/gpu.tmpl > @@ -1579,194 +1579,6 @@ void intel_crt_init(struct drm_device *dev) > entities. > > > - Legacy CRTC Helper Operations > - > - > - bool (*mode_fixup)(struct drm_crtc *crtc, > - const struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode); > - > - Let CRTCs adjust the requested mode or reject it completely.= This > - operation returns true if the mode is accepted (possibly aft= er being > - adjusted) or false if it is rejected. > - > - > - The mode_fixup operation should rej= ect the > - mode if it can't reasonably use it. The definition of "reaso= nable" > - is currently fuzzy in this context. One possible behaviour w= ould be > - to set the adjusted mode to the panel timings when a fixed-m= ode > - panel is used with hardware capable of scaling. Another beha= viour > - would be to accept any input mode and adjust it to the close= st mode > - supported by the hardware (FIXME: This needs to be clarified= ). > - > - I just noticed that this deviates somewhat from what is now in the new documentation in include/drm/drm_modeset_helper_vtables.h. The new documentation suggests that ->mode_fixup() should not modify adjusted_mode but instead reject the mode if the conversion from mode to adjusted_mode can't be supported. The new definition sounds much saner to me, because if we allowed the CRTC's ->mode_fixup() to modify the adjusted_mode, we'd need to make sure that the encoder (and bridge) still accept it. And we'd need to potentially reiterate until everybody agrees. > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_m= odeset_helper_vtables.h > index 29e0dc50031d..b995d5ec50a0 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -131,6 +131,12 @@ struct drm_crtc_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > + * Also beware that the core nor helpers filters mode before passing the "... neither the core nor the helpers filter modes before passing them ..."? > + * to the driver. More specifically modes rejected by the ->mode_valid > + * callback from #drm_connector_helper_funcs can still be requested by > + * userspace and therefore also need to be rejected in either this hook, > + * or the one in #drm_encoder_helper_funcs. Hmm... that's odd. Why would one want to allow a mode that the connector can't support? Also looking at drm_helper_probe_single_connector_modes() this isn't quite true. That helper is used by all connectors except vmwgfx. It also calls drm_mode_prune_invalid(), which will remove all modes from the connector's mode list that don't have status =3D=3D MODE_OK. As far as I can see after they've been removed from the connector's mode list they will no longer be exposed to userspace. > + * > * RETURNS: > * > * True if an acceptable configuration is possible, false if the modeset > @@ -188,7 +194,9 @@ struct drm_crtc_helper_funcs { > * This callback is used by the legacy CRTC helpers to set a new > * framebuffer and scanout position. It is optional and used as an > * optimized fast-path instead of a full mode set operation with all the > - * resulting flickering. Since it can't update other planes it's > + * resulting flickering. If it is not present > + * drm_crtc_helper_set_config() will fall back to a full modeset, using > + * the ->mode_set() callbac. Since it can't update other planes it's "callback" > * incompatible with atomic modeset support. > * > * This callback is only used by the CRTC helpers and deprecated. > @@ -439,6 +447,12 @@ struct drm_encoder_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > + * Also beware that the core nor helpers filters mode before passing the > + * to the driver. More specifically modes rejected by the ->mode_valid > + * callback from #drm_connector_helper_funcs can still be requested by > + * userspace and therefore also need to be rejected in either this hook, > + * or the one in #drm_crtc_helper_funcs. Same comments as for struct drm_crtc_helper_funcs. > + * > * RETURNS: > * > * True if an acceptable configuration is possible, false if the modeset > @@ -640,8 +654,16 @@ struct drm_connector_helper_funcs { > * In this function drivers then parse the modes in the EDID and add > * them by calling drm_add_edid_modes(). But connectors that driver a > * fixed panel can also manually add specific modes using > - * drm_mode_probed_add(). Finally drivers that support audio probably > - * want to update the ELD data, too, using drm_edid_to_eld(). > + * drm_mode_probed_add(). Drivers who manually add modes should also "drivers which" or "drivers that", I think. > + * make sure that the @display_info, @width_mm and @height_mm fields of= the > + * struct #drm_connector are filled out. I think "filled in" is slightly more appropriate in this case. > + * > + * Virtual drivers who just want some standard VESA mode with a given "drivers which" or "drivers that". Thierry --+nBD6E3TurpgldQp Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWgQ14AAoJEN0jrNd/PrOh5OsP/2QKOxa2HXKend5TBqG77NRM Q1J+eg9KECguK2H/jBIonRzsEw4ifqqHoWzd54MDIRSJGBBioqkAe2wnMv6q5he9 zZ+NCNfsUChtwSFIko4kWykCcPtFx002X6gbSm92Tp5J0nDOE81p7SpCjCxY9pcL gIA57VwKg7Du/v5/a+msVddzjddF8xIYeV/9UW99vy4ZfsHFMc5B8RnIXJgBMRi1 yT2EDMeX4IImCWcEEIQ7CREkfPurS3qzb2WvprXmWLYgefwEoYJO2/m3X8apjQq4 dwbe40izoKXHXiAKsJ8c1TL8P+R1FIMmjgvzV77MQm/BwrM8bLZKMcvy0K6us1JS jdazcDmLEPm5gccU6i1KDnFYkdh9DDkvh4h2fJIAtHFPPyg8lQxiStmv269jObvy ErNliZK5j006Ke8PRW5673l0Zu3avaMmspw3kVlJr4WIDbxhNWTR1Q4Or5xskw4Q bLndrAmLrHzcFSrNWLeeUwq+lqJsE5Oga2TbrZI5gUWCSt4dfd2Eep2dU3Zhji2Y MrD17/HBXyUWC+bGeDtwRVGYI2xoGRr+BfuoCMYNYjkRNXmjQUQ2hpwOIZgP0qyW RgPVgX3roDubLsK9kqIuIeOYLWdi8eNOdCsiskxRXV4fn+z5x6R5Y2an4onvB0HF 0fdjdO3m6RzKoVrbLkIk =jt/Q -----END PGP SIGNATURE----- --+nBD6E3TurpgldQp-- --===============0518031534== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0518031534==--