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.pinch...@ideasonboard.com> > Cc: Thierry Reding <tred...@nvidia.com> > Acked-by: Thierry Reding <tred...@nvidia.com> > Signed-off-by: Daniel Vetter <daniel.vet...@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx