[PATCH 0/4] drm/i915: Make video sprites survive a modeset

[PATCH 0/4] drm/i915: Make video sprites survive a modeset

[ville.syrjala]

Currently the video sprites appear to get disabled on modeset more by
accient than by design.

With the current API that behaviour makes very little sense to me.
You first enable some plane, and then it can get disabled due to some
unrelated operation.

So these patches change the behaviour so that planes survive a modeset.
There's a new hook to make sure they get disabled when swithing
back to fbdev to show a panic oops.

[PATCH 1/4] drm: Keep a copy of last plane coordinates

[ville.syrjala]

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

If the update_plane() operation succeeds, make a copy of the requested
src and crtc coordinates, so that the the plane may be reclipped if the
display mode changed later.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d981fe2..6dafb99 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1793,6 +1793,14 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	if (!ret) {
 		plane->crtc = crtc;
 		plane->fb = fb;
+		plane->crtc_x = plane_req->crtc_x;
+		plane->crtc_y = plane_req->crtc_y;
+		plane->crtc_w = plane_req->crtc_w;
+		plane->crtc_h = plane_req->crtc_h;
+		plane->src_x = plane_req->src_x;
+		plane->src_y = plane_req->src_y;
+		plane->src_w = plane_req->src_w;
+		plane->src_h = plane_req->src_h;
 	}
 
 out:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8a17cce..3261492 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -657,6 +657,10 @@ struct drm_plane {
 	void *helper_private;
 
 	struct drm_object_properties properties;
+
+	uint32_t src_x, src_y, src_w, src_h;
+	int32_t crtc_x, crtc_y;
+	uint32_t crtc_w, crtc_h;
 };
 
 /**
-- 
1.7.3.4

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

[PATCH 2/4] drm: Add restore_fbdev_mode() hook to drm_fb_helper

[ville.syrjala]

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

Add an optional driver specific restore_fbdev_mode() hook to
drm_fb_helper. If the driver doesn't provide the hook,
drm_fb_helper_restore_fbdev_mode() is called directly as before.

In this hook the driver can disable additional planes, cursors etc.
that shouldn't be visible while fbdev is in control.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5683b7f..5cfc7dd 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -248,7 +248,10 @@ bool drm_fb_helper_force_kernel_mode(void)
 		if (helper->dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 			continue;
 
-		ret = drm_fb_helper_restore_fbdev_mode(helper);
+		if (helper->funcs->restore_fbdev_mode)
+			ret = helper->funcs->restore_fbdev_mode(helper);
+		else
+			ret = drm_fb_helper_restore_fbdev_mode(helper);
 		if (ret)
 			error = true;
 	}
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 5120b01..7f76e9c 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -56,6 +56,7 @@ struct drm_fb_helper_funcs {
 
 	int (*fb_probe)(struct drm_fb_helper *helper,
 			struct drm_fb_helper_surface_size *sizes);
+	int (*restore_fbdev_mode)(struct drm_fb_helper *helper);
 };
 
 struct drm_fb_helper_connector {
-- 
1.7.3.4

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

[PATCH 3/4] drm/i915: Disable/enable planes around mode set

[ville.syrjala]

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

When setting a display mode, disable all planes on the CRTC beforehand,
and re-enable them after the new mode has been set.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   48 ++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 72ac2f9..aec6cac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3012,6 +3012,49 @@ void intel_cpt_verify_modeset(struct drm_device *dev, int pipe)
 	}
 }
 
+static int intel_crtc_disable_planes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_plane *plane;
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		int ret;
+
+		if (plane->crtc != crtc || !plane->fb)
+			continue;
+
+		ret = plane->funcs->disable_plane(plane);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int intel_crtc_enable_planes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_plane *plane;
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		int ret;
+
+		if (plane->crtc != crtc || !plane->fb)
+			continue;
+
+		ret = plane->funcs->update_plane(plane,
+						 plane->crtc, plane->fb,
+						 plane->crtc_x, plane->crtc_y,
+						 plane->crtc_w, plane->crtc_h,
+						 plane->src_x, plane->src_y,
+						 plane->src_w, plane->src_h);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3060,6 +3103,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_enable_pipe(dev_priv, pipe, is_pch_port);
+	intel_crtc_enable_planes(crtc);
 	intel_enable_plane(dev_priv, plane, pipe);
 
 	if (is_pch_port)
@@ -3088,6 +3132,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	drm_vblank_off(dev, pipe);
 	intel_crtc_update_cursor(crtc, false);
 
+	intel_crtc_disable_planes(crtc);
 	intel_disable_plane(dev_priv, plane, pipe);
 
 	if (dev_priv->cfb_plane == plane)
@@ -3233,6 +3278,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_enable_pll(dev_priv, pipe);
 	intel_enable_pipe(dev_priv, pipe, false);
+
 	intel_enable_plane(dev_priv, plane, pipe);
 
 	intel_crtc_load_lut(crtc);
@@ -3241,6 +3287,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
 	intel_crtc_update_cursor(crtc, true);
+	intel_crtc_enable_planes(crtc);
 }
 
 static void i9xx_crtc_disable(struct drm_crtc *crtc)
@@ -3259,6 +3306,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	drm_vblank_off(dev, pipe);
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
+	intel_crtc_disable_planes(crtc);
 
 	if (dev_priv->cfb_plane == plane)
 		intel_disable_fbc(dev);
-- 
1.7.3.4

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

[PATCH 4/4] drm/i915: Implement restore_fbdev_mode hook

[ville.syrjala]

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

Convert intel_fb_restore_mode to be useable as the
drm_fb_helper.restore_fbdev_mode hook. This will cause all planes to be
disabled when swithing back to fbcon.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  |    2 +-
 drivers/gpu/drm/i915/intel_drv.h |    2 +-
 drivers/gpu/drm/i915/intel_fb.c  |   14 ++++++++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f947926..186308a 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1738,7 +1738,7 @@ void i915_driver_lastclose(struct drm_device * dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	if (!dev_priv || drm_core_check_feature(dev, DRIVER_MODESET)) {
-		intel_fb_restore_mode(dev);
+		intel_fb_restore_mode(&dev_priv->fbdev->helper);
 		vga_switcheroo_process_delayed_switch();
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3e09188..2ec63a8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -452,7 +452,7 @@ extern int intel_overlay_attrs(struct drm_device *dev, void *data,
 			       struct drm_file *file_priv);
 
 extern void intel_fb_output_poll_changed(struct drm_device *dev);
-extern void intel_fb_restore_mode(struct drm_device *dev);
+extern int intel_fb_restore_mode(struct drm_fb_helper *helper);
 
 extern void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 			bool state);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 07404ac..51b7fd1 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -192,6 +192,7 @@ static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 	.gamma_set = intel_crtc_fb_gamma_set,
 	.gamma_get = intel_crtc_fb_gamma_get,
 	.fb_probe = intel_fb_find_or_create_single,
+	.restore_fbdev_mode = intel_fb_restore_mode,
 };
 
 static void intel_fbdev_destroy(struct drm_device *dev,
@@ -272,22 +273,27 @@ void intel_fb_output_poll_changed(struct drm_device *dev)
 	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
 }
 
-void intel_fb_restore_mode(struct drm_device *dev)
+int intel_fb_restore_mode(struct drm_fb_helper *helper)
 {
+	struct drm_device *dev = helper->dev;
 	int ret;
-	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_plane *plane;
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	ret = drm_fb_helper_restore_fbdev_mode(&dev_priv->fbdev->helper);
-	if (ret)
+	ret = drm_fb_helper_restore_fbdev_mode(helper);
+	if (ret) {
 		DRM_DEBUG("failed to restore crtc mode\n");
+		goto out;
+	}
 
 	/* Be sure to shut off any planes that may be active */
 	list_for_each_entry(plane, &config->plane_list, head)
 		plane->funcs->disable_plane(plane);
 
+out:
 	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
 }
-- 
1.7.3.4

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

Re: [PATCH 0/4] drm/i915: Make video sprites survive a modeset

[Jesse Barnes]

On Thu, 24 May 2012 21:29:46 +0300
ville.syrjala@linux.intel.com wrote:

> Currently the video sprites appear to get disabled on modeset more by
> accient than by design.
> 
> With the current API that behaviour makes very little sense to me.
> You first enable some plane, and then it can get disabled due to some
> unrelated operation.
> 
> So these patches change the behaviour so that planes survive a modeset.
> There's a new hook to make sure they get disabled when swithing
> back to fbdev to show a panic oops.

Yeah that's not really a design requirement; the assumption was that
the display manager would do the right thing in any case (both mode
sets and plane sets are privileged ops).  When doing a mode set, the
plane parameters will probably need to be changed anyway...

But keeping it on with some kind of sensible behavior makes the simple
cases easier.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 0/4] drm/i915: Make video sprites survive a modeset

[Daniel Vetter]

On Thu, May 24, 2012 at 11:35:35AM -0700, Jesse Barnes wrote:
> On Thu, 24 May 2012 21:29:46 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > Currently the video sprites appear to get disabled on modeset more by
> > accient than by design.
> > 
> > With the current API that behaviour makes very little sense to me.
> > You first enable some plane, and then it can get disabled due to some
> > unrelated operation.
> > 
> > So these patches change the behaviour so that planes survive a modeset.
> > There's a new hook to make sure they get disabled when swithing
> > back to fbdev to show a panic oops.
> 
> Yeah that's not really a design requirement; the assumption was that
> the display manager would do the right thing in any case (both mode
> sets and plane sets are privileged ops).  When doing a mode set, the
> plane parameters will probably need to be changed anyway...
> 
> But keeping it on with some kind of sensible behavior makes the simple
> cases easier.

tbh I don't see the use-case. If you issue a modeset from userspace, you
better start out with something sensible (like a black screeen) and fade
in nicely whatever you want to show. And if you change the layout, you
have to reorg everything anyway.

We do though do a few modesets from within the kernel (e.g. audio property
on hdmi/dp), and I guess these should forget about any currently visible
planes. Dunno what to do here.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/4] drm/i915: Make video sprites survive a modeset

[Ville Syrjälä]

On Thu, May 24, 2012 at 08:49:53PM +0200, Daniel Vetter wrote:
> On Thu, May 24, 2012 at 11:35:35AM -0700, Jesse Barnes wrote:
> > On Thu, 24 May 2012 21:29:46 +0300
> > ville.syrjala@linux.intel.com wrote:
> > 
> > > Currently the video sprites appear to get disabled on modeset more by
> > > accient than by design.
> > > 
> > > With the current API that behaviour makes very little sense to me.
> > > You first enable some plane, and then it can get disabled due to some
> > > unrelated operation.
> > > 
> > > So these patches change the behaviour so that planes survive a modeset.
> > > There's a new hook to make sure they get disabled when swithing
> > > back to fbdev to show a panic oops.
> > 
> > Yeah that's not really a design requirement; the assumption was that
> > the display manager would do the right thing in any case (both mode
> > sets and plane sets are privileged ops).  When doing a mode set, the
> > plane parameters will probably need to be changed anyway...
> > 
> > But keeping it on with some kind of sensible behavior makes the simple
> > cases easier.
> 
> tbh I don't see the use-case. If you issue a modeset from userspace, you
> better start out with something sensible (like a black screeen) and fade
> in nicely whatever you want to show. And if you change the layout, you
> have to reorg everything anyway.

Mainly I just dislike incoherent behaviour.

One use case might be flipping to another framebuffer using the
setcrtc ioctl, in case the page flip ioctl isn't provided, or can't
be used. With the current code the result depends on various
implementation specific details like whether the driver implements
a set_base type of optimization in a certain way. From the user
space POV it's just a setcrtc ioctl, but there's no sensible way
to know whether the operation will destroy some unrelated state
or not.

-- 
Ville Syrjälä
Intel OTC