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.
> 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.
> + *
> * 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
