[PATCH 0/3] drm: fbdev mode restoration improvements v3

[PATCH 0/3] drm: fbdev mode restoration improvements v3

[ville.syrjala]

Third time's the charm maybe.

Changes from v2:
 - Move the plane->fb NULL check into drm_plane_force_disable()
 - Cursors/planes are now disabled by drm_fb_helper directly, so no
   need for the new hook
 - Had to fix up vmwgfx not to look at file_priv in cursor_set when
   handle is 0

[PATCH v2 1/3] drm: Add drm_plane_force_disable()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_plane_force_disable() will forcibly disable the plane even if user
had previously requested the plane to be enabled.

This can be used to force planes to be off when restoring the fbdev
mode.

The code was simply pulled from drm_framebuffer_remove(), which now
calls the new function as well.

v2: Check plane->fb in drm_plane_force_disable(), drop bogus comment
    about disabling crtc

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 29 +++++++++++++++++++----------
 include/drm/drm_crtc.h     |  1 +
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e7e9242..865ebfe 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -569,16 +569,8 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 		}
 
 		list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-			if (plane->fb == fb) {
-				/* should turn off the crtc */
-				ret = plane->funcs->disable_plane(plane);
-				if (ret)
-					DRM_ERROR("failed to disable plane with busy fb\n");
-				/* disconnect the plane from the fb and crtc: */
-				__drm_framebuffer_unreference(plane->fb);
-				plane->fb = NULL;
-				plane->crtc = NULL;
-			}
+			if (plane->fb == fb)
+				drm_plane_force_disable(plane);
 		}
 		drm_modeset_unlock_all(dev);
 	}
@@ -867,6 +859,23 @@ void drm_plane_cleanup(struct drm_plane *plane)
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
 
+void drm_plane_force_disable(struct drm_plane *plane)
+{
+	int ret;
+
+	if (!plane->fb)
+		return;
+
+	ret = plane->funcs->disable_plane(plane);
+	if (ret)
+		DRM_ERROR("failed to disable plane with busy fb\n");
+	/* disconnect the plane from the fb and crtc: */
+	__drm_framebuffer_unreference(plane->fb);
+	plane->fb = NULL;
+	plane->crtc = NULL;
+}
+EXPORT_SYMBOL(drm_plane_force_disable);
+
 /**
  * drm_mode_create - create a new display mode
  * @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index adb3f9b..db7a885 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -894,6 +894,7 @@ extern int drm_plane_init(struct drm_device *dev,
 			  const uint32_t *formats, uint32_t format_count,
 			  bool priv);
 extern void drm_plane_cleanup(struct drm_plane *plane);
+extern void drm_plane_force_disable(struct drm_plane *plane);
 
 extern void drm_encoder_cleanup(struct drm_encoder *encoder);
 
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/vmwgfx: Don't access file_priv in cursor_set when handle==0

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We want to disable the cursor by calling ->cursor_set() with handle=0
from places where we don't have a file_priv, so don't try to access it
unless necessary.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 3e3c7ab..d4607b2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -174,7 +174,6 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 			   uint32_t handle, uint32_t width, uint32_t height)
 {
 	struct vmw_private *dev_priv = vmw_priv(crtc->dev);
-	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 	struct vmw_surface *surface = NULL;
 	struct vmw_dma_buffer *dmabuf = NULL;
@@ -197,6 +196,8 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	}
 
 	if (handle) {
+		struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+
 		ret = vmw_user_lookup_handle(dev_priv, tfile,
 					     handle, &surface, &dmabuf);
 		if (ret) {
-- 
1.8.1.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/fb-helper: Disable cursors and planes when restoring fbdev mode

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Cursors and plane can obscure whatever fbdev wants to show the user.
Disable them all in drm_fb_helper_restore_fbdev_mode.

After the cursors and planes have been disabled, user space needs to
explicitly re-enable them to make them visible again.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0df0ebb..3d13ca6e2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -287,13 +287,27 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
  */
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
+	struct drm_device *dev = fb_helper->dev;
+	struct drm_plane *plane;
 	bool error = false;
-	int i, ret;
+	int i;
+
+	drm_warn_on_modeset_not_all_locked(dev);
 
-	drm_warn_on_modeset_not_all_locked(fb_helper->dev);
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+		drm_plane_force_disable(plane);
 
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
+		struct drm_crtc *crtc = mode_set->crtc;
+		int ret;
+
+		if (crtc->funcs->cursor_set) {
+			ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
+			if (ret)
+				error = true;
+		}
+
 		ret = drm_mode_set_config_internal(mode_set);
 		if (ret)
 			error = true;
-- 
1.8.1.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vmwgfx: Don't access file_priv in cursor_set when handle==0

[Jakob Bornecrantz]

[-- Attachment #1.1: Type: text/plain, Size: 1877 bytes --]

Thanks, looks good and is
Reviewed-by: Jakob Bornecrantz <jakob@vmware.com>

Cheers, Jakob.


On Mon, Jun 3, 2013 at 3:10 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We want to disable the cursor by calling ->cursor_set() with handle=0
> from places where we don't have a file_priv, so don't try to access it
> unless necessary.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 3e3c7ab..d4607b2 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -174,7 +174,6 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc,
> struct drm_file *file_priv,
>                            uint32_t handle, uint32_t width, uint32_t
> height)
>  {
>         struct vmw_private *dev_priv = vmw_priv(crtc->dev);
> -       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
>         struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
>         struct vmw_surface *surface = NULL;
>         struct vmw_dma_buffer *dmabuf = NULL;
> @@ -197,6 +196,8 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc,
> struct drm_file *file_priv,
>         }
>
>         if (handle) {
> +               struct ttm_object_file *tfile =
> vmw_fpriv(file_priv)->tfile;
> +
>                 ret = vmw_user_lookup_handle(dev_priv, tfile,
>                                              handle, &surface, &dmabuf);
>                 if (ret) {
> --
> 1.8.1.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2695 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: Add drm_plane_force_disable()

[Laurent Pinchart]

Hi Ville,

Thanks for the patch.

On Monday 03 June 2013 16:10:40 ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_plane_force_disable() will forcibly disable the plane even if user
> had previously requested the plane to be enabled.
> 
> This can be used to force planes to be off when restoring the fbdev
> mode.
> 
> The code was simply pulled from drm_framebuffer_remove(), which now
> calls the new function as well.
> 
> v2: Check plane->fb in drm_plane_force_disable(), drop bogus comment
>     about disabling crtc
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 29 +++++++++++++++++++----------
>  include/drm/drm_crtc.h     |  1 +
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e7e9242..865ebfe 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -569,16 +569,8 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> }
> 
>  		list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> -			if (plane->fb == fb) {
> -				/* should turn off the crtc */
> -				ret = plane->funcs->disable_plane(plane);
> -				if (ret)
> -					DRM_ERROR("failed to disable plane with busy fb\n");
> -				/* disconnect the plane from the fb and crtc: */
> -				__drm_framebuffer_unreference(plane->fb);
> -				plane->fb = NULL;
> -				plane->crtc = NULL;
> -			}
> +			if (plane->fb == fb)
> +				drm_plane_force_disable(plane);
>  		}
>  		drm_modeset_unlock_all(dev);
>  	}
> @@ -867,6 +859,23 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);

What about adding kerneldoc ? :-)

