Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile

[Daniel Vetter <daniel@ffwll.ch>]

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@ideasonboard.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.

> > 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?

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.

I'll apply all the other comments.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx