* [PATCH] drm/docs: more leftovers from the big vtable documentation pile
@ 2015-12-16 17:18 Daniel Vetter
2015-12-28 10:22 ` Thierry Reding
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-12-16 17:18 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Thierry Reding,
Laurent Pinchart, Daniel Vetter
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>
- <listitem>
- <synopsis>int (*mode_set_base)(struct drm_crtc *crtc, int x, int y,
- struct drm_framebuffer *old_fb)</synopsis>
- <para>
- Move the CRTC on the current frame buffer (stored in
- <literal>crtc->fb</literal>) to position (x,y). Any of the frame
- buffer, x position or y position may have been modified.
- </para>
- <para>
- This helper operation is optional. If not provided, the
- <function>drm_crtc_helper_set_config</function> function will fall
- back to the <methodname>mode_set</methodname> helper operation.
- </para>
- <note><para>
- FIXME: Why are x and y passed as arguments, as they can be accessed
- through <literal>crtc->x</literal> and
- <literal>crtc->y</literal>?
- </para></note>
- </listitem>
- <listitem>
- <synopsis>void (*prepare)(struct drm_crtc *crtc);</synopsis>
- <para>
- Prepare the CRTC for mode setting. This operation is called after
- validating the requested mode. Drivers use it to perform
- device-specific operations required before setting the new mode.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode, int x, int y,
- struct drm_framebuffer *old_fb);</synopsis>
- <para>
- Set a new mode, position and frame buffer. Depending on the device
- requirements, the mode can be stored internally by the driver and
- applied in the <methodname>commit</methodname> operation, or
- programmed to the hardware immediately.
- </para>
- <para>
- The <methodname>mode_set</methodname> operation returns 0 on success
- or a negative error code if an error occurs.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*commit)(struct drm_crtc *crtc);</synopsis>
- <para>
- Commit a mode. This operation is called after setting the new mode.
- Upon return the device must use the new mode and be fully
- operational.
- </para>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
- <title>Encoder Helper Operations</title>
- <itemizedlist>
- <listitem>
- <synopsis>bool (*mode_fixup)(struct drm_encoder *encoder,
- const struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);</synopsis>
- <para>
- Let encoders 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. See the
- <link linkend="drm-helper-crtc-mode-fixup">mode_fixup CRTC helper
- operation</link> for an explanation of the allowed adjustments.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*prepare)(struct drm_encoder *encoder);</synopsis>
- <para>
- Prepare the encoder for mode setting. This operation is called after
- validating the requested mode. Drivers use it to perform
- device-specific operations required before setting the new mode.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*mode_set)(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);</synopsis>
- <para>
- Set a new mode. Depending on the device requirements, the mode can
- be stored internally by the driver and applied in the
- <methodname>commit</methodname> operation, or programmed to the
- hardware immediately.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*commit)(struct drm_encoder *encoder);</synopsis>
- <para>
- Commit a mode. This operation is called after setting the new mode.
- Upon return the device must use the new mode and be fully
- operational.
- </para>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
- <title>Connector Helper Operations</title>
- <itemizedlist>
- <listitem>
- <synopsis>struct drm_encoder *(*best_encoder)(struct drm_connector *connector);</synopsis>
- <para>
- Return a pointer to the best encoder for the connecter. Device that
- map connectors to encoders 1:1 simply return the pointer to the
- associated encoder. This operation is mandatory.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*get_modes)(struct drm_connector *connector);</synopsis>
- <para>
- Fill the connector's <structfield>probed_modes</structfield> list
- by parsing EDID data with <function>drm_add_edid_modes</function>,
- adding standard VESA DMT modes with <function>drm_add_modes_noedid</function>,
- or calling <function>drm_mode_probed_add</function> directly for every
- supported mode and return the number of modes it has detected. This
- operation is mandatory.
- </para>
- <para>
- Note that the caller function will automatically add standard VESA
- DMT modes up to 1024x768 if the <methodname>get_modes</methodname>
- helper operation returns no mode and if the connector status is
- connector_status_connected. There is no need to call
- <function>drm_add_edid_modes</function> manually in that case.
- </para>
- <para>
- The <structfield>vrefresh</structfield> value is computed by
- <function>drm_helper_probe_single_connector_modes</function>.
- </para>
- <para>
- When parsing EDID data, <function>drm_add_edid_modes</function> fills the
- connector <structfield>display_info</structfield>
- <structfield>width_mm</structfield> and
- <structfield>height_mm</structfield> fields. When creating modes
- manually the <methodname>get_modes</methodname> helper operation must
- set the <structfield>display_info</structfield>
- <structfield>width_mm</structfield> and
- <structfield>height_mm</structfield> fields if they haven't been set
- already (for instance at initialization time when a fixed-size panel is
- attached to the connector). The mode <structfield>width_mm</structfield>
- and <structfield>height_mm</structfield> fields are only used internally
- during EDID parsing and should not be set when creating modes manually.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*mode_valid)(struct drm_connector *connector,
- struct drm_display_mode *mode);</synopsis>
- <para>
- Verify whether a mode is valid for the connector. Return MODE_OK for
- supported modes and one of the enum drm_mode_status values (MODE_*)
- for unsupported modes. This operation is optional.
- </para>
- <para>
- As the mode rejection reason is currently not used beside for
- immediately removing the unsupported mode, an implementation can
- return MODE_BAD regardless of the exact reason why the mode is not
- valid.
- </para>
- <note><para>
- Note that the <methodname>mode_valid</methodname> helper operation is
- only called for modes detected by the device, and
- <emphasis>not</emphasis> for modes set by the user through the CRTC
- <methodname>set_config</methodname> operation.
- </para></note>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
<title>Atomic Modeset Helper Functions Reference</title>
<sect3>
<title>Overview</title>
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
+ * 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.
+ *
* 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
* 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.
+ *
* 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
+ * make sure that the @display_info, @width_mm and @height_mm fields of the
+ * struct #drm_connector are filled out.
+ *
+ * Virtual drivers who just want some standard VESA mode with a given
+ * resolution can call drm_add_modes_noedid(), and mark the preferred
+ * one using drm_set_preferred_mode().
+ *
+ * Finally drivers that support audio probably want to update the ELD
+ * data, too, using drm_edid_to_eld().
*
* This function is only called after the ->detect() hook has indicated
* that a sink is connected and when the EDID isn't overridden through
--
2.6.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2015-12-16 17:18 [PATCH] drm/docs: more leftovers from the big vtable documentation pile Daniel Vetter
@ 2015-12-28 10:22 ` Thierry Reding
2016-01-04 6:49 ` Daniel Vetter
2016-01-04 6:53 ` Daniel Vetter
2016-01-04 7:20 ` ✗ warning: Fi.CI.BAT Patchwork
2 siblings, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2015-12-28 10:22 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Laurent Pinchart,
DRI Development
[-- 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2015-12-28 10:22 ` Thierry Reding
@ 2016-01-04 6:49 ` Daniel Vetter
2016-01-05 15:11 ` Thierry Reding
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2016-01-04 6:49 UTC (permalink / raw)
To: Thierry Reding
Cc: Daniel Vetter, Intel Graphics Development, Laurent Pinchart,
DRI Development, Daniel Vetter
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2015-12-16 17:18 [PATCH] drm/docs: more leftovers from the big vtable documentation pile Daniel Vetter
2015-12-28 10:22 ` Thierry Reding
@ 2016-01-04 6:53 ` Daniel Vetter
2016-01-05 14:48 ` Thierry Reding
2016-01-05 15:22 ` Daniel Vetter
2016-01-04 7:20 ` ✗ warning: Fi.CI.BAT Patchwork
2 siblings, 2 replies; 9+ messages in thread
From: Daniel Vetter @ 2016-01-04 6:53 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Thierry Reding,
Laurent Pinchart, Daniel Vetter
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.
v2: Spelling fixes (Thierry).
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>
- <listitem>
- <synopsis>int (*mode_set_base)(struct drm_crtc *crtc, int x, int y,
- struct drm_framebuffer *old_fb)</synopsis>
- <para>
- Move the CRTC on the current frame buffer (stored in
- <literal>crtc->fb</literal>) to position (x,y). Any of the frame
- buffer, x position or y position may have been modified.
- </para>
- <para>
- This helper operation is optional. If not provided, the
- <function>drm_crtc_helper_set_config</function> function will fall
- back to the <methodname>mode_set</methodname> helper operation.
- </para>
- <note><para>
- FIXME: Why are x and y passed as arguments, as they can be accessed
- through <literal>crtc->x</literal> and
- <literal>crtc->y</literal>?
- </para></note>
- </listitem>
- <listitem>
- <synopsis>void (*prepare)(struct drm_crtc *crtc);</synopsis>
- <para>
- Prepare the CRTC for mode setting. This operation is called after
- validating the requested mode. Drivers use it to perform
- device-specific operations required before setting the new mode.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode, int x, int y,
- struct drm_framebuffer *old_fb);</synopsis>
- <para>
- Set a new mode, position and frame buffer. Depending on the device
- requirements, the mode can be stored internally by the driver and
- applied in the <methodname>commit</methodname> operation, or
- programmed to the hardware immediately.
- </para>
- <para>
- The <methodname>mode_set</methodname> operation returns 0 on success
- or a negative error code if an error occurs.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*commit)(struct drm_crtc *crtc);</synopsis>
- <para>
- Commit a mode. This operation is called after setting the new mode.
- Upon return the device must use the new mode and be fully
- operational.
- </para>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
- <title>Encoder Helper Operations</title>
- <itemizedlist>
- <listitem>
- <synopsis>bool (*mode_fixup)(struct drm_encoder *encoder,
- const struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);</synopsis>
- <para>
- Let encoders 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. See the
- <link linkend="drm-helper-crtc-mode-fixup">mode_fixup CRTC helper
- operation</link> for an explanation of the allowed adjustments.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*prepare)(struct drm_encoder *encoder);</synopsis>
- <para>
- Prepare the encoder for mode setting. This operation is called after
- validating the requested mode. Drivers use it to perform
- device-specific operations required before setting the new mode.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*mode_set)(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);</synopsis>
- <para>
- Set a new mode. Depending on the device requirements, the mode can
- be stored internally by the driver and applied in the
- <methodname>commit</methodname> operation, or programmed to the
- hardware immediately.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*commit)(struct drm_encoder *encoder);</synopsis>
- <para>
- Commit a mode. This operation is called after setting the new mode.
- Upon return the device must use the new mode and be fully
- operational.
- </para>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
- <title>Connector Helper Operations</title>
- <itemizedlist>
- <listitem>
- <synopsis>struct drm_encoder *(*best_encoder)(struct drm_connector *connector);</synopsis>
- <para>
- Return a pointer to the best encoder for the connecter. Device that
- map connectors to encoders 1:1 simply return the pointer to the
- associated encoder. This operation is mandatory.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*get_modes)(struct drm_connector *connector);</synopsis>
- <para>
- Fill the connector's <structfield>probed_modes</structfield> list
- by parsing EDID data with <function>drm_add_edid_modes</function>,
- adding standard VESA DMT modes with <function>drm_add_modes_noedid</function>,
- or calling <function>drm_mode_probed_add</function> directly for every
- supported mode and return the number of modes it has detected. This
- operation is mandatory.
- </para>
- <para>
- Note that the caller function will automatically add standard VESA
- DMT modes up to 1024x768 if the <methodname>get_modes</methodname>
- helper operation returns no mode and if the connector status is
- connector_status_connected. There is no need to call
- <function>drm_add_edid_modes</function> manually in that case.
- </para>
- <para>
- The <structfield>vrefresh</structfield> value is computed by
- <function>drm_helper_probe_single_connector_modes</function>.
- </para>
- <para>
- When parsing EDID data, <function>drm_add_edid_modes</function> fills the
- connector <structfield>display_info</structfield>
- <structfield>width_mm</structfield> and
- <structfield>height_mm</structfield> fields. When creating modes
- manually the <methodname>get_modes</methodname> helper operation must
- set the <structfield>display_info</structfield>
- <structfield>width_mm</structfield> and
- <structfield>height_mm</structfield> fields if they haven't been set
- already (for instance at initialization time when a fixed-size panel is
- attached to the connector). The mode <structfield>width_mm</structfield>
- and <structfield>height_mm</structfield> fields are only used internally
- during EDID parsing and should not be set when creating modes manually.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*mode_valid)(struct drm_connector *connector,
- struct drm_display_mode *mode);</synopsis>
- <para>
- Verify whether a mode is valid for the connector. Return MODE_OK for
- supported modes and one of the enum drm_mode_status values (MODE_*)
- for unsupported modes. This operation is optional.
- </para>
- <para>
- As the mode rejection reason is currently not used beside for
- immediately removing the unsupported mode, an implementation can
- return MODE_BAD regardless of the exact reason why the mode is not
- valid.
- </para>
- <note><para>
- Note that the <methodname>mode_valid</methodname> helper operation is
- only called for modes detected by the device, and
- <emphasis>not</emphasis> for modes set by the user through the CRTC
- <methodname>set_config</methodname> operation.
- </para></note>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
<title>Atomic Modeset Helper Functions Reference</title>
<sect3>
<title>Overview</title>
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 29e0dc50031d..ce5cbf63b5cf 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 neither core nor helpers filter modes 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_encoder_helper_funcs.
+ *
* 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
* 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 neither core nor helpers filter modes 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.
+ *
* 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 which manually add modes should also
+ * make sure that the @display_info, @width_mm and @height_mm fields of the
+ * struct #drm_connector are filled in.
+ *
+ * Virtual drivers that just want some standard VESA mode with a given
+ * resolution can call drm_add_modes_noedid(), and mark the preferred
+ * one using drm_set_preferred_mode().
+ *
+ * Finally drivers that support audio probably want to update the ELD
+ * data, too, using drm_edid_to_eld().
*
* This function is only called after the ->detect() hook has indicated
* that a sink is connected and when the EDID isn't overridden through
--
2.6.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ warning: Fi.CI.BAT
2015-12-16 17:18 [PATCH] drm/docs: more leftovers from the big vtable documentation pile Daniel Vetter
2015-12-28 10:22 ` Thierry Reding
2016-01-04 6:53 ` Daniel Vetter
@ 2016-01-04 7:20 ` Patchwork
2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2016-01-04 7:20 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Summary ==
Built on 79686f613b3955a4ed09cee936e7f70ec4e61b67 drm-intel-nightly: 2015y-12m-30d-11h-59m-54s UTC integration manifest
Test gem_storedw_loop:
Subgroup basic-render:
dmesg-warn -> PASS (skl-i7k-2)
Test kms_flip:
Subgroup basic-flip-vs-dpms:
dmesg-warn -> PASS (ilk-hp8440p)
Subgroup basic-flip-vs-modeset:
dmesg-warn -> PASS (bsw-nuc-2)
pass -> DMESG-WARN (snb-x220t)
dmesg-warn -> PASS (hsw-xps12)
dmesg-warn -> PASS (hsw-brixbox)
dmesg-warn -> PASS (bdw-nuci7)
dmesg-warn -> PASS (ilk-hp8440p)
Subgroup basic-flip-vs-wf_vblank:
dmesg-warn -> PASS (ivb-t430s)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (ivb-t430s)
Subgroup read-crc-pipe-a:
dmesg-warn -> PASS (snb-x220t)
dmesg-warn -> PASS (byt-nuc)
Test kms_setmode:
Subgroup basic-clone-single-crtc:
dmesg-warn -> PASS (snb-dellxps)
bdw-nuci7 total:132 pass:122 dwarn:1 dfail:0 fail:0 skip:9
bdw-ultra total:132 pass:124 dwarn:2 dfail:0 fail:0 skip:6
bsw-nuc-2 total:135 pass:114 dwarn:1 dfail:0 fail:0 skip:20
byt-nuc total:135 pass:120 dwarn:2 dfail:0 fail:0 skip:13
hsw-brixbox total:135 pass:127 dwarn:1 dfail:0 fail:0 skip:7
hsw-gt2 total:135 pass:130 dwarn:1 dfail:0 fail:0 skip:4
hsw-xps12 total:132 pass:126 dwarn:2 dfail:0 fail:0 skip:4
ilk-hp8440p total:135 pass:100 dwarn:0 dfail:0 fail:0 skip:35
ivb-t430s total:135 pass:127 dwarn:2 dfail:0 fail:0 skip:6
skl-i5k-2 total:135 pass:123 dwarn:4 dfail:0 fail:0 skip:8
skl-i7k-2 total:135 pass:125 dwarn:2 dfail:0 fail:0 skip:8
snb-dellxps total:135 pass:122 dwarn:1 dfail:0 fail:0 skip:12
snb-x220t total:135 pass:121 dwarn:2 dfail:0 fail:1 skip:11
Results at /archive/results/CI_IGT_test/Patchwork_1057/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2016-01-04 6:53 ` Daniel Vetter
@ 2016-01-05 14:48 ` Thierry Reding
2016-01-05 15:22 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2016-01-05 14:48 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Thierry Reding,
Laurent Pinchart, DRI Development
[-- Attachment #1.1: Type: text/plain, Size: 2388 bytes --]
On Mon, Jan 04, 2016 at 07:53:36AM +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.
>
> v2: Spelling fixes (Thierry).
Found a couple more.
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 29e0dc50031d..ce5cbf63b5cf 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 neither core nor helpers filter modes before
I'd drop the "the" above: "... that neither core nor helpers..."
> + * passing the to the driver. More specifically modes rejected by the
"passing them"?
> + * ->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.
> + *
> * 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 neither core nor helpers filter modes before
> + * passing the to the driver. More specifically modes rejected by the
Same as above.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2016-01-04 6:49 ` Daniel Vetter
@ 2016-01-05 15:11 ` Thierry Reding
0 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2016-01-05 15:11 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Laurent Pinchart, Daniel Vetter, Thierry Reding
[-- Attachment #1.1: Type: text/plain, Size: 6992 bytes --]
On Mon, Jan 04, 2016 at 07:49:41AM +0100, Daniel Vetter wrote:
> 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.
Okay, fine with me.
> > > 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?
Currently that's not very explicit in the text, since it only suggests
that modes aren't filtered out. It doesn't say anything about the
possibility of modes being manually added by userspace. How about
something along these lines:
While the list of modes that is advertised to userspace is filtered
using the connector's ->mode_valid() callback, neither the core nor
the helpers do any filtering on modes passed in from userspace when
setting a mode. It is therefore possible for userspace to pass in a
mode that was previously filtered out using ->mode_valid() or add a
custom mode that wasn't probed from EDID or similar to begin with.
Even though this is an advanced feature and rarely used nowadays,
some users rely on being able to specify modes manually so drivers
must be prepared to deal with it. Specifically this means that all
drivers need not only validate modes in ->mode_valid() but also in
->mode_fixup() to make sure invalid modes passed in from userspace
are rejected.
> 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.
Yeah, it sounds fine to me to keep this as-is, as long as everybody is
clear on how it works.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2016-01-04 6:53 ` Daniel Vetter
2016-01-05 14:48 ` Thierry Reding
@ 2016-01-05 15:22 ` Daniel Vetter
2016-01-05 15:30 ` Daniel Vetter
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2016-01-05 15:22 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Thierry Reding,
Laurent Pinchart, Daniel Vetter
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.
v2: Spelling fixes (Thierry).
v3: More spelling fixes and use Thierry's proposal to clarify why
drivers need to validate modes both in ->mode_fixup and ->mode_valid.
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Thierry Reding <treding@nvidia.com>
Acked-by: 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 | 44 +++++++-
2 files changed, 41 insertions(+), 191 deletions(-)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 225a246c5f53..faa5e0d4208d 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>
- <listitem>
- <synopsis>int (*mode_set_base)(struct drm_crtc *crtc, int x, int y,
- struct drm_framebuffer *old_fb)</synopsis>
- <para>
- Move the CRTC on the current frame buffer (stored in
- <literal>crtc->fb</literal>) to position (x,y). Any of the frame
- buffer, x position or y position may have been modified.
- </para>
- <para>
- This helper operation is optional. If not provided, the
- <function>drm_crtc_helper_set_config</function> function will fall
- back to the <methodname>mode_set</methodname> helper operation.
- </para>
- <note><para>
- FIXME: Why are x and y passed as arguments, as they can be accessed
- through <literal>crtc->x</literal> and
- <literal>crtc->y</literal>?
- </para></note>
- </listitem>
- <listitem>
- <synopsis>void (*prepare)(struct drm_crtc *crtc);</synopsis>
- <para>
- Prepare the CRTC for mode setting. This operation is called after
- validating the requested mode. Drivers use it to perform
- device-specific operations required before setting the new mode.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode, int x, int y,
- struct drm_framebuffer *old_fb);</synopsis>
- <para>
- Set a new mode, position and frame buffer. Depending on the device
- requirements, the mode can be stored internally by the driver and
- applied in the <methodname>commit</methodname> operation, or
- programmed to the hardware immediately.
- </para>
- <para>
- The <methodname>mode_set</methodname> operation returns 0 on success
- or a negative error code if an error occurs.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*commit)(struct drm_crtc *crtc);</synopsis>
- <para>
- Commit a mode. This operation is called after setting the new mode.
- Upon return the device must use the new mode and be fully
- operational.
- </para>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
- <title>Encoder Helper Operations</title>
- <itemizedlist>
- <listitem>
- <synopsis>bool (*mode_fixup)(struct drm_encoder *encoder,
- const struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);</synopsis>
- <para>
- Let encoders 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. See the
- <link linkend="drm-helper-crtc-mode-fixup">mode_fixup CRTC helper
- operation</link> for an explanation of the allowed adjustments.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*prepare)(struct drm_encoder *encoder);</synopsis>
- <para>
- Prepare the encoder for mode setting. This operation is called after
- validating the requested mode. Drivers use it to perform
- device-specific operations required before setting the new mode.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*mode_set)(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);</synopsis>
- <para>
- Set a new mode. Depending on the device requirements, the mode can
- be stored internally by the driver and applied in the
- <methodname>commit</methodname> operation, or programmed to the
- hardware immediately.
- </para>
- </listitem>
- <listitem>
- <synopsis>void (*commit)(struct drm_encoder *encoder);</synopsis>
- <para>
- Commit a mode. This operation is called after setting the new mode.
- Upon return the device must use the new mode and be fully
- operational.
- </para>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
- <title>Connector Helper Operations</title>
- <itemizedlist>
- <listitem>
- <synopsis>struct drm_encoder *(*best_encoder)(struct drm_connector *connector);</synopsis>
- <para>
- Return a pointer to the best encoder for the connecter. Device that
- map connectors to encoders 1:1 simply return the pointer to the
- associated encoder. This operation is mandatory.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*get_modes)(struct drm_connector *connector);</synopsis>
- <para>
- Fill the connector's <structfield>probed_modes</structfield> list
- by parsing EDID data with <function>drm_add_edid_modes</function>,
- adding standard VESA DMT modes with <function>drm_add_modes_noedid</function>,
- or calling <function>drm_mode_probed_add</function> directly for every
- supported mode and return the number of modes it has detected. This
- operation is mandatory.
- </para>
- <para>
- Note that the caller function will automatically add standard VESA
- DMT modes up to 1024x768 if the <methodname>get_modes</methodname>
- helper operation returns no mode and if the connector status is
- connector_status_connected. There is no need to call
- <function>drm_add_edid_modes</function> manually in that case.
- </para>
- <para>
- The <structfield>vrefresh</structfield> value is computed by
- <function>drm_helper_probe_single_connector_modes</function>.
- </para>
- <para>
- When parsing EDID data, <function>drm_add_edid_modes</function> fills the
- connector <structfield>display_info</structfield>
- <structfield>width_mm</structfield> and
- <structfield>height_mm</structfield> fields. When creating modes
- manually the <methodname>get_modes</methodname> helper operation must
- set the <structfield>display_info</structfield>
- <structfield>width_mm</structfield> and
- <structfield>height_mm</structfield> fields if they haven't been set
- already (for instance at initialization time when a fixed-size panel is
- attached to the connector). The mode <structfield>width_mm</structfield>
- and <structfield>height_mm</structfield> fields are only used internally
- during EDID parsing and should not be set when creating modes manually.
- </para>
- </listitem>
- <listitem>
- <synopsis>int (*mode_valid)(struct drm_connector *connector,
- struct drm_display_mode *mode);</synopsis>
- <para>
- Verify whether a mode is valid for the connector. Return MODE_OK for
- supported modes and one of the enum drm_mode_status values (MODE_*)
- for unsupported modes. This operation is optional.
- </para>
- <para>
- As the mode rejection reason is currently not used beside for
- immediately removing the unsupported mode, an implementation can
- return MODE_BAD regardless of the exact reason why the mode is not
- valid.
- </para>
- <note><para>
- Note that the <methodname>mode_valid</methodname> helper operation is
- only called for modes detected by the device, and
- <emphasis>not</emphasis> for modes set by the user through the CRTC
- <methodname>set_config</methodname> operation.
- </para></note>
- </listitem>
- </itemizedlist>
- </sect2>
- <sect2>
<title>Atomic Modeset Helper Functions Reference</title>
<sect3>
<title>Overview</title>
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 29e0dc50031d..a126a0d7aed4 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -131,6 +131,20 @@ 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 neither core nor helpers filter modes before
+ * passing them to the driver: While the list of modes that is
+ * advertised to userspace is filtered using the connector's
+ * ->mode_valid() callback, neither the core nor the helpers do any
+ * filtering on modes passed in from userspace when setting a mode. It
+ * is therefore possible for userspace to pass in a mode that was
+ * previously filtered out using ->mode_valid() or add a custom mode
+ * that wasn't probed from EDID or similar to begin with. Even though
+ * this is an advanced feature and rarely used nowadays, some users rely
+ * on being able to specify modes manually so drivers must be prepared
+ * to deal with it. Specifically this means that all drivers need not
+ * only validate modes in ->mode_valid() but also in ->mode_fixup() to
+ * make sure invalid modes passed in from userspace are rejected.
+ *
* RETURNS:
*
* True if an acceptable configuration is possible, false if the modeset
@@ -188,7 +202,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() callback. Since it can't update other planes it's
* incompatible with atomic modeset support.
*
* This callback is only used by the CRTC helpers and deprecated.
@@ -439,6 +455,20 @@ 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 neither core nor helpers filter modes before
+ * passing them to the driver: While the list of modes that is
+ * advertised to userspace is filtered using the connector's
+ * ->mode_valid() callback, neither the core nor the helpers do any
+ * filtering on modes passed in from userspace when setting a mode. It
+ * is therefore possible for userspace to pass in a mode that was
+ * previously filtered out using ->mode_valid() or add a custom mode
+ * that wasn't probed from EDID or similar to begin with. Even though
+ * this is an advanced feature and rarely used nowadays, some users rely
+ * on being able to specify modes manually so drivers must be prepared
+ * to deal with it. Specifically this means that all drivers need not
+ * only validate modes in ->mode_valid() but also in ->mode_fixup() to
+ * make sure invalid modes passed in from userspace are rejected.
+ *
* RETURNS:
*
* True if an acceptable configuration is possible, false if the modeset
@@ -640,8 +670,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 which manually add modes should also
+ * make sure that the @display_info, @width_mm and @height_mm fields of the
+ * struct #drm_connector are filled in.
+ *
+ * Virtual drivers that just want some standard VESA mode with a given
+ * resolution can call drm_add_modes_noedid(), and mark the preferred
+ * one using drm_set_preferred_mode().
+ *
+ * Finally drivers that support audio probably want to update the ELD
+ * data, too, using drm_edid_to_eld().
*
* This function is only called after the ->detect() hook has indicated
* that a sink is connected and when the EDID isn't overridden through
--
2.6.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/docs: more leftovers from the big vtable documentation pile
2016-01-05 15:22 ` Daniel Vetter
@ 2016-01-05 15:30 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2016-01-05 15:30 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Thierry Reding,
Laurent Pinchart, Daniel Vetter
On Tue, Jan 05, 2016 at 04:22:15PM +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.
>
> v2: Spelling fixes (Thierry).
>
> v3: More spelling fixes and use Thierry's proposal to clarify why
> drivers need to validate modes both in ->mode_fixup and ->mode_valid.
>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
... and pulled into drm-misc.
-Daniel
> ---
> Documentation/DocBook/gpu.tmpl | 188 -------------------------------
> include/drm/drm_modeset_helper_vtables.h | 44 +++++++-
> 2 files changed, 41 insertions(+), 191 deletions(-)
>
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 225a246c5f53..faa5e0d4208d 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>
> - <listitem>
> - <synopsis>int (*mode_set_base)(struct drm_crtc *crtc, int x, int y,
> - struct drm_framebuffer *old_fb)</synopsis>
> - <para>
> - Move the CRTC on the current frame buffer (stored in
> - <literal>crtc->fb</literal>) to position (x,y). Any of the frame
> - buffer, x position or y position may have been modified.
> - </para>
> - <para>
> - This helper operation is optional. If not provided, the
> - <function>drm_crtc_helper_set_config</function> function will fall
> - back to the <methodname>mode_set</methodname> helper operation.
> - </para>
> - <note><para>
> - FIXME: Why are x and y passed as arguments, as they can be accessed
> - through <literal>crtc->x</literal> and
> - <literal>crtc->y</literal>?
> - </para></note>
> - </listitem>
> - <listitem>
> - <synopsis>void (*prepare)(struct drm_crtc *crtc);</synopsis>
> - <para>
> - Prepare the CRTC for mode setting. This operation is called after
> - validating the requested mode. Drivers use it to perform
> - device-specific operations required before setting the new mode.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>int (*mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode, int x, int y,
> - struct drm_framebuffer *old_fb);</synopsis>
> - <para>
> - Set a new mode, position and frame buffer. Depending on the device
> - requirements, the mode can be stored internally by the driver and
> - applied in the <methodname>commit</methodname> operation, or
> - programmed to the hardware immediately.
> - </para>
> - <para>
> - The <methodname>mode_set</methodname> operation returns 0 on success
> - or a negative error code if an error occurs.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>void (*commit)(struct drm_crtc *crtc);</synopsis>
> - <para>
> - Commit a mode. This operation is called after setting the new mode.
> - Upon return the device must use the new mode and be fully
> - operational.
> - </para>
> - </listitem>
> - </itemizedlist>
> - </sect2>
> - <sect2>
> - <title>Encoder Helper Operations</title>
> - <itemizedlist>
> - <listitem>
> - <synopsis>bool (*mode_fixup)(struct drm_encoder *encoder,
> - const struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode);</synopsis>
> - <para>
> - Let encoders 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. See the
> - <link linkend="drm-helper-crtc-mode-fixup">mode_fixup CRTC helper
> - operation</link> for an explanation of the allowed adjustments.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>void (*prepare)(struct drm_encoder *encoder);</synopsis>
> - <para>
> - Prepare the encoder for mode setting. This operation is called after
> - validating the requested mode. Drivers use it to perform
> - device-specific operations required before setting the new mode.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>void (*mode_set)(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode);</synopsis>
> - <para>
> - Set a new mode. Depending on the device requirements, the mode can
> - be stored internally by the driver and applied in the
> - <methodname>commit</methodname> operation, or programmed to the
> - hardware immediately.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>void (*commit)(struct drm_encoder *encoder);</synopsis>
> - <para>
> - Commit a mode. This operation is called after setting the new mode.
> - Upon return the device must use the new mode and be fully
> - operational.
> - </para>
> - </listitem>
> - </itemizedlist>
> - </sect2>
> - <sect2>
> - <title>Connector Helper Operations</title>
> - <itemizedlist>
> - <listitem>
> - <synopsis>struct drm_encoder *(*best_encoder)(struct drm_connector *connector);</synopsis>
> - <para>
> - Return a pointer to the best encoder for the connecter. Device that
> - map connectors to encoders 1:1 simply return the pointer to the
> - associated encoder. This operation is mandatory.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>int (*get_modes)(struct drm_connector *connector);</synopsis>
> - <para>
> - Fill the connector's <structfield>probed_modes</structfield> list
> - by parsing EDID data with <function>drm_add_edid_modes</function>,
> - adding standard VESA DMT modes with <function>drm_add_modes_noedid</function>,
> - or calling <function>drm_mode_probed_add</function> directly for every
> - supported mode and return the number of modes it has detected. This
> - operation is mandatory.
> - </para>
> - <para>
> - Note that the caller function will automatically add standard VESA
> - DMT modes up to 1024x768 if the <methodname>get_modes</methodname>
> - helper operation returns no mode and if the connector status is
> - connector_status_connected. There is no need to call
> - <function>drm_add_edid_modes</function> manually in that case.
> - </para>
> - <para>
> - The <structfield>vrefresh</structfield> value is computed by
> - <function>drm_helper_probe_single_connector_modes</function>.
> - </para>
> - <para>
> - When parsing EDID data, <function>drm_add_edid_modes</function> fills the
> - connector <structfield>display_info</structfield>
> - <structfield>width_mm</structfield> and
> - <structfield>height_mm</structfield> fields. When creating modes
> - manually the <methodname>get_modes</methodname> helper operation must
> - set the <structfield>display_info</structfield>
> - <structfield>width_mm</structfield> and
> - <structfield>height_mm</structfield> fields if they haven't been set
> - already (for instance at initialization time when a fixed-size panel is
> - attached to the connector). The mode <structfield>width_mm</structfield>
> - and <structfield>height_mm</structfield> fields are only used internally
> - during EDID parsing and should not be set when creating modes manually.
> - </para>
> - </listitem>
> - <listitem>
> - <synopsis>int (*mode_valid)(struct drm_connector *connector,
> - struct drm_display_mode *mode);</synopsis>
> - <para>
> - Verify whether a mode is valid for the connector. Return MODE_OK for
> - supported modes and one of the enum drm_mode_status values (MODE_*)
> - for unsupported modes. This operation is optional.
> - </para>
> - <para>
> - As the mode rejection reason is currently not used beside for
> - immediately removing the unsupported mode, an implementation can
> - return MODE_BAD regardless of the exact reason why the mode is not
> - valid.
> - </para>
> - <note><para>
> - Note that the <methodname>mode_valid</methodname> helper operation is
> - only called for modes detected by the device, and
> - <emphasis>not</emphasis> for modes set by the user through the CRTC
> - <methodname>set_config</methodname> operation.
> - </para></note>
> - </listitem>
> - </itemizedlist>
> - </sect2>
> - <sect2>
> <title>Atomic Modeset Helper Functions Reference</title>
> <sect3>
> <title>Overview</title>
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 29e0dc50031d..a126a0d7aed4 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -131,6 +131,20 @@ 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 neither core nor helpers filter modes before
> + * passing them to the driver: While the list of modes that is
> + * advertised to userspace is filtered using the connector's
> + * ->mode_valid() callback, neither the core nor the helpers do any
> + * filtering on modes passed in from userspace when setting a mode. It
> + * is therefore possible for userspace to pass in a mode that was
> + * previously filtered out using ->mode_valid() or add a custom mode
> + * that wasn't probed from EDID or similar to begin with. Even though
> + * this is an advanced feature and rarely used nowadays, some users rely
> + * on being able to specify modes manually so drivers must be prepared
> + * to deal with it. Specifically this means that all drivers need not
> + * only validate modes in ->mode_valid() but also in ->mode_fixup() to
> + * make sure invalid modes passed in from userspace are rejected.
> + *
> * RETURNS:
> *
> * True if an acceptable configuration is possible, false if the modeset
> @@ -188,7 +202,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() callback. Since it can't update other planes it's
> * incompatible with atomic modeset support.
> *
> * This callback is only used by the CRTC helpers and deprecated.
> @@ -439,6 +455,20 @@ 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 neither core nor helpers filter modes before
> + * passing them to the driver: While the list of modes that is
> + * advertised to userspace is filtered using the connector's
> + * ->mode_valid() callback, neither the core nor the helpers do any
> + * filtering on modes passed in from userspace when setting a mode. It
> + * is therefore possible for userspace to pass in a mode that was
> + * previously filtered out using ->mode_valid() or add a custom mode
> + * that wasn't probed from EDID or similar to begin with. Even though
> + * this is an advanced feature and rarely used nowadays, some users rely
> + * on being able to specify modes manually so drivers must be prepared
> + * to deal with it. Specifically this means that all drivers need not
> + * only validate modes in ->mode_valid() but also in ->mode_fixup() to
> + * make sure invalid modes passed in from userspace are rejected.
> + *
> * RETURNS:
> *
> * True if an acceptable configuration is possible, false if the modeset
> @@ -640,8 +670,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 which manually add modes should also
> + * make sure that the @display_info, @width_mm and @height_mm fields of the
> + * struct #drm_connector are filled in.
> + *
> + * Virtual drivers that just want some standard VESA mode with a given
> + * resolution can call drm_add_modes_noedid(), and mark the preferred
> + * one using drm_set_preferred_mode().
> + *
> + * Finally drivers that support audio probably want to update the ELD
> + * data, too, using drm_edid_to_eld().
> *
> * This function is only called after the ->detect() hook has indicated
> * that a sink is connected and when the EDID isn't overridden through
> --
> 2.6.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-01-05 15:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 17:18 [PATCH] drm/docs: more leftovers from the big vtable documentation pile Daniel Vetter
2015-12-28 10:22 ` Thierry Reding
2016-01-04 6:49 ` Daniel Vetter
2016-01-05 15:11 ` Thierry Reding
2016-01-04 6:53 ` Daniel Vetter
2016-01-05 14:48 ` Thierry Reding
2016-01-05 15:22 ` Daniel Vetter
2016-01-05 15:30 ` Daniel Vetter
2016-01-04 7:20 ` ✗ warning: Fi.CI.BAT Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.