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@xxxxxxxxxxxxxxxx>
> > Cc: Thierry Reding <treding@xxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
