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

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6527 bytes --]

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.

> 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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel