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: Tue, 5 Jan 2016 16:11:29 +0100 Message-ID: <20160105151129.GB18886@ulmo.nvidia.com> References: <1450286305-8980-1-git-send-email-daniel.vetter@ffwll.ch> <20151228102251.GC29195@ulmo.nvidia.com> <20160104064941.GA8076@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1725232198==" Return-path: In-Reply-To: <20160104064941.GA8076@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Daniel Vetter , Intel Graphics Development , DRI Development , Laurent Pinchart , Daniel Vetter , Thierry Reding List-Id: intel-gfx@lists.freedesktop.org --===============1725232198== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="LpQ9ahxlCli8rRTG" Content-Disposition: inline --LpQ9ahxlCli8rRTG Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 04, 2016 at 07:49:41AM +0100, Daniel Vetter wrote: > On Mon, Dec 28, 2015 at 11:22:52AM +0100, Thierry Reding wrote: > > 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/g= pu.tmpl > > > 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 complet= ely. This > > > - operation returns true if the mode is accepted (possibly= after being > > > - adjusted) or false if it is rejected. > > > - > > > - > > > - The mode_fixup operation should= reject the > > > - mode if it can't reasonably use it. The definition of "r= easonable" > > > - is currently fuzzy in this context. One possible behavio= ur would be > > > - to set the adjusted mode to the panel timings when a fix= ed-mode > > > - panel is used with hardware capable of scaling. Another = behaviour > > > - would be to accept any input mode and adjust it to the c= losest mode > > > - supported by the hardware (FIXME: This needs to be clari= fied). > > > - > > > - > >=20 > > 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. >=20 > Yeah, I simply addressed the FIXME while typing the new docs and made this > a strong recommendation (that's why I picked "should" instead of "must"). > There's some drivers (at least i915's TV-out) where the input mode is > massively mangled, but no one will ever fix this. Okay, fine with me. > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/d= rm_modeset_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 passin= g the > >=20 > > "... neither the core nor the helpers filter modes before passing them = =2E.."? > >=20 > > > + * to the driver. More specifically modes rejected by the ->mode_va= lid > > > + * 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. > >=20 > > 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. >=20 > Maybe I need to hammer this in more, but it's a common misconception that > userspace can only ask for modes in the connector list. Generally that's > what's happening, but there's not restrictions on userspace to ask for a > mode which is _not_ in the connector's mode list. >=20 > If you don't believe that, look at xrandr --addmode and try yourself. >=20 > That's why drivers MUST filter modes in both mode_valid and mode_fixup. > Any suggestions for how to make this even more clear? Currently that's not very explicit in the text, since it only suggests that modes aren't filtered out. It doesn't say anything about the possibility of modes being manually added by userspace. How about something along these lines: While the list of modes that is advertised to userspace is filtered using the connector's ->mode_valid() callback, neither the core nor the helpers do any filtering on modes passed in from userspace when setting a mode. It is therefore possible for userspace to pass in a mode that was previously filtered out using ->mode_valid() or add a custom mode that wasn't probed from EDID or similar to begin with. Even though this is an advanced feature and rarely used nowadays, some users rely on being able to specify modes manually so drivers must be prepared to deal with it. Specifically this means that all drivers need not only validate modes in ->mode_valid() but also in ->mode_fixup() to make sure invalid modes passed in from userspace are rejected. > Aside, I tried looking into untangling this a bit with a helper to > implement mode_valid in terms of mode_fixup (by just passing a dummy > adjusted_mode in). But figuring out which encoder/crtc to take (in > general) stopped that idea. Maybe we just need to iterate over all > possible configurations. The other problem was that mode_fixup was allowed > to change software state in legacy drivers, but atomic fixed that. Yeah, it sounds fine to me to keep this as-is, as long as everybody is clear on how it works. Thierry --LpQ9ahxlCli8rRTG Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWi90fAAoJEN0jrNd/PrOhh/IP/2HsmZNia6VYLDfmZDOsSKHu 1n4GSaVq/716+fesDON+rqRD2E5xfGqrwCGOh5mxBRCG/q40yoYwwgAmvKUkZSxN H5amKpm9y5otg+WYKbouDMU533OxWr+Ii5TZgR8gk5+R5ANFD5SunlAca+AwZEf8 OIzdEFFtGkxtffptAyzwr2eT8+79qdBlx3So0/IWI1SecqB0zCFqwzY4zRwQgQ6U /6Q6svt8PNRuAvSmz/apWNXxP2Ma8ehtNXNfeLX204doQ0AnRVVvhqw0mWsMCKTQ qoN83kYOOwyloJk54p8M7BVCF9dQvlGxtj11kdw/1usuKri7fogBvqLnK3sDgoRt ZD6ea8gBTu9YypQ++kCVWymZ+lQpjjYPtdIpUxa8EP00gLZ8mWEBKWHKUtOrbkHc QzoB4LxZTdBvdiY0vlrf7SUi65PwHSxNqF3ingVuujhvCZ56B5p+/Dmy1dZWyv98 eg0JzhriYmJf7Z6N2B647bRH7yjqtCJPp8Km3kzbwJmPokdql0N2hPC0bhIpmSYx KpeL/s+JKxrBlErAJ80mk9GA6Nn3S4WKb8b713Iyp94ihp4qU9Ds7NZ9XFB79/bo YtKEukLrilX4XtDWNxFzMVwxtKmyiPnP+Oa3p44bTLPA2uybr6wgAeX11jkg7re0 fmzXXY45lFWOUGaRJSE0 =mbx4 -----END PGP SIGNATURE----- --LpQ9ahxlCli8rRTG-- --===============1725232198== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============1725232198==--