[Intel-gfx] [PATCH] drm/docs: more leftovers from the big vtable documentation pile

[Thierry Reding]

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.
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Cc: Thierry Reding <treding at nvidia.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > ---
> > >  Documentation/DocBook/gpu.tmpl           | 188 -------------------------------
> > >  include/drm/drm_modeset_helper_vtables.h |  28 ++++-
> > >  2 files changed, 25 insertions(+), 191 deletions(-)
> > > 
> > > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.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.
> > >      </para>
> > >      <sect2>
> > > -      <title>Legacy CRTC Helper Operations</title>
> > > -      <itemizedlist>
> > > -        <listitem id="drm-helper-crtc-mode-fixup">
> > > -          <synopsis>bool (*mode_fixup)(struct drm_crtc *crtc,
> > > -                       const struct drm_display_mode *mode,
> > > -                       struct drm_display_mode *adjusted_mode);</synopsis>
> > > -          <para>
> > > -            Let CRTCs adjust the requested mode or reject it completely. This
> > > -            operation returns true if the mode is accepted (possibly after being
> > > -            adjusted) or false if it is rejected.
> > > -          </para>
> > > -          <para>
> > > -            The <methodname>mode_fixup</methodname> operation should reject the
> > > -            mode if it can't reasonably use it. The definition of "reasonable"
> > > -            is currently fuzzy in this context. One possible behaviour would be
> > > -            to set the adjusted mode to the panel timings when a fixed-mode
> > > -            panel is used with hardware capable of scaling. Another behaviour
> > > -            would be to accept any input mode and adjust it to the closest mode
> > > -            supported by the hardware (FIXME: This needs to be clarified).
> > > -          </para>
> > > -        </listitem>
> > 
> > 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.
> 
> 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/drm_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 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 == 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.
> 
> 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.
> 
> If you don't believe that, look at xrandr --addmode and try yourself.
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20160105/9887f460/attachment.sig>