[01/28] drm: Polish fbdev helper struct docs
diff mbox

Message ID 1449218769-16577-2-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Dec. 4, 2015, 8:45 a.m. UTC
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(-)

Comments

Thierry Reding Dec. 7, 2015, 10:45 a.m. UTC | #1
On Fri, Dec 04, 2015 at 09:45:42AM +0100, Daniel Vetter wrote:
> 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);

Pardon my ignorance, but is there a way to document parameters with this
new syntax?

> +	/**
> +	 * @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);

Nit: While at it, perhaps (here and in the @gamma_set documentation):
s/lut/LUT/ and s/crtc/CRTC/?

>  
> +	/**
> +	 * @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);

Nit: s/drm/DRM/

>  /**
> - * 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

s/kms/KMS

>   * @fb:  Scanout framebuffer object
>   * @dev:  DRM device

There seems to be an extra space between the : and the description. That
was already there, but maybe worth a follow-up.

>   * @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.

s/drm/DRM/, s/kms/KMS/

Otherwise looks really good.

Thierry
Daniel Vetter Dec. 7, 2015, 11:50 a.m. UTC | #2
On Mon, Dec 07, 2015 at 11:45:22AM +0100, Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 09:45:42AM +0100, Daniel Vetter wrote:
> > 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);
> 
> Pardon my ignorance, but is there a way to document parameters with this
> new syntax?

Unfortunately not. Doing that would be quite a bit more rework of the
entire kerneldoc toolchain I think.
> 
> > +	/**
> > +	 * @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);
> 
> Nit: While at it, perhaps (here and in the @gamma_set documentation):
> s/lut/LUT/ and s/crtc/CRTC/?

Yeah I thought about our inconsistency wrt upper-case of abbrevations in
the docs. I think we should do this as a trivial patch thing for newbies.

> > +	/**
> > +	 * @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);
> 
> Nit: s/drm/DRM/
> 
> >  /**
> > - * 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
> 
> s/kms/KMS
> 
> >   * @fb:  Scanout framebuffer object
> >   * @dev:  DRM device
> 
> There seems to be an extra space between the : and the description. That
> was already there, but maybe worth a follow-up.

I think fix that up while applying, same for the others.
-Daniel

> 
> >   * @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.
> 
> s/drm/DRM/, s/kms/KMS/
> 
> Otherwise looks really good.
> 
> Thierry
Thierry Reding Dec. 7, 2015, 11:53 a.m. UTC | #3
On Mon, Dec 07, 2015 at 12:50:24PM +0100, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 11:45:22AM +0100, Thierry Reding wrote:
> > On Fri, Dec 04, 2015 at 09:45:42AM +0100, Daniel Vetter wrote:
> > > 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);
> > 
> > Pardon my ignorance, but is there a way to document parameters with this
> > new syntax?
> 
> Unfortunately not. Doing that would be quite a bit more rework of the
> entire kerneldoc toolchain I think.

Yes, that was my suspicion as well.

> > > +	/**
> > > +	 * @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);
> > 
> > Nit: While at it, perhaps (here and in the @gamma_set documentation):
> > s/lut/LUT/ and s/crtc/CRTC/?
> 
> Yeah I thought about our inconsistency wrt upper-case of abbrevations in
> the docs. I think we should do this as a trivial patch thing for newbies.

Fair enough.

Thierry

> > >   * @fb:  Scanout framebuffer object
> > >   * @dev:  DRM device
> > 
> > There seems to be an extra space between the : and the description. That
> > was already there, but maybe worth a follow-up.
> 
> I think fix that up while applying, same for the others.

Okay, either way, this is a good improvement, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

Patch
diff mbox

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;
 
 	/**