> +void drm_plane_force_disable(struct drm_plane *plane)
> +{
> +	int ret;
> +
> +	if (!plane->fb)
> +		return;
> +
> +	ret = plane->funcs->disable_plane(plane);
> +	if (ret)
> +		DRM_ERROR("failed to disable plane with busy fb\n");
> +	/* disconnect the plane from the fb and crtc: */
> +	__drm_framebuffer_unreference(plane->fb);
> +	plane->fb = NULL;
> +	plane->crtc = NULL;
> +}
> +EXPORT_SYMBOL(drm_plane_force_disable);
> +
>  /**
>   * drm_mode_create - create a new display mode
>   * @dev: DRM device
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index adb3f9b..db7a885 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -894,6 +894,7 @@ extern int drm_plane_init(struct drm_device *dev,
>  			  const uint32_t *formats, uint32_t format_count,
>  			  bool priv);
>  extern void drm_plane_cleanup(struct drm_plane *plane);
> +extern void drm_plane_force_disable(struct drm_plane *plane);
> 
>  extern void drm_encoder_cleanup(struct drm_encoder *encoder);
-- 
Regards,

Laurent Pinchart

[PATCH] drm: Add kernel-doc for plane functions

[ville.syrjala]

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.
+ *
+ * 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;
-- 
1.8.1.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/fb-helper: Disable cursors and planes when restoring fbdev mode

[Daniel Vetter]

On Mon, Jun 03, 2013 at 04:10:42PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Cursors and plane can obscure whatever fbdev wants to show the user.
> Disable them all in drm_fb_helper_restore_fbdev_mode.
> 
> After the cursors and planes have been disabled, user space needs to
> explicitly re-enable them to make them visible again.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yeah, I like that color ;-) For the series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0df0ebb..3d13ca6e2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -287,13 +287,27 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>   */
>  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  {
> +	struct drm_device *dev = fb_helper->dev;
> +	struct drm_plane *plane;
>  	bool error = false;
> -	int i, ret;
> +	int i;
> +
> +	drm_warn_on_modeset_not_all_locked(dev);
>  
> -	drm_warn_on_modeset_not_all_locked(fb_helper->dev);
> +	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> +		drm_plane_force_disable(plane);
>  
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
>  		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> +		struct drm_crtc *crtc = mode_set->crtc;
> +		int ret;
> +
> +		if (crtc->funcs->cursor_set) {
> +			ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
> +			if (ret)
> +				error = true;
> +		}
> +
>  		ret = drm_mode_set_config_internal(mode_set);
>  		if (ret)
>  			error = true;
> -- 
> 1.8.1.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm: Add kernel-doc for plane functions

[Laurent Pinchart]

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;
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: Add kernel-doc for plane functions

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/2] drm: Improve drm_crtc documentation

[ville.syrjala]

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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f00ba75..857acf2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -585,7 +585,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
  * @crtc: CRTC object to init
  * @funcs: callbacks for the new CRTC
  *
- * Inits a new object created as base part of an driver crtc object.
+ * Inits a new object created as base part of a driver crtc object.
  *
  * RETURNS:
  * Zero on success, error code on failure.
@@ -620,11 +620,12 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_crtc_init);
 
 /**
- * drm_crtc_cleanup - Cleans up the core crtc usage.
+ * drm_crtc_cleanup - Clean up the core crtc usage
  * @crtc: CRTC to cleanup
  *
- * Cleanup @crtc. Removes from drm modesetting space
- * does NOT free object, caller does that.
+ * This function cleans up @crtc and removes it from the DRM mode setting
+ * core. Note that the function does *not* free the crtc structure itself,
+ * this is the responsibility of the caller.
  */
 void drm_crtc_cleanup(struct drm_crtc *crtc)
 {
-- 
1.8.1.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm: Add kernel-doc for plane functions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

v2: Follow the drm_crtc documentation fixes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 857acf2..4b8953c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -796,6 +796,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 a 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,
@@ -844,6 +859,14 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 }
 EXPORT_SYMBOL(drm_plane_init);
 
+/**
+ * drm_plane_cleanup - Clean up the core plane usage
+ * @plane: plane to cleanup
+ *
+ * This function cleans 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;
@@ -860,6 +883,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;
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Improve drm_crtc documentation

[Alex Deucher]

On Wed, Jun 5, 2013 at 8:39 AM,  <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>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_crtc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f00ba75..857acf2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -585,7 +585,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
>   * @crtc: CRTC object to init
>   * @funcs: callbacks for the new CRTC
>   *
> - * Inits a new object created as base part of an driver crtc object.
> + * Inits a new object created as base part of a driver crtc object.
>   *
>   * RETURNS:
>   * Zero on success, error code on failure.
> @@ -620,11 +620,12 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_crtc_init);
>
>  /**
> - * drm_crtc_cleanup - Cleans up the core crtc usage.
> + * drm_crtc_cleanup - Clean up the core crtc usage
>   * @crtc: CRTC to cleanup
>   *
> - * Cleanup @crtc. Removes from drm modesetting space
> - * does NOT free object, caller does that.
> + * This function cleans up @crtc and removes it from the DRM mode setting
> + * core. Note that the function does *not* free the crtc structure itself,
> + * this is the responsibility of the caller.
>   */
>  void drm_crtc_cleanup(struct drm_crtc *crtc)
>  {
> --
> 1.8.1.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel