| Message ID | 1370332715-1131-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ville, Thank you for the patch. On Tuesday 04 June 2013 10:58:35 ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index f00ba75..f1f11e1 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -795,6 +795,21 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > } > EXPORT_SYMBOL(drm_encoder_cleanup); > > +/** > + * drm_plane_init - Initialise a new plane object > + * @dev: DRM device > + * @plane: plane object to init > + * @possible_crtcs: bitmask of possible CRTCs > + * @funcs: callbacks for the new plane > + * @formats: array of supported formats (%DRM_FORMAT_*) > + * @format_count: number of elements in @formats > + * @priv: plane is private (hidden from userspace)? > + * > + * Inits a new object created as base part of an driver plane object. s/an driver/a driver/ > + * > + * RETURNS: > + * Zero on success, error code on failure. > + */ > int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > unsigned long possible_crtcs, > const struct drm_plane_funcs *funcs, > @@ -843,6 +858,13 @@ int drm_plane_init(struct drm_device *dev, struct > drm_plane *plane, } > EXPORT_SYMBOL(drm_plane_init); > > +/** > + * drm_plane_cleanup - Cleans up the core plane usage. Nitpicking, you could remove the full stop at the end of the line to be consistent with the other two kerneldoc blocks. And s/Cleans/Clean/ > + * @plane: plane to cleanup > + * > + * Cleanup @plane. Removes from drm modesetting space > + * does NOT free object, caller does that. As this is documentation, I'd use a more verbose style. This function clean up @plane and removes it from the DRM mode setting core. Note that the function does *not* free the plane structure itself, this is the responsibility of the caller. > + */ > void drm_plane_cleanup(struct drm_plane *plane) > { > struct drm_device *dev = plane->dev; > @@ -859,6 +881,15 @@ void drm_plane_cleanup(struct drm_plane *plane) > } > EXPORT_SYMBOL(drm_plane_cleanup); > > +/** > + * drm_plane_force_disable - Forcibly disable a plane > + * @plane: plane to disable > + * > + * Forces the plane to be disabled. This feels a bit unclear to me. In particular, how is "force_disable" different from just disabling the plane ? Maybe the function should be renamed to drm_plane_disable(), and the documentation updated to mention that the function just disables the plane and disassociate with from its frame buffer. > + * > + * Used when the plane's current framebuffer is destroyed, > + * and when restoring fbdev mode. > + */ > void drm_plane_force_disable(struct drm_plane *plane) > { > int ret;
On Wed, Jun 05, 2013 at 04:13:01AM +0200, Laurent Pinchart wrote: > Hi Ville, > > Thank you for the patch. > > On Tuesday 04 June 2013 10:58:35 ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index f00ba75..f1f11e1 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -795,6 +795,21 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > > } > > EXPORT_SYMBOL(drm_encoder_cleanup); > > > > +/** > > + * drm_plane_init - Initialise a new plane object > > + * @dev: DRM device > > + * @plane: plane object to init > > + * @possible_crtcs: bitmask of possible CRTCs > > + * @funcs: callbacks for the new plane > > + * @formats: array of supported formats (%DRM_FORMAT_*) > > + * @format_count: number of elements in @formats > > + * @priv: plane is private (hidden from userspace)? > > + * > > + * Inits a new object created as base part of an driver plane object. > > s/an driver/a driver/ You can blame the guy who wrote the drm_crtc_init() docs :) > > > + * > > + * RETURNS: > > + * Zero on success, error code on failure. > > + */ > > int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > > unsigned long possible_crtcs, > > const struct drm_plane_funcs *funcs, > > @@ -843,6 +858,13 @@ int drm_plane_init(struct drm_device *dev, struct > > drm_plane *plane, } > > EXPORT_SYMBOL(drm_plane_init); > > > > +/** > > + * drm_plane_cleanup - Cleans up the core plane usage. > > Nitpicking, you could remove the full stop at the end of the line to be > consistent with the other two kerneldoc blocks. > > And s/Cleans/Clean/ Same deal here. I'll fix up the originals as well... > > > + * @plane: plane to cleanup > > + * > > + * Cleanup @plane. Removes from drm modesetting space > > + * does NOT free object, caller does that. > > As this is documentation, I'd use a more verbose style. > > This function clean up @plane and removes it from the DRM mode setting core. > Note that the function does *not* free the plane structure itself, this is the > responsibility of the caller. Again just copy-pasted from somewhere. > > > + */ > > void drm_plane_cleanup(struct drm_plane *plane) > > { > > struct drm_device *dev = plane->dev; > > @@ -859,6 +881,15 @@ void drm_plane_cleanup(struct drm_plane *plane) > > } > > EXPORT_SYMBOL(drm_plane_cleanup); > > > > +/** > > + * drm_plane_force_disable - Forcibly disable a plane > > + * @plane: plane to disable > > + * > > + * Forces the plane to be disabled. > > This feels a bit unclear to me. In particular, how is "force_disable" > different from just disabling the plane ? Maybe the function should be renamed > to drm_plane_disable(), and the documentation updated to mention that the > function just disables the plane and disassociate with from its frame buffer. Normal disable would happen in response to the setplane ioctl w/ NULL fb, whereas this guy is meant more for unsolicited disable. I'm afraid if I call it drm_plane_disable() someone will send a patch to call it from setplane, or people start to call it from drivers' disable_plane hook.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f00ba75..f1f11e1 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -795,6 +795,21 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) } EXPORT_SYMBOL(drm_encoder_cleanup); +/** + * drm_plane_init - Initialise a new plane object + * @dev: DRM device + * @plane: plane object to init + * @possible_crtcs: bitmask of possible CRTCs + * @funcs: callbacks for the new plane + * @formats: array of supported formats (%DRM_FORMAT_*) + * @format_count: number of elements in @formats + * @priv: plane is private (hidden from userspace)? + * + * Inits a new object created as base part of an driver plane object. + * + * RETURNS: + * Zero on success, error code on failure. + */ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, unsigned long possible_crtcs, const struct drm_plane_funcs *funcs, @@ -843,6 +858,13 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, } EXPORT_SYMBOL(drm_plane_init); +/** + * drm_plane_cleanup - Cleans up the core plane usage. + * @plane: plane to cleanup + * + * Cleanup @plane. Removes from drm modesetting space + * does NOT free object, caller does that. + */ void drm_plane_cleanup(struct drm_plane *plane) { struct drm_device *dev = plane->dev; @@ -859,6 +881,15 @@ void drm_plane_cleanup(struct drm_plane *plane) } EXPORT_SYMBOL(drm_plane_cleanup); +/** + * drm_plane_force_disable - Forcibly disable a plane + * @plane: plane to disable + * + * Forces the plane to be disabled. + * + * Used when the plane's current framebuffer is destroyed, + * and when restoring fbdev mode. + */ void drm_plane_force_disable(struct drm_plane *plane) { int ret;