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
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. Thisoperation returns true if the mode is accepted (possibly after beingadjusted) or false if it is rejected.</para><para>The <methodname>mode_fixup</methodname> operation should reject themode if it can't reasonably use it. The definition of "reasonable"is currently fuzzy in this context. One possible behaviour would beto set the adjusted mode to the panel timings when a fixed-modepanel is used with hardware capable of scaling. Another behaviourwould be to accept any input mode and adjust it to the closest modesupported 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
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. Thisoperation returns true if the mode is accepted (possibly after beingadjusted) or false if it is rejected.</para><para>The <methodname>mode_fixup</methodname> operation should reject themode if it can't reasonably use it. The definition of "reasonable"is currently fuzzy in this context. One possible behaviour would beto set the adjusted mode to the panel timings when a fixed-modepanel is used with hardware capable of scaling. Another behaviourwould be to accept any input mode and adjust it to the closest modesupported 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
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. Thisoperation returns true if the mode is accepted (possibly after beingadjusted) or false if it is rejected.</para><para>The <methodname>mode_fixup</methodname> operation should reject themode if it can't reasonably use it. The definition of "reasonable"is currently fuzzy in this context. One possible behaviour would beto set the adjusted mode to the panel timings when a fixed-modepanel is used with hardware capable of scaling. Another behaviourwould be to accept any input mode and adjust it to the closest modesupported 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
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
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
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
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. Thisoperation returns true if the mode is accepted (possibly after beingadjusted) or false if it is rejected.</para><para>The <methodname>mode_fixup</methodname> operation should reject themode if it can't reasonably use it. The definition of "reasonable"is currently fuzzy in this context. One possible behaviour would beto set the adjusted mode to the panel timings when a fixed-modepanel is used with hardware capable of scaling. Another behaviourwould be to accept any input mode and adjust it to the closest modesupported 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 framebuffer, 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 fallback to the <methodname>mode_set</methodname> helper operation.</para><note><para>FIXME: Why are x and y passed as arguments, as they can be accessedthrough <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 aftervalidating the requested mode. Drivers use it to performdevice-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 devicerequirements, the mode can be stored internally by the driver andapplied in the <methodname>commit</methodname> operation, orprogrammed to the hardware immediately.</para><para>The <methodname>mode_set</methodname> operation returns 0 on successor 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 fullyoperational.</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. Thisoperation returns true if the mode is accepted (possibly after beingadjusted) or false if it is rejected. See the<link linkend="drm-helper-crtc-mode-fixup">mode_fixup CRTC helperoperation</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 aftervalidating the requested mode. Drivers use it to performdevice-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 canbe stored internally by the driver and applied in the<methodname>commit</methodname> operation, or programmed to thehardware 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 fullyoperational.</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 thatmap connectors to encoders 1:1 simply return the pointer to theassociated 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> listby 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 everysupported mode and return the number of modes it has detected. Thisoperation is mandatory.</para><para>Note that the caller function will automatically add standard VESADMT modes up to 1024x768 if the <methodname>get_modes</methodname>helper operation returns no mode and if the connector status isconnector_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 theconnector <structfield>display_info</structfield><structfield>width_mm</structfield> and<structfield>height_mm</structfield> fields. When creating modesmanually the <methodname>get_modes</methodname> helper operation mustset the <structfield>display_info</structfield><structfield>width_mm</structfield> and<structfield>height_mm</structfield> fields if they haven't been setalready (for instance at initialization time when a fixed-size panel isattached to the connector). The mode <structfield>width_mm</structfield>and <structfield>height_mm</structfield> fields are only used internallyduring 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 forsupported 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 forimmediately removing the unsupported mode, an implementation canreturn MODE_BAD regardless of the exact reason why the mode is notvalid.</para><note><para>Note that the <methodname>mode_valid</methodname> helper operation isonly 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@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Thierry Reding -
Thierry Reding