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

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