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@xxxxxxxxx>
> > ---
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
