[RESEND PATCH 0/3] Miscellaneous KMS fixes

[RESEND PATCH 0/3] Miscellaneous KMS fixes

[Laurent Pinchart]

Hello,

Upon Daniel Vetter's request here's a repost of two mode set and page flip
fixes that were initially sent nearly a year ago under the title

	"[PATCH 0/2] Miscellaneous mode set and page flip fixes".

I've also added a third, already acked, pending patch to the set.

Laurent Pinchart (3):
  drm: Don't allow page flip to change pixel format
  drm: Perform a full mode set when the pixel format changed
  drm: Destroy property blobs at mode config cleanup time

 drivers/gpu/drm/drm_crtc.c        | 216 ++++++++++++++++++++------------------
 drivers/gpu/drm/drm_crtc_helper.c |   3 +
 2 files changed, 118 insertions(+), 101 deletions(-)

-- 
Regards,

Laurent Pinchart

[RESEND PATCH 1/3] drm: Don't allow page flip to change pixel format

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

A page flip is not a mode set, changing the frame buffer pixel format
doesn't make sense and isn't handled by most drivers anyway. Disallow
it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_crtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index dd64a06..ef247d5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3739,6 +3739,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
+	if (crtc->fb->pixel_format != fb->pixel_format ||
+	    crtc->fb->bits_per_pixel != crtc->fb->bits_per_pixel ||
+	    crtc->fb->depth != fb->depth) {
+		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 		ret = -ENOMEM;
 		spin_lock_irqsave(&dev->event_lock, flags);
-- 
1.8.1.5

[RESEND PATCH 2/3] drm: Perform a full mode set when the pixel format changed

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Test whether the pixel format changes in the mode set handler, and
perform a full mode set instead of a mode set base if it does.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 7b2d378..e974f93 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -648,6 +648,9 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 		} else if (set->fb->bits_per_pixel !=
 			   set->crtc->fb->bits_per_pixel) {
 			mode_changed = true;
+		} else if (set->fb->pixel_format !=
+			   set->crtc->fb->pixel_format) {
+			mode_changed = true;
 		} else
 			fb_changed = true;
 	}
-- 
1.8.1.5

[RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Laurent Pinchart]

Property blob objects need to be destroyed when cleaning up to avoid
memory leaks. Go through the list of all blobs in the
drm_mode_config_cleanup() function and destroy them.

The drm_mode_config_cleanup() function needs to be moved after the
drm_property_destroy_blob() declaration. Move drm_mode_config_init() as
well to keep the functions together.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 208 +++++++++++++++++++++++----------------------
 1 file changed, 107 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ef247d5..db9707e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1120,44 +1120,6 @@ int drm_mode_create_dirty_info_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
 
-/**
- * drm_mode_config_init - initialize DRM mode_configuration structure
- * @dev: DRM device
- *
- * Initialize @dev's mode_config structure, used for tracking the graphics
- * configuration of @dev.
- *
- * Since this initializes the modeset locks, no locking is possible. Which is no
- * problem, since this should happen single threaded at init time. It is the
- * driver's problem to ensure this guarantee.
- *
- */
-void drm_mode_config_init(struct drm_device *dev)
-{
-	mutex_init(&dev->mode_config.mutex);
-	mutex_init(&dev->mode_config.idr_mutex);
-	mutex_init(&dev->mode_config.fb_lock);
-	INIT_LIST_HEAD(&dev->mode_config.fb_list);
-	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
-	INIT_LIST_HEAD(&dev->mode_config.connector_list);
-	INIT_LIST_HEAD(&dev->mode_config.encoder_list);
-	INIT_LIST_HEAD(&dev->mode_config.property_list);
-	INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
-	INIT_LIST_HEAD(&dev->mode_config.plane_list);
-	idr_init(&dev->mode_config.crtc_idr);
-
-	drm_modeset_lock_all(dev);
-	drm_mode_create_standard_connector_properties(dev);
-	drm_modeset_unlock_all(dev);
-
-	/* Just to be sure */
-	dev->mode_config.num_fb = 0;
-	dev->mode_config.num_connector = 0;
-	dev->mode_config.num_crtc = 0;
-	dev->mode_config.num_encoder = 0;
-}
-EXPORT_SYMBOL(drm_mode_config_init);
-
 int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
 {
 	uint32_t total_objects = 0;
@@ -1203,69 +1165,6 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
 EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
 
 /**
- * drm_mode_config_cleanup - free up DRM mode_config info
- * @dev: DRM device
- *
- * Free up all the connectors and CRTCs associated with this DRM device, then
- * free up the framebuffers and associated buffer objects.
- *
- * Note that since this /should/ happen single-threaded at driver/device
- * teardown time, no locking is required. It's the driver's job to ensure that
- * this guarantee actually holds true.
- *
- * FIXME: cleanup any dangling user buffer objects too
- */
-void drm_mode_config_cleanup(struct drm_device *dev)
-{
-	struct drm_connector *connector, *ot;
-	struct drm_crtc *crtc, *ct;
-	struct drm_encoder *encoder, *enct;
-	struct drm_framebuffer *fb, *fbt;
-	struct drm_property *property, *pt;
-	struct drm_plane *plane, *plt;
-
-	list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
-				 head) {
-		encoder->funcs->destroy(encoder);
-	}
-
-	list_for_each_entry_safe(connector, ot,
-				 &dev->mode_config.connector_list, head) {
-		connector->funcs->destroy(connector);
-	}
-
-	list_for_each_entry_safe(property, pt, &dev->mode_config.property_list,
-				 head) {
-		drm_property_destroy(dev, property);
-	}
-
-	/*
-	 * Single-threaded teardown context, so it's not required to grab the
-	 * fb_lock to protect against concurrent fb_list access. Contrary, it
-	 * would actually deadlock with the drm_framebuffer_cleanup function.
-	 *
-	 * Also, if there are any framebuffers left, that's a driver leak now,
-	 * so politely WARN about this.
-	 */
-	WARN_ON(!list_empty(&dev->mode_config.fb_list));
-	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-		drm_framebuffer_remove(fb);
-	}
-
-	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
-				 head) {
-		plane->funcs->destroy(plane);
-	}
-
-	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
-		crtc->funcs->destroy(crtc);
-	}
-
-	idr_destroy(&dev->mode_config.crtc_idr);
-}
-EXPORT_SYMBOL(drm_mode_config_cleanup);
-
-/**
  * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
  * @out: drm_mode_modeinfo struct to return to the user
  * @in: drm_display_mode to use
@@ -4072,3 +3971,110 @@ int drm_format_vert_chroma_subsampling(uint32_t format)
 	}
 }
 EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
+
+/**
+ * drm_mode_config_init - initialize DRM mode_configuration structure
+ * @dev: DRM device
+ *
+ * Initialize @dev's mode_config structure, used for tracking the graphics
+ * configuration of @dev.
+ *
+ * Since this initializes the modeset locks, no locking is possible. Which is no
+ * problem, since this should happen single threaded at init time. It is the
+ * driver's problem to ensure this guarantee.
+ *
+ */
+void drm_mode_config_init(struct drm_device *dev)
+{
+	mutex_init(&dev->mode_config.mutex);
+	mutex_init(&dev->mode_config.idr_mutex);
+	mutex_init(&dev->mode_config.fb_lock);
+	INIT_LIST_HEAD(&dev->mode_config.fb_list);
+	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
+	INIT_LIST_HEAD(&dev->mode_config.connector_list);
+	INIT_LIST_HEAD(&dev->mode_config.encoder_list);
+	INIT_LIST_HEAD(&dev->mode_config.property_list);
+	INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
+	INIT_LIST_HEAD(&dev->mode_config.plane_list);
+	idr_init(&dev->mode_config.crtc_idr);
+
+	drm_modeset_lock_all(dev);
+	drm_mode_create_standard_connector_properties(dev);
+	drm_modeset_unlock_all(dev);
+
+	/* Just to be sure */
+	dev->mode_config.num_fb = 0;
+	dev->mode_config.num_connector = 0;
+	dev->mode_config.num_crtc = 0;
+	dev->mode_config.num_encoder = 0;
+}
+EXPORT_SYMBOL(drm_mode_config_init);
+
+/**
+ * drm_mode_config_cleanup - free up DRM mode_config info
+ * @dev: DRM device
+ *
+ * Free up all the connectors and CRTCs associated with this DRM device, then
+ * free up the framebuffers and associated buffer objects.
+ *
+ * Note that since this /should/ happen single-threaded at driver/device
+ * teardown time, no locking is required. It's the driver's job to ensure that
+ * this guarantee actually holds true.
+ *
+ * FIXME: cleanup any dangling user buffer objects too
+ */
+void drm_mode_config_cleanup(struct drm_device *dev)
+{
+	struct drm_connector *connector, *ot;
+	struct drm_crtc *crtc, *ct;
+	struct drm_encoder *encoder, *enct;
+	struct drm_framebuffer *fb, *fbt;
+	struct drm_property *property, *pt;
+	struct drm_property_blob *blob, *bt;
+	struct drm_plane *plane, *plt;
+
+	list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
+				 head) {
+		encoder->funcs->destroy(encoder);
+	}
+
+	list_for_each_entry_safe(connector, ot,
+				 &dev->mode_config.connector_list, head) {
+		connector->funcs->destroy(connector);
+	}
+
+	list_for_each_entry_safe(property, pt, &dev->mode_config.property_list,
+				 head) {
+		drm_property_destroy(dev, property);
+	}
+
+	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
+				 head) {
+		drm_property_destroy_blob(dev, blob);
+	}
+
+	/*
+	 * Single-threaded teardown context, so it's not required to grab the
+	 * fb_lock to protect against concurrent fb_list access. Contrary, it
+	 * would actually deadlock with the drm_framebuffer_cleanup function.
+	 *
+	 * Also, if there are any framebuffers left, that's a driver leak now,
+	 * so politely WARN about this.
+	 */
+	WARN_ON(!list_empty(&dev->mode_config.fb_list));
+	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
+		drm_framebuffer_remove(fb);
+	}
+
+	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
+				 head) {
+		plane->funcs->destroy(plane);
+	}
+
+	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
+		crtc->funcs->destroy(crtc);
+	}
+
+	idr_destroy(&dev->mode_config.crtc_idr);
+}
+EXPORT_SYMBOL(drm_mode_config_cleanup);
-- 
1.8.1.5

Re: [RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Dave Airlie]

On Mon, Apr 15, 2013 at 11:37 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> Property blob objects need to be destroyed when cleaning up to avoid
> memory leaks. Go through the list of all blobs in the
> drm_mode_config_cleanup() function and destroy them.
>
> The drm_mode_config_cleanup() function needs to be moved after the
> drm_property_destroy_blob() declaration. Move drm_mode_config_init() as
> well to keep the functions together.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I've applied this one, the other two I don't mind, but I'm not sure
they aren't a bit restrictive in the generic code,

Dave.

Re: [RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Daniel Vetter]

On Tue, Apr 16, 2013 at 01:12:21PM +1000, Dave Airlie wrote:
> On Mon, Apr 15, 2013 at 11:37 PM, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
> > Property blob objects need to be destroyed when cleaning up to avoid
> > memory leaks. Go through the list of all blobs in the
> > drm_mode_config_cleanup() function and destroy them.
> >
> > The drm_mode_config_cleanup() function needs to be moved after the
> > drm_property_destroy_blob() declaration. Move drm_mode_config_init() as
> > well to keep the functions together.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I've applied this one, the other two I don't mind, but I'm not sure
> they aren't a bit restrictive in the generic code,

Patch 2 is just for the crtc helpers, and matches what we've just done in
the i915 code. I think it fits in with the general design of the crtc
helpers.

Patch 1 matches a check in the intel code, too. So since applications
can't really know whether a flip to a different pixel format works I don't
think that capability is useful at all. And at least for i915 it's just
broken ;-) On patch 1 itself though: Just checking fb->pixel_format should
be good enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 16 April 2013 21:06:43 Daniel Vetter wrote:
> On Tue, Apr 16, 2013 at 01:12:21PM +1000, Dave Airlie wrote:
> > On Mon, Apr 15, 2013 at 11:37 PM, Laurent Pinchart wrote:
> > > Property blob objects need to be destroyed when cleaning up to avoid
> > > memory leaks. Go through the list of all blobs in the
> > > drm_mode_config_cleanup() function and destroy them.
> > > 
> > > The drm_mode_config_cleanup() function needs to be moved after the
> > > drm_property_destroy_blob() declaration. Move drm_mode_config_init() as
> > > well to keep the functions together.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I've applied this one, the other two I don't mind, but I'm not sure
> > they aren't a bit restrictive in the generic code,
> 
> Patch 2 is just for the crtc helpers, and matches what we've just done in
> the i915 code. I think it fits in with the general design of the crtc
> helpers.
> 
> Patch 1 matches a check in the intel code, too. So since applications
> can't really know whether a flip to a different pixel format works I don't
> think that capability is useful at all. And at least for i915 it's just
> broken ;-) On patch 1 itself though: Just checking fb->pixel_format should
> be good enough.

I've fixed that and I'll resubmit after we reach an agreement on 1/3 and 2/3.

-- 
Regards,

Laurent Pinchart