Mostly this is just adding extensive docs for the callbacks, but also a few other additions.

v2: Use FIXME comments to annotate helper hooks that should be replaced.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_fb_helper.h | 92 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 13 deletions(-)

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 87b090c4b730..5ce033e36039 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -74,25 +74,76 @@ struct drm_fb_helper_surface_size {

/** * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation library - * @gamma_set: Set the given gamma lut register on the given crtc. - * @gamma_get: Read the given gamma lut register on the given crtc, used to - * save the current lut when force-restoring the fbdev for e.g. - * kdbg. - * @fb_probe: Driver callback to allocate and initialize the fbdev info - * structure. Furthermore it also needs to allocate the drm - * framebuffer used to back the fbdev. - * @initial_config: Setup an initial fbdev display configuration * * Driver callbacks used by the fbdev emulation helper library. */ struct drm_fb_helper_funcs { + /** + * @gamma_set: + * + * Set the given gamma lut register on the given crtc. + * + * This callback is optional. + * + * FIXME: + * + * This callback is functionally redundant with the core gamma table + * support and simply exists because the fbdev hasn't yet been + * refactored to use the core gamma table interfaces. + */ void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green, u16 blue, int regno); + /** + * @gamma_get: + * + * Read the given gamma lut register on the given crtc, used to save the + * current lut when force-restoring the fbdev for e.g. kdbg. + * + * This callback is optional. + * + * FIXME: + * + * This callback is functionally redundant with the core gamma table + * support and simply exists because the fbdev hasn't yet been + * refactored to use the core gamma table interfaces. + */ void (*gamma_get)(struct drm_crtc *crtc, u16 *red, u16 *green, u16 *blue, int regno);

+ /** + * @fb_probe: + * + * Driver callback to allocate and initialize the fbdev info structure. + * Furthermore it also needs to allocate the drm framebuffer used to + * back the fbdev. + * + * This callback is mandatory. + * + * RETURNS: + * + * The driver should return 0 on success and a negative error code on + * failure. + */ int (*fb_probe)(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes); + + /** + * @initial_config: + * + * Driver callback to setup an initial fbdev display configuration. + * Drivers can use this callback to tell the fbdev emulation what the + * preferred initial configuration is. This is useful to implement + * smooth booting where the fbdev (and subsequently all userspace) never + * changes the mode, but always inherits the existing configuration. + * + * This callback is optional. + * + * RETURNS: + * + * The driver should return true if a suitable initial configuration has + * been filled out and false when the fbdev helper should fall back to + * the default probing logic. + */ bool (*initial_config)(struct drm_fb_helper *fb_helper, struct drm_fb_helper_crtc **crtcs, struct drm_display_mode **modes, @@ -105,18 +156,22 @@ struct drm_fb_helper_connector { };

/** - * struct drm_fb_helper - helper to emulate fbdev on top of kms + * struct drm_fb_helper - main structure to emulate fbdev on top of kms * @fb: Scanout framebuffer object * @dev: DRM device * @crtc_count: number of possible CRTCs * @crtc_info: per-CRTC helper state (mode, x/y offset, etc) * @connector_count: number of connected connectors * @connector_info_alloc_count: size of connector_info + * @connector_info: array of per-connector information * @funcs: driver callbacks for fb helper * @fbdev: emulated fbdev device info struct * @pseudo_palette: fake palette of 16 colors - * @kernel_fb_list: list_head in kernel_fb_helper_list - * @delayed_hotplug: was there a hotplug while kms master active? + * + * This is the main structure used by the fbdev helpers. Drivers supporting + * fbdev emulation should embedded this into their overall driver structure. + * Drivers must also fill out a struct &drm_fb_helper_funcs with a few + * operations. */ struct drm_fb_helper { struct drm_framebuffer *fb; @@ -129,10 +184,21 @@ struct drm_fb_helper { const struct drm_fb_helper_funcs *funcs; struct fb_info *fbdev; u32 pseudo_palette[17]; + + /** + * @kernel_fb_list: + * + * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit. + */ struct list_head kernel_fb_list;

- /* we got a hotplug but fbdev wasn't running the console - delay until next set_par */ + /** + * @delayed_hotplug: + * + * A hotplug was received while fbdev wasn't in control of the drm + * device, i.e. another kms master was active. The output configuration + * needs to be reprobe when fbdev is in control again. + */ bool delayed_hotplug;

/**

This is only used for kgdb (and previously panic) handlers in the fbdev emulation, so belongs there.

Note that this means we'll leave behind a forward declaration, but once all the helper vtables are consolidated (in the next patch) that will make more sense.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/nouveau/nv50_display.c | 1 + include/drm/drm_crtc_helper.h | 5 +---- include/drm/drm_fb_helper.h | 5 +++++ 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index 12f23eede8c6..da277a69a859 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -28,6 +28,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_dp_helper.h> +#include <drm/drm_fb_helper.h>

#include <nvif/class.h>

diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index e22ab29d2d00..26cc978f79e7 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -41,10 +41,7 @@

#include <drm/drm_crtc.h>

-enum mode_set_atomic { - LEAVE_ATOMIC_MODE_SET, - ENTER_ATOMIC_MODE_SET, -}; +enum mode_set_atomic;

/** * struct drm_crtc_helper_funcs - helper operations for CRTCs diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5ce033e36039..51782e765ed2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -34,6 +34,11 @@ struct drm_fb_helper;

#include <linux/kgdb.h>

+enum mode_set_atomic { + LEAVE_ATOMIC_MODE_SET, + ENTER_ATOMIC_MODE_SET, +}; + struct drm_fb_offset { int x, y; };

Currently we have 4 helper libraries (probe, crtc, plane & atomic) that all use the same helper vtables. And that's by necessity since we don't want to litter the core structs with one ops pointer per helper library. Also often the reuse the same hooks (like atomic does, to facilite conversion from existing drivers using crtc and plane helpers).

Given all that it doesn't make sense to put the docs for these next to specific helpers. Instead extract them into a new header file and section in the docbook, and add references to them everywhere.

Unfortunately kernel-doc complains when an include directive doesn't find anything (and it does by dumping crap into the output file). We have to remove the now empty includes to avoid that, instead of leaving them in for future proofing.

v2: More OCD in ordering functions.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/DocBook/gpu.tmpl | 6 +- drivers/gpu/drm/drm_atomic_helper.c | 6 + drivers/gpu/drm/drm_crtc_helper.c | 5 + drivers/gpu/drm/drm_plane_helper.c | 4 + drivers/gpu/drm/drm_probe_helper.c | 3 + include/drm/drm_crtc_helper.h | 158 +------------------ include/drm/drm_modeset_helper_vtables.h | 252 +++++++++++++++++++++++++++++++ include/drm/drm_plane_helper.h | 38 +---- 8 files changed, 277 insertions(+), 195 deletions(-) create mode 100644 include/drm/drm_modeset_helper_vtables.h

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index e6735d2b6ae8..9952d6ff052f 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -2303,8 +2303,12 @@ void intel_crt_init(struct drm_device *dev) !Edrivers/gpu/drm/drm_atomic_helper.c </sect2> <sect2> + <title>Modeset Helper Reference for Common Vtables</title> +!Iinclude/drm/drm_modeset_helper_vtables.h +!Pinclude/drm/drm_modeset_helper_vtables.h overview + </sect2> + <sect2> <title>Modeset Helper Functions Reference</title> -!Iinclude/drm/drm_crtc_helper.h !Edrivers/gpu/drm/drm_crtc_helper.c !Pdrivers/gpu/drm/drm_crtc_helper.c overview </sect2> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d9053ebf4903..2cf8ab7dbc8c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -52,6 +52,12 @@ * drm_atomic_helper_disable_plane(), drm_atomic_helper_disable_plane() and the * various functions to implement set_property callbacks. New drivers must not * implement these functions themselves but must use the provided helpers. + * + * The atomic helper uses the same function table structures as all other + * modesetting helpers. See the documentation for struct &drm_crtc_helper_funcs, + * struct &drm_encoder_helper_funcs and struct &drm_connector_helper_funcs. It + * also shares the struct &drm_plane_helper_funcs function table with the plane + * helpers. */ static void drm_atomic_helper_plane_changed(struct drm_atomic_state *state, diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 10d0989db273..077e48d3cac2 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -62,6 +62,11 @@ * converting to the plane helpers). New drivers must not use these functions * but need to implement the atomic interface instead, potentially using the * atomic helpers for that. + * + * The these legacy modeset helpers use the same function table structures as + * all other modesetting helpers. See the documentation for struct + * &drm_crtc_helper_funcs, struct &drm_encoder_helper_funcs and struct + * &drm_connector_helper_funcs. */ MODULE_AUTHOR("David Airlie, Jesse Barnes"); MODULE_DESCRIPTION("DRM KMS helper"); diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index a6983d41920d..8455e996dd9c 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -57,6 +57,10 @@ * by the atomic helpers. * * Again drivers are strongly urged to switch to the new interfaces. + * + * The plane helpers share the function table structures with other helpers, + * specifically also the atomic helpers. See struct &drm_plane_helper_funcs for + * the details. */

/* diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index b7bdf12c54a5..dcd7c0289e03 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -53,6 +53,9 @@ * This helper library can be used independently of the modeset helper library. * Drivers can also overwrite different parts e.g. use their own hotplug * handling code to avoid probing unrelated outputs. + * + * The probe helpers share the function table structures with other display + * helper libraries. See struct &drm_connector_helper_funcs for the details. */

static bool drm_kms_helper_poll = true; diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 26cc978f79e7..f94ff54ae25e 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -40,145 +40,7 @@ #include <linux/fb.h>

#include <drm/drm_crtc.h> - -enum mode_set_atomic; - -/** - * struct drm_crtc_helper_funcs - helper operations for CRTCs - * @dpms: set power state - * @prepare: prepare the CRTC, called before @mode_set - * @commit: commit changes to CRTC, called after @mode_set - * @mode_fixup: try to fixup proposed mode for this CRTC - * @mode_set: set this mode - * @mode_set_nofb: set mode only (no scanout buffer attached) - * @mode_set_base: update the scanout buffer - * @mode_set_base_atomic: non-blocking mode set (used for kgdb support) - * @load_lut: load color palette - * @disable: disable CRTC when no longer in use - * @enable: enable CRTC - * @atomic_check: check for validity of an atomic state - * @atomic_begin: begin atomic update - * @atomic_flush: flush atomic update - * - * The helper operations are called by the mid-layer CRTC helper. - * - * Note that with atomic helpers @dpms, @prepare and @commit hooks are - * deprecated. Used @enable and @disable instead exclusively. - * - * With legacy crtc helpers there's a big semantic difference between @disable - * and the other hooks: @disable also needs to release any resources acquired in - * @mode_set (like shared PLLs). - */ -struct drm_crtc_helper_funcs { - /* - * Control power levels on the CRTC. If the mode passed in is - * unsupported, the provider must use the next lowest power level. - */ - void (*dpms)(struct drm_crtc *crtc, int mode); - void (*prepare)(struct drm_crtc *crtc); - void (*commit)(struct drm_crtc *crtc); - - /* Provider can fixup or change mode timings before modeset occurs */ - bool (*mode_fixup)(struct drm_crtc *crtc, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode); - /* Actually set the mode */ - 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); - /* Actually set the mode for atomic helpers, optional */ - void (*mode_set_nofb)(struct drm_crtc *crtc); - - /* Move the crtc on the current fb to the given position *optional* */ - int (*mode_set_base)(struct drm_crtc *crtc, int x, int y, - struct drm_framebuffer *old_fb); - int (*mode_set_base_atomic)(struct drm_crtc *crtc, - struct drm_framebuffer *fb, int x, int y, - enum mode_set_atomic); - - /* reload the current crtc LUT */ - void (*load_lut)(struct drm_crtc *crtc); - - void (*disable)(struct drm_crtc *crtc); - void (*enable)(struct drm_crtc *crtc); - - /* atomic helpers */ - int (*atomic_check)(struct drm_crtc *crtc, - struct drm_crtc_state *state); - void (*atomic_begin)(struct drm_crtc *crtc, - struct drm_crtc_state *old_crtc_state); - void (*atomic_flush)(struct drm_crtc *crtc, - struct drm_crtc_state *old_crtc_state); -}; - -/** - * struct drm_encoder_helper_funcs - helper operations for encoders - * @dpms: set power state - * @save: save connector state - * @restore: restore connector state - * @mode_fixup: try to fixup proposed mode for this connector - * @prepare: part of the disable sequence, called before the CRTC modeset - * @commit: called after the CRTC modeset - * @mode_set: set this mode, optional for atomic helpers - * @get_crtc: return CRTC that the encoder is currently attached to - * @detect: connection status detection - * @disable: disable encoder when not in use (overrides DPMS off) - * @enable: enable encoder - * @atomic_check: check for validity of an atomic update - * - * The helper operations are called by the mid-layer CRTC helper. - * - * Note that with atomic helpers @dpms, @prepare and @commit hooks are - * deprecated. Used @enable and @disable instead exclusively. - * - * With legacy crtc helpers there's a big semantic difference between @disable - * and the other hooks: @disable also needs to release any resources acquired in - * @mode_set (like shared PLLs). - */ -struct drm_encoder_helper_funcs { - void (*dpms)(struct drm_encoder *encoder, int mode); - void (*save)(struct drm_encoder *encoder); - void (*restore)(struct drm_encoder *encoder); - - bool (*mode_fixup)(struct drm_encoder *encoder, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode); - void (*prepare)(struct drm_encoder *encoder); - void (*commit)(struct drm_encoder *encoder); - void (*mode_set)(struct drm_encoder *encoder, - struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode); - struct drm_crtc *(*get_crtc)(struct drm_encoder *encoder); - /* detect for DAC style encoders */ - enum drm_connector_status (*detect)(struct drm_encoder *encoder, - struct drm_connector *connector); - void (*disable)(struct drm_encoder *encoder); - - void (*enable)(struct drm_encoder *encoder); - - /* atomic helpers */ - int (*atomic_check)(struct drm_encoder *encoder, - struct drm_crtc_state *crtc_state, - struct drm_connector_state *conn_state); -}; - -/** - * struct drm_connector_helper_funcs - helper operations for connectors - * @get_modes: get mode list for this connector - * @mode_valid: is this mode valid on the given connector? (optional) - * @best_encoder: return the preferred encoder for this connector - * @atomic_best_encoder: atomic version of @best_encoder - * - * The helper operations are called by the mid-layer CRTC helper. - */ -struct drm_connector_helper_funcs { - int (*get_modes)(struct drm_connector *connector); - enum drm_mode_status (*mode_valid)(struct drm_connector *connector, - struct drm_display_mode *mode); - struct drm_encoder *(*best_encoder)(struct drm_connector *connector); - struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector, - struct drm_connector_state *connector_state); -}; +#include <drm/drm_modeset_helper_vtables.h>

extern void drm_helper_disable_unused_functions(struct drm_device *dev); extern int drm_crtc_helper_set_config(struct drm_mode_set *set); @@ -196,24 +58,6 @@ extern void drm_helper_move_panel_connectors_to_head(struct drm_device *); extern void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb, const struct drm_mode_fb_cmd2 *mode_cmd);

-static inline void drm_crtc_helper_add(struct drm_crtc *crtc, - const struct drm_crtc_helper_funcs *funcs) -{ - crtc->helper_private = funcs; -} - -static inline void drm_encoder_helper_add(struct drm_encoder *encoder, - const struct drm_encoder_helper_funcs *funcs) -{ - encoder->helper_private = funcs; -} - -static inline void drm_connector_helper_add(struct drm_connector *connector, - const struct drm_connector_helper_funcs *funcs) -{ - connector->helper_private = funcs; -} - extern void drm_helper_resume_force_mode(struct drm_device *dev);

int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h new file mode 100644 index 000000000000..35c5a1c4e7ba --- /dev/null +++ b/include/drm/drm_modeset_helper_vtables.h @@ -0,0 +1,252 @@ +/* + * Copyright © 2015 Intel Corporation + * Daniel Vetter daniel.vetter@ffwll.ch + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef __DRM_MODESET_HELPER_VTABLES_H__ +#define __DRM_MODESET_HELPER_VTABLES_H__ + +#include <drm/drm_crtc.h> + +/** + * DOC: overview + * + * The DRM mode setting helper functions are common code for drivers to use if + * they wish. Drivers are not forced to use this code in their + * implementations but it would be useful if the code they do use at least + * provides a consistent interface and operation to userspace. Therefore it is + * highly recommended to use the provided helpers as much as possible. + * + * Because there is only one pointer per modeset object to hold a vfunc table + * for helper libraries they are by necessity shared among the different + * helpers. + * + * To make this clear all the helper vtables are pulled together in this location here. + */ + +enum mode_set_atomic; + +/** + * struct drm_crtc_helper_funcs - helper operations for CRTCs + * @dpms: set power state + * @prepare: prepare the CRTC, called before @mode_set + * @commit: commit changes to CRTC, called after @mode_set + * @mode_fixup: try to fixup proposed mode for this CRTC + * @mode_set: set this mode + * @mode_set_nofb: set mode only (no scanout buffer attached) + * @mode_set_base: update the scanout buffer + * @mode_set_base_atomic: non-blocking mode set (used for kgdb support) + * @load_lut: load color palette + * @disable: disable CRTC when no longer in use + * @enable: enable CRTC + * @atomic_check: check for validity of an atomic state + * @atomic_begin: begin atomic update + * @atomic_flush: flush atomic update + * + * The helper operations are called by the mid-layer CRTC helper. + * + * Note that with atomic helpers @dpms, @prepare and @commit hooks are + * deprecated. Used @enable and @disable instead exclusively. + * + * With legacy crtc helpers there's a big semantic difference between @disable + * and the other hooks: @disable also needs to release any resources acquired in + * @mode_set (like shared PLLs). + */ +struct drm_crtc_helper_funcs { + /* + * Control power levels on the CRTC. If the mode passed in is + * unsupported, the provider must use the next lowest power level. + */ + void (*dpms)(struct drm_crtc *crtc, int mode); + void (*prepare)(struct drm_crtc *crtc); + void (*commit)(struct drm_crtc *crtc); + + /* Provider can fixup or change mode timings before modeset occurs */ + bool (*mode_fixup)(struct drm_crtc *crtc, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); + /* Actually set the mode */ + 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); + /* Actually set the mode for atomic helpers, optional */ + void (*mode_set_nofb)(struct drm_crtc *crtc); + + /* Move the crtc on the current fb to the given position *optional* */ + int (*mode_set_base)(struct drm_crtc *crtc, int x, int y, + struct drm_framebuffer *old_fb); + int (*mode_set_base_atomic)(struct drm_crtc *crtc, + struct drm_framebuffer *fb, int x, int y, + enum mode_set_atomic); + + /* reload the current crtc LUT */ + void (*load_lut)(struct drm_crtc *crtc); + + void (*disable)(struct drm_crtc *crtc); + void (*enable)(struct drm_crtc *crtc); + + /* atomic helpers */ + int (*atomic_check)(struct drm_crtc *crtc, + struct drm_crtc_state *state); + void (*atomic_begin)(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state); + void (*atomic_flush)(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state); +}; + +/** + * drm_crtc_helper_add - sets the helper vtable for a crtc + * @crtc: DRM CRTC + * @funcs: helper vtable to set for @crtc + */ +static inline void drm_crtc_helper_add(struct drm_crtc *crtc, + const struct drm_crtc_helper_funcs *funcs) +{ + crtc->helper_private = funcs; +} + +/** + * struct drm_encoder_helper_funcs - helper operations for encoders + * @dpms: set power state + * @save: save connector state + * @restore: restore connector state + * @mode_fixup: try to fixup proposed mode for this connector + * @prepare: part of the disable sequence, called before the CRTC modeset + * @commit: called after the CRTC modeset + * @mode_set: set this mode, optional for atomic helpers + * @get_crtc: return CRTC that the encoder is currently attached to + * @detect: connection status detection + * @disable: disable encoder when not in use (overrides DPMS off) + * @enable: enable encoder + * @atomic_check: check for validity of an atomic update + * + * The helper operations are called by the mid-layer CRTC helper. + * + * Note that with atomic helpers @dpms, @prepare and @commit hooks are + * deprecated. Used @enable and @disable instead exclusively. + * + * With legacy crtc helpers there's a big semantic difference between @disable + * and the other hooks: @disable also needs to release any resources acquired in + * @mode_set (like shared PLLs). + */ +struct drm_encoder_helper_funcs { + void (*dpms)(struct drm_encoder *encoder, int mode); + void (*save)(struct drm_encoder *encoder); + void (*restore)(struct drm_encoder *encoder); + + bool (*mode_fixup)(struct drm_encoder *encoder, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); + void (*prepare)(struct drm_encoder *encoder); + void (*commit)(struct drm_encoder *encoder); + void (*mode_set)(struct drm_encoder *encoder, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); + struct drm_crtc *(*get_crtc)(struct drm_encoder *encoder); + /* detect for DAC style encoders */ + enum drm_connector_status (*detect)(struct drm_encoder *encoder, + struct drm_connector *connector); + void (*disable)(struct drm_encoder *encoder); + + void (*enable)(struct drm_encoder *encoder); + + /* atomic helpers */ + int (*atomic_check)(struct drm_encoder *encoder, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state); +}; + +/** + * drm_encoder_helper_add - sets the helper vtable for a encoder + * @encoder: DRM encoder + * @funcs: helper vtable to set for @encoder + */ +static inline void drm_encoder_helper_add(struct drm_encoder *encoder, + const struct drm_encoder_helper_funcs *funcs) +{ + encoder->helper_private = funcs; +} + +/** + * struct drm_connector_helper_funcs - helper operations for connectors + * @get_modes: get mode list for this connector + * @mode_valid: is this mode valid on the given connector? (optional) + * @best_encoder: return the preferred encoder for this connector + * @atomic_best_encoder: atomic version of @best_encoder + * + * The helper operations are called by the mid-layer CRTC helper. + */ +struct drm_connector_helper_funcs { + int (*get_modes)(struct drm_connector *connector); + enum drm_mode_status (*mode_valid)(struct drm_connector *connector, + struct drm_display_mode *mode); + struct drm_encoder *(*best_encoder)(struct drm_connector *connector); + struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector, + struct drm_connector_state *connector_state); +}; + +/** + * drm_connector_helper_add - sets the helper vtable for a connector + * @connector: DRM connector + * @funcs: helper vtable to set for @connector + */ +static inline void drm_connector_helper_add(struct drm_connector *connector, + const struct drm_connector_helper_funcs *funcs) +{ + connector->helper_private = funcs; +} + +/** + * struct drm_plane_helper_funcs - helper operations for CRTCs + * @prepare_fb: prepare a framebuffer for use by the plane + * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane + * @atomic_check: check that a given atomic state is valid and can be applied + * @atomic_update: apply an atomic state to the plane (mandatory) + * @atomic_disable: disable the plane + * + * The helper operations are called by the mid-layer CRTC helper. + */ +struct drm_plane_helper_funcs { + int (*prepare_fb)(struct drm_plane *plane, + const struct drm_plane_state *new_state); + void (*cleanup_fb)(struct drm_plane *plane, + const struct drm_plane_state *old_state); + + int (*atomic_check)(struct drm_plane *plane, + struct drm_plane_state *state); + void (*atomic_update)(struct drm_plane *plane, + struct drm_plane_state *old_state); + void (*atomic_disable)(struct drm_plane *plane, + struct drm_plane_state *old_state); +}; + +/** + * drm_plane_helper_add - sets the helper vtable for a plane + * @plane: DRM plane + * @funcs: helper vtable to set for @plane + */ +static inline void drm_plane_helper_add(struct drm_plane *plane, + const struct drm_plane_helper_funcs *funcs) +{ + plane->helper_private = funcs; +} + +#endif diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index 5a7f9d4efb1d..4421f3f4ca8d 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -26,6 +26,7 @@

#include <drm/drm_rect.h> #include <drm/drm_crtc.h> +#include <drm/drm_modeset_helper_vtables.h>

/* * Drivers that don't allow primary plane scaling may pass this macro in place @@ -36,46 +37,9 @@ */ #define DRM_PLANE_HELPER_NO_SCALING (1<<16)

-/** - * DOC: plane helpers - * - * Helper functions to assist with creation and handling of CRTC primary - * planes. - */ - int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs);

-/** - * drm_plane_helper_funcs - helper operations for CRTCs - * @prepare_fb: prepare a framebuffer for use by the plane - * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane - * @atomic_check: check that a given atomic state is valid and can be applied - * @atomic_update: apply an atomic state to the plane (mandatory) - * @atomic_disable: disable the plane - * - * The helper operations are called by the mid-layer CRTC helper. - */ -struct drm_plane_helper_funcs { - int (*prepare_fb)(struct drm_plane *plane, - const struct drm_plane_state *new_state); - void (*cleanup_fb)(struct drm_plane *plane, - const struct drm_plane_state *old_state); - - int (*atomic_check)(struct drm_plane *plane, - struct drm_plane_state *state); - void (*atomic_update)(struct drm_plane *plane, - struct drm_plane_state *old_state); - void (*atomic_disable)(struct drm_plane *plane, - struct drm_plane_state *old_state); -}; - -static inline void drm_plane_helper_add(struct drm_plane *plane, - const struct drm_plane_helper_funcs *funcs) -{ - plane->helper_private = funcs; -} - int drm_plane_helper_check_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb,

Originally the idea behind void* was to allow different sets of helpers. But now we have that (with probe, plane, crtc and atomic helpers) and we still just use the same set of vtables. That's the only way to make the individual helpers modular and allow drivers to pick&choose and transition between them. So this flexibility isn't really needed. Also we have lots of non-vtable data meanwhile in core structures too, this is not the first one at all.

Given that the void * is only trouble since gcc can't warn you if you mix them up. Let's fix that and make them typesafe.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_crtc.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4765df331002..e8f8e4e9a6a1 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -254,6 +254,11 @@ struct drm_plane; struct drm_bridge; struct drm_atomic_state;

+struct drm_crtc_helper_funcs; +struct drm_encoder_helper_funcs; +struct drm_connector_helper_funcs; +struct drm_plane_helper_funcs; + /** * struct drm_crtc_state - mutable CRTC state * @crtc: backpointer to the CRTC @@ -467,7 +472,7 @@ struct drm_crtc { uint16_t *gamma_store;

/* if you are using the helper */ - const void *helper_private; + const struct drm_crtc_helper_funcs *helper_private;

struct drm_object_properties properties;

@@ -597,7 +602,7 @@ struct drm_encoder { struct drm_crtc *crtc; struct drm_bridge *bridge; const struct drm_encoder_funcs *funcs; - const void *helper_private; + const struct drm_encoder_helper_funcs *helper_private; };

/* should we poll this connector for connects and disconnects */ @@ -702,7 +707,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;

- const void *helper_private; + const struct drm_connector_helper_funcs *helper_private;

/* forced on connector */ struct drm_cmdline_mode cmdline_mode; @@ -870,7 +875,7 @@ struct drm_plane {

enum drm_plane_type type;

- const void *helper_private; + const struct drm_plane_helper_funcs *helper_private;

struct drm_plane_state *state; };

Duplication is bad, luckily both help texts highlighted different issues so the kerneldoc gained quite a bit!

While at it also sprinkle more references to the vtable structs around and make it clear that legacy CRTC helpers are deprecated and which functions to use instead.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/DocBook/gpu.tmpl | 80 +------------------------------------- drivers/gpu/drm/drm_crtc_helper.c | 47 +++++++++++++++++++--- drivers/gpu/drm/drm_probe_helper.c | 27 ++++++++++--- 3 files changed, 65 insertions(+), 89 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 9952d6ff052f..8b298fc2dd4d 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -1830,83 +1830,7 @@ void intel_crt_init(struct drm_device *dev) entities. </para> <sect2> - <title>Helper Functions</title> - <itemizedlist> - <listitem> - <synopsis>int drm_crtc_helper_set_config(struct drm_mode_set *set);</synopsis> - <para> - The <function>drm_crtc_helper_set_config</function> helper function - is a CRTC <methodname>set_config</methodname> implementation. It - first tries to locate the best encoder for each connector by calling - the connector <methodname>best_encoder</methodname> helper - operation. - </para> - <para> - After locating the appropriate encoders, the helper function will - call the <methodname>mode_fixup</methodname> encoder and CRTC helper - operations to adjust the requested mode, or reject it completely in - which case an error will be returned to the application. If the new - configuration after mode adjustment is identical to the current - configuration the helper function will return without performing any - other operation. - </para> - <para> - If the adjusted mode is identical to the current mode but changes to - the frame buffer need to be applied, the - <function>drm_crtc_helper_set_config</function> function will call - the CRTC <methodname>mode_set_base</methodname> helper operation. If - the adjusted mode differs from the current mode, or if the - <methodname>mode_set_base</methodname> helper operation is not - provided, the helper function performs a full mode set sequence by - calling the <methodname>prepare</methodname>, - <methodname>mode_set</methodname> and - <methodname>commit</methodname> CRTC and encoder helper operations, - in that order. - </para> - </listitem> - <listitem> - <synopsis>void drm_helper_connector_dpms(struct drm_connector *connector, int mode);</synopsis> - <para> - The <function>drm_helper_connector_dpms</function> helper function - is a connector <methodname>dpms</methodname> implementation that - tracks power state of connectors. To use the function, drivers must - provide <methodname>dpms</methodname> helper operations for CRTCs - and encoders to apply the DPMS state to the device. - </para> - <para> - The mid-layer doesn't track the power state of CRTCs and encoders. - The <methodname>dpms</methodname> helper operations can thus be - called with a mode identical to the currently active mode. - </para> - </listitem> - <listitem> - <synopsis>int drm_helper_probe_single_connector_modes(struct drm_connector *connector, - uint32_t maxX, uint32_t maxY);</synopsis> - <para> - The <function>drm_helper_probe_single_connector_modes</function> helper - function is a connector <methodname>fill_modes</methodname> - implementation that updates the connection status for the connector - and then retrieves a list of modes by calling the connector - <methodname>get_modes</methodname> helper operation. - </para> - <para> - If the helper operation returns no mode, and if the connector status - is connector_status_connected, standard VESA DMT modes up to - 1024x768 are automatically added to the modes list by a call to - <function>drm_add_modes_noedid</function>. - </para> - <para> - The function then filters out modes larger than - <parameter>max_width</parameter> and <parameter>max_height</parameter> - if specified. It finally calls the optional connector - <methodname>mode_valid</methodname> helper operation for each mode in - the probed list to check whether the mode is valid for the connector. - </para> - </listitem> - </itemizedlist> - </sect2> - <sect2> - <title>CRTC Helper Operations</title> + <title>Legacy CRTC Helper Operations</title> <itemizedlist> <listitem id="drm-helper-crtc-mode-fixup"> <synopsis>bool (*mode_fixup)(struct drm_crtc *crtc, @@ -2308,7 +2232,7 @@ void intel_crt_init(struct drm_device *dev) !Pinclude/drm/drm_modeset_helper_vtables.h overview </sect2> <sect2> - <title>Modeset Helper Functions Reference</title> + <title>Legacy CRTC/Modeset Helper Functions Reference</title> !Edrivers/gpu/drm/drm_crtc_helper.c !Pdrivers/gpu/drm/drm_crtc_helper.c overview </sect2> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 077e48d3cac2..c4fbcf8e6664 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -51,6 +51,11 @@ * the same callbacks which drivers can use to e.g. restore the modeset * configuration on resume with drm_helper_resume_force_mode(). * + * Note that this helper library doesn't track the current power state of CRTCs + * and encoders. It can callbacks like ->dpms() even though the hardware is + * already in the desired state. This deficiency has been fixed in the atomic + * helpers. + * * The driver callbacks are mostly compatible with the atomic modeset helpers, * except for the handling of the primary plane: Atomic helpers require that the * primary plane is implemented as a real standalone plane and not directly tied @@ -450,11 +455,33 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) * drm_crtc_helper_set_config - set a new config from userspace * @set: mode set configuration * - * Setup a new configuration, provided by the upper layers (either an ioctl call - * from userspace or internally e.g. from the fbdev support code) in @set, and - * enable it. This is the main helper functions for drivers that implement - * kernel mode setting with the crtc helper functions and the assorted - * ->prepare(), ->modeset() and ->commit() helper callbacks. + * The drm_crtc_helper_set_config() helper function implements the set_config + * callback of struct &drm_crtc_funcs for drivers using the legacy CRTC helpers. + * + * It first tries to locate the best encoder for each connector by calling the + * connector best_encoder (struct &drm_connector_helper_funcs) helper operation. + * + * After locating the appropriate encoders, the helper function will call the + * mode_fixup encoder and CRTC helper operations to adjust the requested mode, + * or reject it completely in which case an error will be returned to the + * application. If the new configuration after mode adjustment is identical to + * the current configuration the helper function will return without performing + * any other operation. + * + * If the adjusted mode is identical to the current mode but changes to the + * frame buffer need to be applied, the drm_crtc_helper_set_config function will + * call the CRTC mode_set_base (struct &drm_crtc_helper_funcs) helper operation. + * + * If the adjusted mode differs from the current mode, or if the mode_set_base + * helper operation is not provided, the helper function performs a full mode + * set sequence by calling the prepare, mode_set and commit CRTC and encoder + * helper operations, in that order. Alternatively it can also use the dpms and + * disable helper operations. For details see struct &drm_crtc_helper_funcs and + * struct &drm_encoder_helper_funcs. + * + * This function is deprecated. New drivers must implement atomic modeset + * support, for which this function is unsuitable. Instead drivers should use + * drm_atomic_helper_set_config(). * * Returns: * Returns 0 on success, negative errno numbers on failure. @@ -763,10 +790,18 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) * @connector: affected connector * @mode: DPMS mode * + * The drm_helper_connector_dpms() helper function implements the dpms + * callback of struct &drm_connector_funcs for drivers using the legacy CRTC helpers. + * * This is the main helper function provided by the crtc helper framework for * implementing the DPMS connector attribute. It computes the new desired DPMS * state for all encoders and crtcs in the output mesh and calls the ->dpms() - * callback provided by the driver appropriately. + * callbacks provided by the driver in struct &drm_crtc_helper_funcs and struct + * &drm_encoder_helper_funcs appropriately. + * + * This function is deprecated. New drivers must implement atomic modeset + * support, for which this function is unsuitable. Instead drivers should use + * drm_atomic_helper_connector_dpms(). * * Returns: * Always returns 0. diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index dcd7c0289e03..62889249cbf8 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -272,15 +272,29 @@ prune: * @maxX: max width for modes * @maxY: max height for modes * - * Based on the helper callbacks implemented by @connector try to detect all - * valid modes. Modes will first be added to the connector's probed_modes list, - * then culled (based on validity and the @maxX, @maxY parameters) and put into - * the normal modes list. + * Based on the helper callbacks implemented by @connector in struct + * &drm_connector_helper_funcs try to detect all valid modes. Modes will first + * be added to the connector's probed_modes list, then culled (based on validity + * and the @maxX, @maxY parameters) and put into the normal modes list. * * Intended to be use as a generic implementation of the ->fill_modes() * @connector vfunc for drivers that use the crtc helpers for output mode * filtering and detection. * + * If the helper operation returns no mode, and if the connector status is + * connector_status_connected, standard VESA DMT modes up to 1024x768 are + * automatically added to the modes list by a call to + * drm_add_modes_noedid(). + * + * The function then filters out modes larger than + * @maxX and maxY if specified. It finally calls the optional connector + * mode_valid helper operation for each mode in the probed list to check whether + * the mode is valid for the connector. + * + * Compared to drm_helper_probe_single_connector_modes_nomerge() this function + * merged the mode bits for the preferred mode, as a hack to work around some + * quirky issues on funky hardware. + * * Returns: * The number of modes found on @connector. */ @@ -297,8 +311,11 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes); * @maxX: max width for modes * @maxY: max height for modes * - * This operates like drm_hehlper_probe_single_connector_modes except it + * This operates like drm_hehlper_probe_single_connector_modes() except it * replaces the mode bits instead of merging them for preferred modes. + * + * Returns: + * The number of modes found on @connector. */ int drm_helper_probe_single_connector_modes_nomerge(struct drm_connector *connector, uint32_t maxX, uint32_t maxY)

Especially document the assumptions and semantics of the callbacks carefully. Just a warm-up excercise really.

v2: Spelling fixes (Eric).

v3: Consolidate more with existing docs:

- Remove the overview section explaining the bridge funcs, that's now all in the drm_bridge_funcs kerneldoc in much more detail.

- Use & to reference structs so that kerneldoc automatically inserts hyperlinks.

Cc: Eric Anholt eric@anholt.net Cc: Archit Taneja architt@codeaurora.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_bridge.c | 69 +++++++++++------------------ include/drm/drm_crtc.h | 102 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 122 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 6b8f7211e543..26dd1cfb6078 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -31,14 +31,14 @@ /** * DOC: overview * - * drm_bridge represents a device that hangs on to an encoder. These are handy - * when a regular drm_encoder entity isn't enough to represent the entire + * struct &drm_bridge represents a device that hangs on to an encoder. These are + * handy when a regular &drm_encoder entity isn't enough to represent the entire * encoder chain. * - * A bridge is always associated to a single drm_encoder at a time, but can be + * A bridge is always associated to a single &drm_encoder at a time, but can be * either connected to it directly, or through an intermediate bridge: * - * encoder ---> bridge B ---> bridge A + * encoder ---> bridge B ---> bridge A * * Here, the output of the encoder feeds to bridge B, and that furthers feeds to * bridge A. @@ -46,11 +46,16 @@ * The driver using the bridge is responsible to make the associations between * the encoder and bridges. Once these links are made, the bridges will * participate along with encoder functions to perform mode_set/enable/disable - * through the ops provided in drm_bridge_funcs. + * through the ops provided in &drm_bridge_funcs. * * drm_bridge, like drm_panel, aren't drm_mode_object entities like planes, - * crtcs, encoders or connectors. They just provide additional hooks to get the - * desired output at the end of the encoder chain. + * crtcs, encoders or connectors and hence are not visible to userspace. They + * just provide additional hooks to get the desired output at the end of the + * encoder chain. + * + * Bridges can also be chained up using the next pointer in struct &drm_bridge. + * + * Both legacy CRTC helpers and the new atomic modeset helpers support bridges. */

static DEFINE_MUTEX(bridge_lock); @@ -122,34 +127,12 @@ EXPORT_SYMBOL(drm_bridge_attach); /** * DOC: bridge callbacks * - * The drm_bridge_funcs ops are populated by the bridge driver. The drm - * internals(atomic and crtc helpers) use the helpers defined in drm_bridge.c - * These helpers call a specific drm_bridge_funcs op for all the bridges + * The &drm_bridge_funcs ops are populated by the bridge driver. The drm + * internals (atomic and crtc helpers) use the helpers defined in drm_bridge.c + * These helpers call a specific &drm_bridge_funcs op for all the bridges * during encoder configuration. * - * When creating a bridge driver, one can implement drm_bridge_funcs op with - * the help of these rough rules: - * - * pre_enable: this contains things needed to be done for the bridge before - * its clock and timings are enabled by its source. For a bridge, its source - * is generally the encoder or bridge just before it in the encoder chain. - * - * enable: this contains things needed to be done for the bridge once its - * source is enabled. In other words, enable is called once the source is - * ready with clock and timing needed by the bridge. - * - * disable: this contains things needed to be done for the bridge assuming - * that its source is still enabled, i.e. clock and timings are still on. - * - * post_disable: this contains things needed to be done for the bridge once - * its source is disabled, i.e. once clocks and timings are off. - * - * mode_fixup: this should fixup the given mode for the bridge. It is called - * after the encoder's mode fixup. mode_fixup can also reject a mode completely - * if it's unsuitable for the hardware. - * - * mode_set: this sets up the mode for the bridge. It assumes that its source - * (an encoder or a bridge) has set the mode too. + * For detailed specification of the bridge callbacks see &drm_bridge_funcs. */

/** @@ -159,7 +142,7 @@ EXPORT_SYMBOL(drm_bridge_attach); * @mode: desired mode to be set for the bridge * @adjusted_mode: updated mode that works for this bridge * - * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the + * Calls 'mode_fixup' &drm_bridge_funcs op for all the bridges in the * encoder chain, starting from the first bridge to the last. * * Note: the bridge passed should be the one closest to the encoder @@ -186,11 +169,11 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_bridge_mode_fixup);

/** - * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all + * drm_bridge_disable - calls 'disable' &drm_bridge_funcs op for all * bridges in the encoder chain. * @bridge: bridge control structure * - * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder + * Calls 'disable' &drm_bridge_funcs op for all the bridges in the encoder * chain, starting from the last bridge to the first. These are called before * calling the encoder's prepare op. * @@ -208,11 +191,11 @@ void drm_bridge_disable(struct drm_bridge *bridge) EXPORT_SYMBOL(drm_bridge_disable);

/** - * drm_bridge_post_disable - calls 'post_disable' drm_bridge_funcs op for + * drm_bridge_post_disable - calls 'post_disable' &drm_bridge_funcs op for * all bridges in the encoder chain. * @bridge: bridge control structure * - * Calls 'post_disable' drm_bridge_funcs op for all the bridges in the + * Calls 'post_disable' &drm_bridge_funcs op for all the bridges in the * encoder chain, starting from the first bridge to the last. These are called * after completing the encoder's prepare op. * @@ -236,7 +219,7 @@ EXPORT_SYMBOL(drm_bridge_post_disable); * @mode: desired mode to be set for the bridge * @adjusted_mode: updated mode that works for this bridge * - * Calls 'mode_set' drm_bridge_funcs op for all the bridges in the + * Calls 'mode_set' &drm_bridge_funcs op for all the bridges in the * encoder chain, starting from the first bridge to the last. * * Note: the bridge passed should be the one closest to the encoder @@ -256,11 +239,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_bridge_mode_set);

/** - * drm_bridge_pre_enable - calls 'pre_enable' drm_bridge_funcs op for all + * drm_bridge_pre_enable - calls 'pre_enable' &drm_bridge_funcs op for all * bridges in the encoder chain. * @bridge: bridge control structure * - * Calls 'pre_enable' drm_bridge_funcs op for all the bridges in the encoder + * Calls 'pre_enable' &drm_bridge_funcs op for all the bridges in the encoder * chain, starting from the last bridge to the first. These are called * before calling the encoder's commit op. * @@ -278,11 +261,11 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge) EXPORT_SYMBOL(drm_bridge_pre_enable);

/** - * drm_bridge_enable - calls 'enable' drm_bridge_funcs op for all bridges + * drm_bridge_enable - calls 'enable' &drm_bridge_funcs op for all bridges * in the encoder chain. * @bridge: bridge control structure * - * Calls 'enable' drm_bridge_funcs op for all the bridges in the encoder + * Calls 'enable' &drm_bridge_funcs op for all the bridges in the encoder * chain, starting from the first bridge to the last. These are called * after completing the encoder's commit op. * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e8f8e4e9a6a1..ec3632e62b3f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -883,24 +883,114 @@ struct drm_plane { /** * struct drm_bridge_funcs - drm_bridge control functions * @attach: Called during drm_bridge_attach - * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge - * @disable: Called right before encoder prepare, disables the bridge - * @post_disable: Called right after encoder prepare, for lockstepped disable - * @mode_set: Set this mode to the bridge - * @pre_enable: Called right before encoder commit, for lockstepped commit - * @enable: Called right after encoder commit, enables the bridge */ struct drm_bridge_funcs { int (*attach)(struct drm_bridge *bridge); + + /** + * @mode_fixup: + * + * This callback is used to validate and adjust a mode. The paramater + * mode is the display mode that should be fed to the next element in + * the display chain, either the final &drm_connector or the next + * &drm_bridge. The parameter adjusted_mode is the input mode the bridge + * requires. It can be modified by this callback and does not need to + * match mode. + * + * This is the only hook that allows a bridge to reject a modeset. If + * this function passes all other callbacks must succeed for this + * configuration. + * + * NOTE: + * + * This function is called in the check phase of atomic modesets, which + * can be aborted for any reason (including on userspace's request to + * just check whether a configuration would be possible). Drivers MUST + * NOT touch any persistent state (hardware or software) or data + * structures except the passed in adjusted_mode parameter. + * + * RETURNS: + * + * True if an acceptable configuration is possible, false if the modeset + * operation should be rejected. + */ bool (*mode_fixup)(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); + /** + * @disable: + * + * This callback should disable the bridge. It is called right before + * the preceding element in the display pipe is disabled. If the + * preceding element is a bridge this means it's called before that + * bridge's ->disaable function. If the preceding element is a + * &drm_encoder it's called right before the encoder's ->disable, + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The bridge can assume that the display pipe (i.e. clocks and timing + * signals) feeding it is still running when this callback is called. + */ void (*disable)(struct drm_bridge *bridge); + + /** + * @post_disable: + * + * This callback should disable the bridge. It is called right after + * the preceding element in the display pipe is disabled. If the + * preceding element is a bridge this means it's called after that + * bridge's ->post_disaable function. If the preceding element is a + * &drm_encoder it's called right after the encoder's ->disable, + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The bridge must assume that the display pipe (i.e. clocks and timing + * singals) feeding it is no longer running when this callback is + * called. + */ void (*post_disable)(struct drm_bridge *bridge); + + /** + * @mode_set: + * + * This callback should set the given mode on the bridge. It is called + * after the ->mode_set callback for the preceding element in the + * display pipeline has been called already. The display pipe (i.e. + * clocks and timing signals) is off when this function is called. + */ void (*mode_set)(struct drm_bridge *bridge, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); + /** + * @pre_enable: + * + * This callback should enable the bridge. It is called right before + * the preceding element in the display pipe is enabled. If the + * preceding element is a bridge this means it's called before that + * bridge's ->pre_enable function. If the preceding element is a + * &drm_encoder it's called right before the encoder's ->enable, + * ->commit or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The display pipe (i.e. clocks and timing signals) feeding this bridge + * will not yet be running when this callback is called. The bridge must + * not enable the display link feeding the next bridge in the chain (if + * there is one) when this callback is called. + */ void (*pre_enable)(struct drm_bridge *bridge); + + /** + * @enable: + * + * This callback should enable the bridge. It is called right after + * the preceding element in the display pipe is enabled. If the + * preceding element is a bridge this means it's called after that + * bridge's ->enable function. If the preceding element is a + * &drm_encoder it's called right after the encoder's ->enable, + * ->commit or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The bridge can assume that the display pipe (i.e. clocks and timing + * signals) feeding it is running when this callback is called. This + * callback must enable the display link feeding the next bridge in the + * chain if there is one. + */ void (*enable)(struct drm_bridge *bridge); };

gcc does this for us, and these hooks will be gone soon.

Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/nouveau/nouveau_connector.c | 6 ------ 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 2e7cbe933533..5dd1d0111cac 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -898,8 +898,6 @@ nouveau_connector_helper_funcs = { static const struct drm_connector_funcs nouveau_connector_funcs = { .dpms = drm_helper_connector_dpms, - .save = NULL, - .restore = NULL, .detect = nouveau_connector_detect, .destroy = nouveau_connector_destroy, .fill_modes = drm_helper_probe_single_connector_modes, @@ -910,8 +908,6 @@ nouveau_connector_funcs = { static const struct drm_connector_funcs nouveau_connector_funcs_lvds = { .dpms = drm_helper_connector_dpms, - .save = NULL, - .restore = NULL, .detect = nouveau_connector_detect_lvds, .destroy = nouveau_connector_destroy, .fill_modes = drm_helper_probe_single_connector_modes, @@ -944,8 +940,6 @@ nouveau_connector_dp_dpms(struct drm_connector *connector, int mode) static const struct drm_connector_funcs nouveau_connector_funcs_dp = { .dpms = nouveau_connector_dp_dpms, - .save = NULL, - .restore = NULL, .detect = nouveau_connector_detect, .destroy = nouveau_connector_destroy, .fill_modes = drm_helper_probe_single_connector_modes,

These hooks will be gone soon.

Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/qxl/qxl_display.c | 12 ------------ 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index cddba079197f..ebf7f3946a65 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -876,16 +876,6 @@ static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = { .best_encoder = qxl_best_encoder, };

-static void qxl_conn_save(struct drm_connector *connector) -{ - DRM_DEBUG("\n"); -} - -static void qxl_conn_restore(struct drm_connector *connector) -{ - DRM_DEBUG("\n"); -} - static enum drm_connector_status qxl_conn_detect( struct drm_connector *connector, bool force) @@ -932,8 +922,6 @@ static void qxl_conn_destroy(struct drm_connector *connector)

static const struct drm_connector_funcs qxl_connector_funcs = { .dpms = drm_helper_connector_dpms, - .save = qxl_conn_save, - .restore = qxl_conn_restore, .detect = qxl_conn_detect, .fill_modes = drm_helper_probe_single_connector_modes_nomerge, .set_property = qxl_conn_set_property,

These hooks will be gone soon.

Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/virtio/virtgpu_display.c | 12 ------------ 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 8e6044d7660a..623c98cf024d 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -374,16 +374,6 @@ static const struct drm_connector_helper_funcs virtio_gpu_conn_helper_funcs = { .best_encoder = virtio_gpu_best_encoder, };

-static void virtio_gpu_conn_save(struct drm_connector *connector) -{ - DRM_DEBUG("\n"); -} - -static void virtio_gpu_conn_restore(struct drm_connector *connector) -{ - DRM_DEBUG("\n"); -} - static enum drm_connector_status virtio_gpu_conn_detect( struct drm_connector *connector, bool force) @@ -409,8 +399,6 @@ static void virtio_gpu_conn_destroy(struct drm_connector *connector)

static const struct drm_connector_funcs virtio_gpu_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms, - .save = virtio_gpu_conn_save, - .restore = virtio_gpu_conn_restore, .detect = virtio_gpu_conn_detect, .fill_modes = drm_helper_probe_single_connector_modes_nomerge, .destroy = virtio_gpu_conn_destroy,

These hooks will be gone soon.

Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 16 ---------------- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4 ---- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ---- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 ---- 4 files changed, 28 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index e38db35132ed..d50596153679 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -1331,14 +1331,6 @@ static int vmw_du_update_layout(struct vmw_private *dev_priv, unsigned num, return 0; }

-void vmw_du_crtc_save(struct drm_crtc *crtc) -{ -} - -void vmw_du_crtc_restore(struct drm_crtc *crtc) -{ -} - void vmw_du_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, uint32_t start, uint32_t size) @@ -1360,14 +1352,6 @@ int vmw_du_connector_dpms(struct drm_connector *connector, int mode) return 0; }

-void vmw_du_connector_save(struct drm_connector *connector) -{ -} - -void vmw_du_connector_restore(struct drm_connector *connector) -{ -} - enum drm_connector_status vmw_du_connector_detect(struct drm_connector *connector, bool force) { diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index bb63e4d795fa..c1c09b338cc1 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -295,8 +295,6 @@ static int vmw_ldu_crtc_set_config(struct drm_mode_set *set) }

static struct drm_crtc_funcs vmw_legacy_crtc_funcs = { - .save = vmw_du_crtc_save, - .restore = vmw_du_crtc_restore, .cursor_set = vmw_du_crtc_cursor_set, .cursor_move = vmw_du_crtc_cursor_move, .gamma_set = vmw_du_crtc_gamma_set, @@ -329,8 +327,6 @@ static void vmw_ldu_connector_destroy(struct drm_connector *connector)

static struct drm_connector_funcs vmw_legacy_connector_funcs = { .dpms = vmw_du_connector_dpms, - .save = vmw_du_connector_save, - .restore = vmw_du_connector_restore, .detect = vmw_du_connector_detect, .fill_modes = vmw_du_connector_fill_modes, .set_property = vmw_du_connector_set_property, diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index b96d1ab610c5..26dfed6b0c48 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -531,8 +531,6 @@ out_no_fence: }

static struct drm_crtc_funcs vmw_screen_object_crtc_funcs = { - .save = vmw_du_crtc_save, - .restore = vmw_du_crtc_restore, .cursor_set = vmw_du_crtc_cursor_set, .cursor_move = vmw_du_crtc_cursor_move, .gamma_set = vmw_du_crtc_gamma_set, @@ -565,8 +563,6 @@ static void vmw_sou_connector_destroy(struct drm_connector *connector)

static struct drm_connector_funcs vmw_sou_connector_funcs = { .dpms = vmw_du_connector_dpms, - .save = vmw_du_connector_save, - .restore = vmw_du_connector_restore, .detect = vmw_du_connector_detect, .fill_modes = vmw_du_connector_fill_modes, .set_property = vmw_du_connector_set_property, diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index b1fc1c02792d..05375a8cc129 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -1041,8 +1041,6 @@ out_finish: * Screen Target CRTC dispatch table */ static struct drm_crtc_funcs vmw_stdu_crtc_funcs = { - .save = vmw_du_crtc_save, - .restore = vmw_du_crtc_restore, .cursor_set = vmw_du_crtc_cursor_set, .cursor_move = vmw_du_crtc_cursor_move, .gamma_set = vmw_du_crtc_gamma_set, @@ -1101,8 +1099,6 @@ static void vmw_stdu_connector_destroy(struct drm_connector *connector)

static struct drm_connector_funcs vmw_stdu_connector_funcs = { .dpms = vmw_du_connector_dpms, - .save = vmw_du_connector_save, - .restore = vmw_du_connector_restore, .detect = vmw_du_connector_detect, .fill_modes = vmw_du_connector_fill_modes, .set_property = vmw_du_connector_set_property,

They're not how system suspend/resume should be done with atomic (there's new helpers for that developed by Thierry Redding), and for legacy drivers this really should be a helper hook and not a core one.

But there's not even helper code to use them, and only 2 drivers (which now have their own private hooks) set them. Ditch them.

Saves me typing some kerneldoc, too ;-)

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_crtc.h | 11 ----------- 1 file changed, 11 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 340d750f1364..6c92da82f8c5 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -320,8 +320,6 @@ struct drm_crtc_state {

/** * struct drm_crtc_funcs - control CRTCs for a given device - * @save: save CRTC state - * @restore: restore CRTC state * @cursor_set: setup the cursor * @cursor_set2: setup the cursor with hotspot, superseeds @cursor_set if set * @cursor_move: move the cursor @@ -339,11 +337,6 @@ struct drm_crtc_state { * bus accessors. */ struct drm_crtc_funcs { - /* Save CRTC state */ - void (*save)(struct drm_crtc *crtc); /* suspend? */ - /* Restore CRTC state */ - void (*restore)(struct drm_crtc *crtc); /* resume? */ - /** * @reset: * @@ -626,8 +619,6 @@ struct drm_connector_state { /** * struct drm_connector_funcs - control connectors on a given device * @dpms: set power state - * @save: save connector state - * @restore: restore connector state * @detect: is this connector active? * @fill_modes: fill mode list for this connector * @force: notify the driver that the connector is forced on @@ -638,8 +629,6 @@ struct drm_connector_state { */ struct drm_connector_funcs { int (*dpms)(struct drm_connector *connector, int mode); - void (*save)(struct drm_connector *connector); - void (*restore)(struct drm_connector *connector);

/** * @reset:

Nouveau is the only user, and atomic drivers should do state save/restoring differently. So move it into noveau.

Saves me typing some kerneldoc, too ;-)

Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/dispnv04/dac.c | 7 +++---- drivers/gpu/drm/nouveau/dispnv04/dfp.c | 7 +++---- drivers/gpu/drm/nouveau/dispnv04/disp.c | 34 ++++++++++++------------------- drivers/gpu/drm/nouveau/dispnv04/tvnv04.c | 5 +++-- drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 5 +++-- drivers/gpu/drm/nouveau/nouveau_encoder.h | 3 +++ include/drm/drm_modeset_helper_vtables.h | 4 ---- 7 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/dac.c b/drivers/gpu/drm/nouveau/dispnv04/dac.c index 78cb033bc015..6c442def403d 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/dac.c +++ b/drivers/gpu/drm/nouveau/dispnv04/dac.c @@ -504,8 +504,6 @@ static void nv04_dac_destroy(struct drm_encoder *encoder)

static const struct drm_encoder_helper_funcs nv04_dac_helper_funcs = { .dpms = nv04_dac_dpms, - .save = nv04_dac_save, - .restore = nv04_dac_restore, .mode_fixup = nv04_dac_mode_fixup, .prepare = nv04_dac_prepare, .commit = nv04_dac_commit, @@ -515,8 +513,6 @@ static const struct drm_encoder_helper_funcs nv04_dac_helper_funcs = {

static const struct drm_encoder_helper_funcs nv17_dac_helper_funcs = { .dpms = nv04_dac_dpms, - .save = nv04_dac_save, - .restore = nv04_dac_restore, .mode_fixup = nv04_dac_mode_fixup, .prepare = nv04_dac_prepare, .commit = nv04_dac_commit, @@ -545,6 +541,9 @@ nv04_dac_create(struct drm_connector *connector, struct dcb_output *entry) nv_encoder->dcb = entry; nv_encoder->or = ffs(entry->or) - 1;

+ nv_encoder->enc_save = nv04_dac_save; + nv_encoder->enc_restore = nv04_dac_restore; + if (nv_gf4_disp_arch(dev)) helper = &nv17_dac_helper_funcs; else diff --git a/drivers/gpu/drm/nouveau/dispnv04/dfp.c b/drivers/gpu/drm/nouveau/dispnv04/dfp.c index 429ab5e3025a..4c5fb89d74db 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/dfp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/dfp.c @@ -652,8 +652,6 @@ static void nv04_tmds_slave_init(struct drm_encoder *encoder)

static const struct drm_encoder_helper_funcs nv04_lvds_helper_funcs = { .dpms = nv04_lvds_dpms, - .save = nv04_dfp_save, - .restore = nv04_dfp_restore, .mode_fixup = nv04_dfp_mode_fixup, .prepare = nv04_dfp_prepare, .commit = nv04_dfp_commit, @@ -663,8 +661,6 @@ static const struct drm_encoder_helper_funcs nv04_lvds_helper_funcs = {

static const struct drm_encoder_helper_funcs nv04_tmds_helper_funcs = { .dpms = nv04_tmds_dpms, - .save = nv04_dfp_save, - .restore = nv04_dfp_restore, .mode_fixup = nv04_dfp_mode_fixup, .prepare = nv04_dfp_prepare, .commit = nv04_dfp_commit, @@ -701,6 +697,9 @@ nv04_dfp_create(struct drm_connector *connector, struct dcb_output *entry) if (!nv_encoder) return -ENOMEM;

+ nv_encoder->enc_save = nv04_dfp_save; + nv_encoder->enc_restore = nv04_dfp_restore; + encoder = to_drm_encoder(nv_encoder);

nv_encoder->dcb = entry; diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c index ebd9430e0628..449d92a2175e 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c @@ -39,6 +39,7 @@ nv04_display_create(struct drm_device *dev) struct dcb_table *dcb = &drm->vbios.dcb; struct drm_connector *connector, *ct; struct drm_encoder *encoder; + struct nouveau_encoder *nv_encoder; struct nouveau_crtc *crtc; struct nv04_display *disp; int i, ret; @@ -108,13 +109,10 @@ nv04_display_create(struct drm_device *dev)

/* Save previous state */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) - crtc->save(crtc); + crtc->save(&crtc->base);

- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - const struct drm_encoder_helper_funcs *func = encoder->helper_private; - - func->save(encoder); - } + list_for_each_entry(nv_encoder, &dev->mode_config.encoder_list, base.base.head) + nv_encoder->enc_save(&nv_encoder->base.base);

nouveau_overlay_init(dev);

@@ -126,7 +124,7 @@ nv04_display_destroy(struct drm_device *dev) { struct nv04_display *disp = nv04_display(dev); struct nouveau_drm *drm = nouveau_drm(dev); - struct drm_encoder *encoder; + struct nouveau_encoder *encoder; struct drm_crtc *crtc; struct nouveau_crtc *nv_crtc;

@@ -140,11 +138,8 @@ nv04_display_destroy(struct drm_device *dev) }

/* Restore state */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - const struct drm_encoder_helper_funcs *func = encoder->helper_private; - - func->restore(encoder); - } + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.base.head) + encoder->enc_restore(&encoder->base.base);

list_for_each_entry(nv_crtc, &dev->mode_config.crtc_list, base.head) nv_crtc->restore(crtc); @@ -160,8 +155,8 @@ nv04_display_destroy(struct drm_device *dev) int nv04_display_init(struct drm_device *dev) { - struct drm_encoder *encoder; - struct drm_crtc *crtc; + struct nouveau_encoder *encoder; + struct nouveau_crtc *crtc;

/* meh.. modeset apparently doesn't setup all the regs and depends * on pre-existing state, for now load the state of the card *before* @@ -171,14 +166,11 @@ nv04_display_init(struct drm_device *dev) * save/restore "pre-load" state, but more general so we can save * on suspend too. */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - const struct drm_encoder_helper_funcs *func = encoder->helper_private; - - func->restore(encoder); - } + list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) + crtc->save(&crtc->base);

- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) - crtc->funcs->restore(crtc); + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.base.head) + encoder->enc_save(&encoder->base.base);

return 0; } diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c b/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c index 5345eb5378a8..91d689400d2e 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c @@ -192,8 +192,6 @@ static const struct drm_encoder_funcs nv04_tv_funcs = {

static const struct drm_encoder_helper_funcs nv04_tv_helper_funcs = { .dpms = nv04_tv_dpms, - .save = drm_i2c_encoder_save, - .restore = drm_i2c_encoder_restore, .mode_fixup = drm_i2c_encoder_mode_fixup, .prepare = nv04_tv_prepare, .commit = nv04_tv_commit, @@ -228,6 +226,9 @@ nv04_tv_create(struct drm_connector *connector, struct dcb_output *entry) drm_encoder_init(dev, encoder, &nv04_tv_funcs, DRM_MODE_ENCODER_TVDAC); drm_encoder_helper_add(encoder, &nv04_tv_helper_funcs);

+ nv_encoder->enc_save = drm_i2c_encoder_save; + nv_encoder->enc_restore = drm_i2c_encoder_restore; + encoder->possible_crtcs = entry->heads; encoder->possible_clones = 0; nv_encoder->dcb = entry; diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c index b734195d80a0..ff8c55866b18 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c @@ -771,8 +771,6 @@ static void nv17_tv_destroy(struct drm_encoder *encoder)

static struct drm_encoder_helper_funcs nv17_tv_helper_funcs = { .dpms = nv17_tv_dpms, - .save = nv17_tv_save, - .restore = nv17_tv_restore, .mode_fixup = nv17_tv_mode_fixup, .prepare = nv17_tv_prepare, .commit = nv17_tv_commit, @@ -820,6 +818,9 @@ nv17_tv_create(struct drm_connector *connector, struct dcb_output *entry) drm_encoder_helper_add(encoder, &nv17_tv_helper_funcs); to_encoder_slave(encoder)->slave_funcs = &nv17_tv_slave_funcs;

+ tv_enc->base.enc_save = nv17_tv_save; + tv_enc->base.enc_restore = nv17_tv_restore; + encoder->possible_crtcs = entry->heads; encoder->possible_clones = 0;

diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h index b37da95105b0..c38a86408363 100644 --- a/drivers/gpu/drm/nouveau/nouveau_encoder.h +++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h @@ -63,6 +63,9 @@ struct nouveau_encoder { u32 datarate; } dp; }; + + void (*enc_save)(struct drm_encoder *encoder); + void (*enc_restore)(struct drm_encoder *encoder); };

struct nouveau_encoder * diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 35c5a1c4e7ba..bc731754fc70 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -126,8 +126,6 @@ static inline void drm_crtc_helper_add(struct drm_crtc *crtc, /** * struct drm_encoder_helper_funcs - helper operations for encoders * @dpms: set power state - * @save: save connector state - * @restore: restore connector state * @mode_fixup: try to fixup proposed mode for this connector * @prepare: part of the disable sequence, called before the CRTC modeset * @commit: called after the CRTC modeset @@ -149,8 +147,6 @@ static inline void drm_crtc_helper_add(struct drm_crtc *crtc, */ struct drm_encoder_helper_funcs { void (*dpms)(struct drm_encoder *encoder, int mode); - void (*save)(struct drm_encoder *encoder); - void (*restore)(struct drm_encoder *encoder);

bool (*mode_fixup)(struct drm_encoder *encoder, const struct drm_display_mode *mode,

The special case here is that both ->detect and ->force are actually functions only called by the probe helpers and hence really shouldn't be here. But since they've used by pretty much every driver I figured it's better to just document this for now instead of holding this doc patch hostage until that's all fixed. For that reason also group force right next to detect.

v2: Use FIXME comments to annotate where we should move a hook to helpers.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_crtc.h | 84 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 10 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6c92da82f8c5..fbfe617bc492 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -618,16 +618,28 @@ struct drm_connector_state {

/** * struct drm_connector_funcs - control connectors on a given device - * @dpms: set power state - * @detect: is this connector active? - * @fill_modes: fill mode list for this connector - * @force: notify the driver that the connector is forced on * * Each CRTC may have one or more connectors attached to it. The functions * below allow the core DRM code to control connectors, enumerate available modes, * etc. */ struct drm_connector_funcs { + /** + * @dpms: + * + * Legacy entry point to set the per-connector DPMS state. Legacy DPMS + * is exposed as a standard property on the connector, but diverted to + * this callback in the drm core. Note that atomic drivers don't + * implement the 4 level DPMS support on the connector any more, but + * instead only have an on/off "ACTIVE" property on the CRTC object. + * + * Drivers implementing atomic modeset should use + * drm_atomic_helper_connector_dpms() to implement this hook. + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ int (*dpms)(struct drm_connector *connector, int mode);

/** @@ -642,14 +654,67 @@ struct drm_connector_funcs { */ void (*reset)(struct drm_connector *connector);

- /* Check to see if anything is attached to the connector. - * @force is set to false whilst polling, true when checking the - * connector due to user request. @force can be used by the driver - * to avoid expensive, destructive operations during automated - * probing. + /** + * @detect: + * + * Check to see if anything is attached to the connector. The parameter + * force is set to false whilst polling, true when checking the + * connector due to a user request. force can be used by the driver to + * avoid expensive, destructive operations during automated probing. + * + * FIXME: + * + * Note that this hook is only called by the probe helper. It's not in + * the helper library vtable purely for historical reasons. The only DRM + * core entry point to probe connector state is @fill_modes. + * + * RETURNS: + * + * drm_connector_status indicating the connector's status. */ enum drm_connector_status (*detect)(struct drm_connector *connector, bool force); + + /** + * @force: + * + * This function is called to update internal encoder state when the + * connector is forced to a certain state by userspace, either through + * the sysfs interfaces or on the kernel cmdline. In that case the + * @detect callback isn't called. + * + * FIXME: + * + * Note that this hook is only called by the probe helper. It's not in + * the helper library vtable purely for historical reasons. The only DRM + * core entry point to probe connector state is @fill_modes. + */ + void (*force)(struct drm_connector *connector); + + /** + * @fill_modes: + * + * Entry point for output detection and basic mode validation. The + * driver should reprobe the output if needed (e.g. when hotplug + * handling is unreliable), add all detected modes to connector->modes + * and filter out any the device can't support in any configuration. It + * also needs to filter out any modes wider or higher than the + * parameters max_width and max_height indicate. + * + * The drivers must also prune any modes no longer valid from + * connector->modes. Furthermore it must update connector->status and + * connector->edid. If no EDID has been received for this output + * connector->edid must be NULL. + * + * Drivers using the probe helpers should use + * drm_helper_probe_single_connector_modes() or + * drm_helper_probe_single_connector_modes_nomerge() to implement this + * function. + * + * RETURNS: + * + * The number of modes detected and filled into connector->modes. + */ int (*fill_modes)(struct drm_connector *connector, uint32_t max_width, uint32_t max_height);

/** @@ -679,7 +744,6 @@ struct drm_connector_funcs { * when a connector is being hot-unplugged. */ void (*destroy)(struct drm_connector *connector); - void (*force)(struct drm_connector *connector);

/** * @atomic_duplicate_state:

We always register the DPMS property, it's really a fundamental part of a display driver. So don't check whether the vfunc is there, it's non-optional

Yes I've audited all the almost 100 drm_connector_funcs we have, no one botched this ;-)

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 40ec79e3f0fc..92ca63335a8d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4785,9 +4785,7 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,

/* Do DPMS ourselves */ if (property == connector->dev->mode_config.dpms_property) { - ret = 0; - if (connector->funcs->dpms) - ret = (*connector->funcs->dpms)(connector, (int)value); + ret = (*connector->funcs->dpms)(connector, (int)value); } else if (connector->funcs->set_property) ret = connector->funcs->set_property(connector, property, value);

And merge any docbook we have into the kerneldoc comments.

Since it's a legacy entry point with only two implementation (one each in atomic and legacy crtc helpers) I've made the documentation for set_config fairly sparse - no one should ever need to look at this again, all the ABI we have is baked into code.

For ->page_flip otoh I kept all the extensive docs from the docbook and even extended it where it was lacking: Currently we have a pile of legacy page_flip implemantations, and even for atomic drivers there's not yet a standard implementation in the helpers. Which means every driver needs to implement this itself, and precise specs are really valuable.

Otherwise there's just cursor, which really just boils down to "use at least universal planes". And gamma tables (where we have a bit a mess with the fbdev helper gamma hooks).

v2: Spelling fixes (Eric).

Cc: Eric Anholt eric@anholt.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/DocBook/gpu.tmpl | 117 +---------------------------- include/drm/drm_crtc.h | 164 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 149 insertions(+), 132 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 02c7d44f517c..5312f5a05798 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -1174,121 +1174,6 @@ int max_width, max_height;</synopsis> pointer to CRTC functions. </para> </sect3> - <sect3 id="drm-kms-crtcops"> - <title>CRTC Operations</title> - <sect4> - <title>Set Configuration</title> - <synopsis>int (*set_config)(struct drm_mode_set *set);</synopsis> - <para> - Apply a new CRTC configuration to the device. The configuration - specifies a CRTC, a frame buffer to scan out from, a (x,y) position in - the frame buffer, a display mode and an array of connectors to drive - with the CRTC if possible. - </para> - <para> - If the frame buffer specified in the configuration is NULL, the driver - must detach all encoders connected to the CRTC and all connectors - attached to those encoders and disable them. - </para> - <para> - This operation is called with the mode config lock held. - </para> - <note><para> - Note that the drm core has no notion of restoring the mode setting - state after resume, since all resume handling is in the full - responsibility of the driver. The common mode setting helper library - though provides a helper which can be used for this: - <function>drm_helper_resume_force_mode</function>. - </para></note> - </sect4> - <sect4> - <title>Page Flipping</title> - <synopsis>int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event);</synopsis> - <para> - Schedule a page flip to the given frame buffer for the CRTC. This - operation is called with the mode config mutex held. - </para> - <para> - Page flipping is a synchronization mechanism that replaces the frame - buffer being scanned out by the CRTC with a new frame buffer during - vertical blanking, avoiding tearing. When an application requests a page - flip the DRM core verifies that the new frame buffer is large enough to - be scanned out by the CRTC in the currently configured mode and then - calls the CRTC <methodname>page_flip</methodname> operation with a - pointer to the new frame buffer. - </para> - <para> - The <methodname>page_flip</methodname> operation schedules a page flip. - Once any pending rendering targeting the new frame buffer has - completed, the CRTC will be reprogrammed to display that frame buffer - after the next vertical refresh. The operation must return immediately - without waiting for rendering or page flip to complete and must block - any new rendering to the frame buffer until the page flip completes. - </para> - <para> - If a page flip can be successfully scheduled the driver must set the - <code>drm_crtc-&gt;fb</code> field to the new framebuffer pointed to - by <code>fb</code>. This is important so that the reference counting - on framebuffers stays balanced. - </para> - <para> - If a page flip is already pending, the - <methodname>page_flip</methodname> operation must return - -<errorname>EBUSY</errorname>. - </para> - <para> - To synchronize page flip to vertical blanking the driver will likely - need to enable vertical blanking interrupts. It should call - <function>drm_vblank_get</function> for that purpose, and call - <function>drm_vblank_put</function> after the page flip completes. - </para> - <para> - If the application has requested to be notified when page flip completes - the <methodname>page_flip</methodname> operation will be called with a - non-NULL <parameter>event</parameter> argument pointing to a - <structname>drm_pending_vblank_event</structname> instance. Upon page - flip completion the driver must call <methodname>drm_send_vblank_event</methodname> - to fill in the event and send to wake up any waiting processes. - This can be performed with - <programlisting><![CDATA[ - spin_lock_irqsave(&dev->event_lock, flags); - ... - drm_send_vblank_event(dev, pipe, event); - spin_unlock_irqrestore(&dev->event_lock, flags); - ]]></programlisting> - </para> - <note><para> - FIXME: Could drivers that don't need to wait for rendering to complete - just add the event to <literal>dev-&gt;vblank_event_list</literal> and - let the DRM core handle everything, as for "normal" vertical blanking - events? - </para></note> - <para> - While waiting for the page flip to complete, the - <literal>event-&gt;base.link</literal> list head can be used freely by - the driver to store the pending event in a driver-specific list. - </para> - <para> - If the file handle is closed before the event is signaled, drivers must - take care to destroy the event in their - <methodname>preclose</methodname> operation (and, if needed, call - <function>drm_vblank_put</function>). - </para> - </sect4> - <sect4> - <title>Miscellaneous</title> - <itemizedlist> - <listitem> - <synopsis>void (*gamma_set)(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, - uint32_t start, uint32_t size);</synopsis> - <para> - Apply a gamma table to the device. The operation is optional. - </para> - </listitem> - </itemizedlist> - </sect4> - </sect3> </sect2> <sect2> <title>Planes (struct <structname>drm_plane</structname>)</title> @@ -1305,7 +1190,7 @@ int max_width, max_height;</synopsis> <listitem> DRM_PLANE_TYPE_PRIMARY represents a "main" plane for a CRTC. Primary planes are the planes operated upon by CRTC modesetting and flipping - operations described in <xref linkend="drm-kms-crtcops"/>. + operations described in the page_flip hook in <structname>drm_crtc_funcs</structname>. </listitem> <listitem> DRM_PLANE_TYPE_CURSOR represents a "cursor" plane for a CRTC. Cursor diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fbfe617bc492..72a7e096dd42 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -320,12 +320,6 @@ struct drm_crtc_state {

/** * struct drm_crtc_funcs - control CRTCs for a given device - * @cursor_set: setup the cursor - * @cursor_set2: setup the cursor with hotspot, superseeds @cursor_set if set - * @cursor_move: move the cursor - * @gamma_set: specify color ramp for CRTC - * @set_config: apply a new CRTC configuration - * @page_flip: initiate a page flip * * The drm_crtc_funcs structure is the central CRTC management structure * in the DRM. Each CRTC controls one or more connectors (note that the name @@ -349,15 +343,86 @@ struct drm_crtc_funcs { */ void (*reset)(struct drm_crtc *crtc);

- /* cursor controls */ + /** + * @cursor_set: + * + * Update the cursor image. The cursor position is relative to the CRTC + * and can be partially or fully outside of the visible area. + * + * Note that contrary to all other KMS function the legacy cursor entry + * points don't take a framebuffer object, but instead take directly a + * raw buffer object id from the driver's buffer manager (which is + * either GEM or TTM for current drivers). + * + * This entry point is deprecated, drivers should instead implement + * universal plane support and register a proper cursor plane using + * drm_crtc_init_with_planes(). + * + * This callback is optional + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ int (*cursor_set)(struct drm_crtc *crtc, struct drm_file *file_priv, uint32_t handle, uint32_t width, uint32_t height); + + /** + * @cursor_set2: + * + * Update the cursor image, including hotspot information. The hotspot + * must not affect the cursor position in CRTC coordinates, but is only + * meant as a hint for virtualized display hardware to coordinate the + * guests and hosts cursor position. The cursor hotspot is relative to + * the cursor image. Otherwise this works exactly like @cursor_set. + * + * This entry point is deprecated, drivers should instead implement + * universal plane support and register a proper cursor plane using + * drm_crtc_init_with_planes(). + * + * This callback is optional + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ int (*cursor_set2)(struct drm_crtc *crtc, struct drm_file *file_priv, uint32_t handle, uint32_t width, uint32_t height, int32_t hot_x, int32_t hot_y); + + /** + * @cursor_move: + * + * Update the cursor position. The cursor does not need to be visible + * when this hook is called. + * + * This entry point is deprecated, drivers should instead implement + * universal plane support and register a proper cursor plane using + * drm_crtc_init_with_planes(). + * + * This callback is optional + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ int (*cursor_move)(struct drm_crtc *crtc, int x, int y);

- /* Set gamma on the CRTC */ + /** + * @gamma_set: + * + * Set gamma on the CRTC. + * + * This callback is optional. + * + * NOTE: + * + * Drivers that support gamma tables and also fbdev emulation through + * the provided helper library need to take care to fill out the gamma + * hooks for both. Currently there's a bit an unfortunate duplication + * going on, which should eventually be unified to just one set of + * hooks. + */ void (*gamma_set)(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, uint32_t start, uint32_t size);

@@ -370,16 +435,83 @@ struct drm_crtc_funcs { */ void (*destroy)(struct drm_crtc *crtc);

+ /** + * @set_config: + * + * This is the main legacy entry point to change the modeset state on a + * CRTC. All the details of the desired configuration are passed in a + * struct &drm_mode_set - see there for details. + * + * Drivers implementing atomic modeset should use + * drm_atomic_helper_set_config() to implement this hook. + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ int (*set_config)(struct drm_mode_set *set);

- /* - * Flip to the given framebuffer. This implements the page - * flip ioctl described in drm_mode.h, specifically, the - * implementation must return immediately and block all - * rendering to the current fb until the flip has completed. - * If userspace set the event flag in the ioctl, the event - * argument will point to an event to send back when the flip - * completes, otherwise it will be NULL. + /** + * @page_flip: + * + * Legacy entry point to schedule a flip to the given framebuffer. + * + * Page flipping is a synchronization mechanism that replaces the frame + * buffer being scanned out by the CRTC with a new frame buffer during + * vertical blanking, avoiding tearing (except when requested otherwise + * through the DRM_MODE_PAGE_FLIP_ASYN flag). When an application + * requests a page flip the DRM core verifies that the new frame buffer is large + * enough to be scanned out by the CRTC in the currently configured mode + * and then calls the CRTC page_flip operation with a pointer to the new + * frame buffer. + * + * The driver must wait for any pending rendering to the new framebuffer + * to complete before executing the flip. It should also wait for any + * pending rendering from other drivers if the underlying buffer is a + * shared dma-buf. + * + * An application can request to be notified when the page flip has + * completed. The drm core will supply a struct &drm_event in the event + * parameter in this case. This can be handled by the + * drm_crtc_send_vblank_event() function, which the driver should call on + * the provided event upon completion of the flip. Note that if + * the driver supports vblank signalling and timestamping the vblank + * counters and timestamps must agree with the ones returned from page + * flip events. With the current vblank helper infrastructure this can + * be achieved by holding a vblank reference while the page flip is + * pending, acquired through drm_crtc_vblank_get() and released with + * drm_crtc_vblank_put(). Drivers are free to implement their own vblank + * counter and timestamp tracking though, e.g. if they have accurate + * timestamp registers in hardware. + * + * FIXME: + * + * Up to that point drivers need to manage events themselves and can use + * even->base.list freely for that. Specifically they need to ensure + * that they don't send out page flip (or vblank) events for which the + * corresponding drm file has been closed already. The drm core + * unfortunately does not (yet) take care of that. Therefore drivers + * currently must clean up and release pending events in their + * ->preclose driver function. + * + * This callback is optional. + * + * NOTE: + * + * Very early versions of the KMS ABI mandated that the driver must + * block (but not reject) any rendering to the old framebuffer until the + * flip operation has completed and the old framebuffer is not longer + * visible. This requirement has been lifted, and userspace is instead + * expected to request delivery of a event and wait with recycling old + * buffers until such has been received. + * + * RETURNS: + * + * 0 on success or a negative error code on failure. Note that if a + * page_flip operation is already pending the callback should return + * -EBUSY. Pageflips on a disabled CRTC (either by setting a NULL mode + * or just runtime disabled through DPMS respectively the new atomic + * "ACTIVE" state) should result in an -EINVAL error code. */ int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb,

While typing these I noticed that ->dirty is a bit a can of worms and even supports blt/fill semantics ... shocked me a bit.

Oh well it's defined in a way that nothing bad (just a bit of inefficiency) will happen for drivers which supports this. So I didn't bother copying the detailed spec into the new kerneldoc.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- include/drm/drm_crtc.h | 50 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 72a7e096dd42..551484fb55e5 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -162,23 +162,55 @@ struct drm_tile_group { u8 group_data[8]; };

+/** + * struct drm_framebuffer_funcs - framebuffer hooks + */ struct drm_framebuffer_funcs { - /* note: use drm_framebuffer_remove() */ + /** + * @destroy: + * + * Clean up framebuffer resources, specifically also unreference the + * backing storage. The core guarantees to call this function for every + * framebuffer succesfully created by fb_create in + * &drm_mode_config_funcs. + */ void (*destroy)(struct drm_framebuffer *framebuffer); + + /** + * @create_handle: + * + * Create a buffer handle in the driver-specific buffer manager (either + * GEM or TTM) valid for the passed-in struct &drm_file. This is used by + * the core to implement the GETFB ioctl, which returns (for + * sufficiently priviledged user) also a native buffer handle. This can + * be used for seamless transitions between modesetting clients by + * copying the current screen contents to a private buffer and blending + * between that and the new contents. + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ int (*create_handle)(struct drm_framebuffer *fb, struct drm_file *file_priv, unsigned int *handle); - /* + /** + * @dirty: + * * Optional callback for the dirty fb ioctl. * - * Userspace can notify the driver via this callback - * that a area of the framebuffer has changed and should - * be flushed to the display hardware. + * Userspace can notify the driver via this callback that a area of the + * framebuffer has changed and should be flushed to the display + * hardware. This can also be used internally, e.g. by the fbdev + * emulation, though that's not (yet) the case currently. + * + * See documentation in drm_mode.h for the struct drm_mode_fb_dirty_cmd + * for more information as all the semantics and arguments have a one to + * one mapping on this function. * - * See documentation in drm_mode.h for the struct - * drm_mode_fb_dirty_cmd for more information as all - * the semantics and arguments have a one to one mapping - * on this function. + * RETURNS: + * + * 0 on success or a negative error code on failure. */ int (*dirty)(struct drm_framebuffer *framebuffer, struct drm_file *file_priv, unsigned flags,

The meat here is definitely the detailed specs for what atomic_check and atomic_commit are supposed to do.

And another candidate for a core vfunc that should be in a helper really (output_poll_changed this time around).

v2: Feedback from Eric on irc: - spelling fixes. - spec what async should do - copy the event related paragraphs from page_flip and adjust - make it clear that a successful async commit is not allowed to leave the pipe dead or disabled.

v3: Use FIXME comments to annotate functions that we should move to some helpers.

Cc: Eric Anholt eric@anholt.net Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- include/drm/drm_crtc.h | 241 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 233 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 551484fb55e5..37cff64a26ef 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1715,31 +1715,256 @@ struct drm_mode_set {

/** * struct drm_mode_config_funcs - basic driver provided mode setting functions - * @fb_create: create a new framebuffer object - * @output_poll_changed: function to handle output configuration changes - * @atomic_check: check whether a given atomic state update is possible - * @atomic_commit: commit an atomic state update previously verified with - * atomic_check() - * @atomic_state_alloc: allocate a new atomic state - * @atomic_state_clear: clear the atomic state - * @atomic_state_free: free the atomic state * * Some global (i.e. not per-CRTC, connector, etc) mode setting functions that * involve drivers. */ struct drm_mode_config_funcs { + /** + * @fb_create: + * + * Create a new framebuffer object. The core does basic checks on the + * requested metadata, but most of that is left to the driver. See + * struct &drm_mode_fb_cmd2 for details. + * + * RETURNS: + * + * A new framebuffer with an initial refernce count of 1 or a negative + * error code encoded with ERR_PTR(). + */ struct drm_framebuffer *(*fb_create)(struct drm_device *dev, struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd); + + /** + * @output_poll_changed: + * + * Callback used by helpers to inform the driver of output configuration + * changes. + * + * Drivers implementing fbdev emulation with the helpers can call + * drm_fb_helper_hotplug_changed from this hook to inform the fbdev + * helper of output changes. + * + * FIXME: + * + * Except that there's no vtable for device-level helper callbacks + * there's no reason this is a core function. + */ void (*output_poll_changed)(struct drm_device *dev);

+ /** + * @atomic_check: + * + * This is the only hook to validate an atomic modeset update. This + * function must reject any modeset and state changes which the hardware + * or driver doesn't support. This includes but is of course not limited + * to: + * + * - Checking that the modes, framebuffers, scaling and placement + * requirements and so on are within the limits of the hardware. + * + * - Checking that any hidden shared resources are not oversubscribed. + * This can be shared PLLs, shared lanes, overall memory bandwidth, + * display fifo space (where shared between planes or maybe even + * CRTCs). + * + * - Checking that virtualized resources exported to userspace are not + * oversubscribed. For various reasons it can make sense to expose + * more planes, crtcs or encoders than which are physically there. One + * example is dual-pipe operations (which generally should be hidden + * from userspace if when lockstepped in hardware, otherwise exposed), + * where a plane might need 1 hardware plane (if it's just on one + * pipe), 2 hardware planes (when it spans both pipes) or maybe even + * shared a hardware plane with a 2nd plane (if there's a compatible + * plane requested on the area handled by the other pipe). + * + * - Check that any transitional state is possible and that if + * requested, the update can indeed be done in the vblank period + * without temporarily disabling some functions. + * + * - Check any other constraints the driver or hardware might have. + * + * - This callback also needs to correctly fill out the &drm_crtc_state + * in this update to make sure that drm_atomic_crtc_needs_modeset() + * reflects the nature of the possible update and returns true if and + * only if the update cannot be applied without tearing within one + * vblank on that CRTC. The core uses that information to reject + * updates which require a full modeset (i.e. blanking the screen, or + * at least pausing updates for a substantial amount of time) if + * userspace has disallowed that in its request. + * + * - The driver also does not need to repeat basic input validation + * like done for the corresponding legacy entry points. The core does + * that before calling this hook. + * + * See the documentation of @atomic_commit for an exhaustive list of + * error conditions which are allowed to not be checked in this + * callback. + * + * See the documentation for struct &drm_atomic_state for how exactly + * an atomic modeset update is described. + * + * Drivers using the atomic helpers can implement this hook using + * drm_atomic_helper_check(), or one of the exported sub-functions of + * it. + * + * RETURNS: + * + * 0 on success or one of the below negative error codes: + * + * - -EINVAL, if any of the above constraints are violated. + * + * - -EDEADLK, when returned from an attempt to acquire an additional + * &drm_modeset_lock through drm_modeset_lock(). + * + * - -ENOMEM, if allocating additional state sub-structures failed due + * to lack of memory. + * + * - -EINTR, -EAGAIN or -ERESTARTSYS, if the ioctl should be restarted. + * This can either be due to a pending signal, or because the driver + * needs to completely bail out to recover from an exceptional + * situation like a gpu hang. From a userspace point all errors are + * treated equally. + */ int (*atomic_check)(struct drm_device *dev, struct drm_atomic_state *a); + + /** + * @atomic_commit: + * + * This is the only hook to commit an atomic modeset update. The core + * guarantees that @atomic_check has been called successfully before + * calling this function, and that nothing has been changed in the + * interim. + * + * See the documentation for struct &drm_atomic_state for how exactly + * an atomic modeset update is described. + * + * Drivers using the atomic helpers can implement this hook using + * drm_atomic_helper_commit(), or one of the exported sub-functions of + * it. + * + * Asynchronous commits (as indicated with the async parameter) must + * do any preparatory work which might result in an unsuccessful commit + * in the context of this callback. The only exception is hardware + * errors resulting in -EIO. But even in that case the driver must + * ensure that the display pipe is at least running, to avoid + * compositors crashing when pageflips don't work. Anything else, + * specifically committing the update to the hardware, should be done + * without blocking the caller. For updates which do not require a + * modeset this must be guaranteed. + * + * The driver must wait for any pending rendering to the new + * framebuffers to complete before executing the flip. It should also + * wait for any pending rendering from other drivers if the underlying + * buffer is a shared dma-buf. Asynchronous commits must not wait for + * rendering in the context of this callback. + * + * An application can request to be notified when the atomic commit has + * completed. These events are per-CRTC and can be distinguished by the + * CRTC index supplied in &drm_event to userspace. + * + * The drm core will supply a struct &drm_event in the event + * member of each CRTC's &drm_crtc_state structure. This can be handled by the + * drm_crtc_send_vblank_event() function, which the driver should call on + * the provided event upon completion of the atomic commit. Note that if + * the driver supports vblank signalling and timestamping the vblank + * counters and timestamps must agree with the ones returned from page + * flip events. With the current vblank helper infrastructure this can + * be achieved by holding a vblank reference while the page flip is + * pending, acquired through drm_crtc_vblank_get() and released with + * drm_crtc_vblank_put(). Drivers are free to implement their own vblank + * counter and timestamp tracking though, e.g. if they have accurate + * timestamp registers in hardware. + * + * NOTE: + * + * Drivers are not allowed to shut down any display pipe successfully + * enabled through an atomic commit on their own. Doing so can result in + * compositors crashing if a page flip is suddenly reject because the + * pipe is off. + * + * RETURNS: + * + * 0 on success or one of the below negative error codes: + * + * - -EBUSY, if an asynchronous updated is requested and there is + * currently an earlier updated still pending. Drivers are allowed to + * support a queue of outstanding updates, but currently no driver + * supports that. Note that drivers must wait for preceding updates + * to complete if a synchronous update is requested, they are not + * allowed to fail the commit in that case. + * + * - -ENOMEM, if the driver failed to allocate memory. Specifically + * this can happen when trying to pin framebuffers, which must only + * be done when committing the state. + * + * - -ENOSPC, as a refinement of the more generic -ENOMEM to indicate + * that the driver has run out of vram, iommu space or similar gpu + * address space needed for framebuffer. + * + * - -EIO, if the hardware completely died. + * + * - -EINTR, -EAGAIN or -ERESTARTSYS, if the ioctl should be restarted. + * This can either be due to a pending signal, or because the driver + * needs to completely bail out to recover from an exceptional + * situation like a gpu hang. From a userspace point of view all errors are + * treated equally. + * + * This list is exhaustive. Specifically this hook is not allowed to + * return -EINVAL (any invalid requests should be caught in + * @atomic_check) or -EDEADLK (this function must not acquire + * additional modeset locks). The core will also reject any async + * atomic flips with -EINVAL already (for matching semantics in this + * case with legacy page flips). + */ int (*atomic_commit)(struct drm_device *dev, struct drm_atomic_state *a, bool async); + + /** + * @atomic_state_alloc: + * + * This optional hook can be used by drivers who want to subclass struct + * &drm_atomic_state to be able to track their own driver-private global + * state easily. If this hook is implemented, drivers must also + * implement @atomic_state_clear and @atomic_state_free. + * + * RETURNS: + * + * A new &drm_atomic_state on success or NULL on failure. + */ struct drm_atomic_state *(*atomic_state_alloc)(struct drm_device *dev); + + /** + * @atomic_state_clear: + * + * This hook must clear any driver private state duplicated into the + * passed-in &drm_atomic_state. This hook is called when the caller + * encountered a &drm_modeset_lock deadlock and needs to drop all + * already acquired locks as part of the deadlock avoidance dance + * implemented in drm_modeset_lock_backoff(). + * + * Any duplicated state must be invalidated since a concurrent atomic + * update might change it, and the drm atomic interfaces always apply + * updates as relative changes to the current state. + * + * Drivers who implement this must call drm_atomic_state_default_clear() + * to clear common state. + */ void (*atomic_state_clear)(struct drm_atomic_state *state); + + /** + * @atomic_state_free: + * + * This hook needs driver private resources and the &drm_atomic_state + * itself. Note that the core first calls drm_atomic_state_clear to + * avoid code duplicate between the clear and free hooks. + * + * Drivers who implement this must call drm_atomic_state_default_free() + * to release common resources. + */ void (*atomic_state_free)(struct drm_atomic_state *state); };

Plus related hooks used to do atomic plane updates since they only really make sense as a package.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_modeset_helper_vtables.h | 209 +++++++++++++++++++++++++++++-- 1 file changed, 198 insertions(+), 11 deletions(-)

diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index bc731754fc70..66b78c14154e 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -57,9 +57,6 @@ enum mode_set_atomic; * @load_lut: load color palette * @disable: disable CRTC when no longer in use * @enable: enable CRTC - * @atomic_check: check for validity of an atomic state - * @atomic_begin: begin atomic update - * @atomic_flush: flush atomic update * * The helper operations are called by the mid-layer CRTC helper. * @@ -103,11 +100,95 @@ struct drm_crtc_helper_funcs { void (*disable)(struct drm_crtc *crtc); void (*enable)(struct drm_crtc *crtc);

- /* atomic helpers */ + /** + * @atomic_check: + * + * Drivers should check plane-update related CRTC constraints in this + * hook. They can also check mode related limitations but need to be + * aware of the calling order, since this hook is used by + * drm_atomic_helper_check_planes() whereas the prepartions needed to + * check output routing and the display mode is done in + * drm_atomic_helper_check_modeset(). Therefore drivers who want to + * check output routing and display mode contstraints in this callback + * mus ensure that drm_atomic_helper_check_modeset() has been called + * beforehand. + * + * When using drm_atomic_helper_check_planes CRTC's ->atomic_check() + * hooks are called after the ones for palnes, which allows drivers to + * assign shared resources requested by planes in the CRTC callback + * here. For more complicated depencies the driver can call the provided + * check helpers multiple times until the computed state has a final + * configuration and everything has been checked. + * + * This function is also allowed to inspect any other object's state and + * can add more state objects to the atomic commit if needed. Care must + * be taken though to ensure that state check&compute functions for + * these added states are all called, and derived state in other objects + * all updated. Again the recommendation is to just call check helpers + * until a maximal configuration is reached. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + * + * NOTE: + * + * This function is called in the check phase of an atomic update. The + * driver is not allowed to change anything outside of the free-standing + * state objects passed-in or assembled in the overall &drm_atomic_state + * update tracking structure. + * + * RETURNS: + * + * 0 on success, -EINVAL if the state or the transition can't be + * support, -ENOMEM on memory allocation failure and -EDEADLK if an + * attempt to obtain another state object ran into a &drm_modeset_lock + * deadlock. + */ int (*atomic_check)(struct drm_crtc *crtc, struct drm_crtc_state *state); + + /** + * @atomic_begin: + * + * Drivers should prepare for an atomic update of multiple planes on + * a CRTC in this hook. Depending upon hardware this might be vblank + * evasion, blocking updates by setting bits or doing prepatory work for + * e.g. manual update display. + * + * This hook is called before any plane commit functions are called. + * + * Note that the power state of the display pipe when this function is + * called depends upon the exact helpers and calling sequence the driver + * has picked. See drm_atomic_commit_planes for a discussion of the + * tradeoffs and variants of plane commit helpers. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + */ void (*atomic_begin)(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state); + /** + * @atomic_flush: + * + * Drivers should prepare for an atomic update of multiple planes on + * a CRTC in this hook. Depending upon hardware this might include + * checking that vblank evasion was successful, unblocking updates by + * setting bits or setting the GO bit to flush out all updates. + * + * Simple hardware or hardware with special requirements can commit and + * flush out all updates for all planes from this hook and forgo all the + * other commit hooks for plane updates. + * + * This hook is called after any plane commit functions are called. + * + * Note that the power state of the display pipe when this function is + * called depends upon the exact helpers and calling sequence the driver + * has picked. See drm_atomic_commit_planes for a discussion of the + * tradeoffs and variants of plane commit helpers. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + */ void (*atomic_flush)(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state); }; @@ -211,25 +292,131 @@ static inline void drm_connector_helper_add(struct drm_connector *connector, }

/** - * struct drm_plane_helper_funcs - helper operations for CRTCs - * @prepare_fb: prepare a framebuffer for use by the plane - * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane - * @atomic_check: check that a given atomic state is valid and can be applied - * @atomic_update: apply an atomic state to the plane (mandatory) - * @atomic_disable: disable the plane + * struct drm_plane_helper_funcs - helper operations for planes * - * The helper operations are called by the mid-layer CRTC helper. + * These functions are used by the atomic helpers and by the transitional plane + * helpers. */ struct drm_plane_helper_funcs { + /** + * @prepare_fb: + * + * This hook is to prepare a framebuffer for scanout by e.g. pinning + * it's backing storage or relocating it into a contiguous block of + * vram. Other possible preparatory work includes flushing caches. + * + * This function must not block for outstanding rendering, since it is + * called in the context of the atomic IOCTL even for async commits to + * be able to return any errors to userspace. Instead the recommended + * why is to fill out the fence member of the passed-in + * &drm_plane_state. If the driver doesn't support native fences then + * equivalent functionality should be implemented through private + * members in the plane structure. + * + * The helpers will call @cleanup_fb with matching arguments for every + * successful call to this hook. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + * + * RETURNS: + * + * 0 on sucess or one of the following negative error codes allowed by + * the atomic_commit hook in &drm_mode_config_funcs. When using helpers + * this callback is the only one which can fail an atomic commit, + * everything else must complete successfully. + */ int (*prepare_fb)(struct drm_plane *plane, const struct drm_plane_state *new_state); + /** + * @cleanup_fb: + * + * This hook is called to clean up any resources allocated for the given + * framebuffer and plane configuration in @prepare_fb. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + */ void (*cleanup_fb)(struct drm_plane *plane, const struct drm_plane_state *old_state);

+ /** + * @atomic_check: + * + * Drivers should check plane specific constraints in this hook. + * + * When using drm_atomic_helper_check_planes plane's ->atomic_check() + * hooks are called before the ones for CRTCs, which allows drivers to + * request shared resources that the CRTC controls here. For more + * complicated depencies the driver can call the provided check helpers + * multiple times until the computed state has a final configuration and + * everything has been checked. + * + * This function is also allowed to inspect any other object's state and + * can add more state objects to the atomic commit if needed. Care must + * be taken though to ensure that state check&compute functions for + * these added states are all called, and derived state in other objects + * all updated. Again the recommendation is to just call check helpers + * until a maximal configuration is reached. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + * + * NOTE: + * + * This function is called in the check phase of an atomic update. The + * driver is not allowed to change anything outside of the free-standing + * state objects passed-in or assembled in the overall &drm_atomic_state + * update tracking structure. + * + * RETURNS: + * + * 0 on success, -EINVAL if the state or the transition can't be + * support, -ENOMEM on memory allocation failure and -EDEADLK if an + * attempt to obtain another state object ran into a &drm_modeset_lock + * deadlock. + */ int (*atomic_check)(struct drm_plane *plane, struct drm_plane_state *state); + + /** + * @atomic_update: + * + * Drivers should use this function to update the plane state. This + * hook is called in-between the ->atomic_begin and + * ->atomic_flush() of &drm_crtc_helper_funcs. + * + * Note that the power state of the display pipe when this function is + * called depends upon the exact helpers and calling sequence the driver + * has picked. See drm_atomic_commit_planes for a discussion of the + * tradeoffs and variants of plane commit helpers. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + */ void (*atomic_update)(struct drm_plane *plane, struct drm_plane_state *old_state); + /** + * @atomic_disable: + * + * Drivers should use this function to unconditionally disable a plane. + * This hook is called in-between the ->atomic_begin and + * ->atomic_flush() of &drm_crtc_helper_funcs. It is an alternative to + * @atomic_update, which will be called for disabling planes, too, if + * the @atomic_disable hook isn't implemented. + * + * This hook is also useful to disable planes in preration of a modeset, + * by calling drm_atomic_helper_disable_planes_on_crtc() from the + * ->disable hook in &drm_crtc_helper_funcs. + * + * Note that the power state of the display pipe when this function is + * called depends upon the exact helpers and calling sequence the driver + * has picked. See drm_atomic_commit_planes for a discussion of the + * tradeoffs and variants of plane commit helpers. + * + * This callback is used by the atomic modeset helpers and by the + * transitional plane helpers, but it is optional. + */ void (*atomic_disable)(struct drm_plane *plane, struct drm_plane_state *old_state); };