[RFC PATCH v2 0/5] Add vblank hooks to struct drm_crtc_funcs

[RFC PATCH v2 0/5] Add vblank hooks to struct drm_crtc_funcs

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

The vblank is mostly CRTC specific and implemented as part of CRTC
driver.  The first patch adds 3 vblank core-driver hooks into struct
drm_crtc_funcs, and wraps around core vblank handling code to use the
new hooks for modern MODESET drivers and the ones in struct drm_driver
as fallback for legacy drivers.

The other patches in the series are to demonstrate how the new hooks
are going to influence the driver code.  There are more drivers than
the ones included here can be converted.  But before doing that, I would
like to get some feedbacks first, expecially on how .get_vblank_counter
should be converted when it's being drm_vblank_no_hw_counter().

	.get_vblank_counter = drm_vblank_no_hw_counter

The series is generated against branch drm-misc-next.

Changes for v2:
 - Wrap around core vblank handling code to save
   drm_crtc_enable[disable]_vblank() helpers
 - Add .get_vblank_counter to struct drm_crtc_funcs
 - Add some comments to link between two sets of hooks
 - Add one hdlcd driver patch for example

Shawn Guo (5):
  drm: add vblank hooks to struct drm_crtc_funcs
  drm: hdlcd: use vblank hooks in struct drm_crtc_funcs
  drm: zte: zx_vou_enable[disable]_vblank can be static
  drm: rockchip: remove struct rockchip_crtc_funcs
  drm: imx: remove struct imx_drm_crtc and imx_drm_crtc_helper_funcs

 drivers/gpu/drm/arm/hdlcd_crtc.c            |  20 ++++++
 drivers/gpu/drm/arm/hdlcd_drv.c             |  20 ------
 drivers/gpu/drm/drm_irq.c                   |  53 +++++++++++++--
 drivers/gpu/drm/imx/imx-drm-core.c          | 101 ----------------------------
 drivers/gpu/drm/imx/imx-drm.h               |  13 ----
 drivers/gpu/drm/imx/ipuv3-crtc.c            |  58 ++++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  51 --------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  14 ----
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   9 +--
 drivers/gpu/drm/zte/zx_drm_drv.c            |   2 -
 drivers/gpu/drm/zte/zx_vou.c                |  61 +++++++----------
 drivers/gpu/drm/zte/zx_vou.h                |   3 -
 include/drm/drm_crtc.h                      |  34 ++++++++++
 include/drm/drm_drv.h                       |   9 +++
 14 files changed, 157 insertions(+), 291 deletions(-)

-- 
1.9.1

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

[RFC PATCH v2 1/5] drm: add vblank hooks to struct drm_crtc_funcs

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

The vblank is mostly CRTC specific and implemented as part of CRTC
driver.  Let's keep the vblank hooks struct drm_driver for legacy
drivers, and add corresponding hooks in struct drm_crtc_funcs.  These
hooks take struct drm_crtc pointer as argument, and will be called by
core vblank handling code for MODESET drivers.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpu/drm/drm_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++------
 include/drm/drm_crtc.h    | 34 ++++++++++++++++++++++++++++++
 include/drm/drm_drv.h     |  9 ++++++++
 3 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 88c69e71102e..604a2acda9d6 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -89,6 +89,19 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
 	write_sequnlock(&vblank->seqlock);
 }
 
+static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+{
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+
+		if (crtc->funcs->get_vblank_counter)
+			return crtc->funcs->get_vblank_counter(crtc);
+	}
+
+	/* fallback for legacy drivers */
+	return dev->driver->get_vblank_counter(dev, pipe);
+}
+
 /*
  * Reset the stored timestamp for the current vblank count to correspond
  * to the last vblank occurred.
@@ -112,9 +125,9 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 	 * when drm_vblank_enable() applies the diff
 	 */
 	do {
-		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
+		cur_vblank = __get_vblank_counter(dev, pipe);
 		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0);
-	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
+	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
 
 	/*
 	 * Only reinitialize corresponding vblank timestamp if high-precision query
@@ -168,9 +181,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	 * corresponding vblank timestamp.
 	 */
 	do {
-		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
+		cur_vblank = __get_vblank_counter(dev, pipe);
 		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags);
-	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
+	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
 
 	if (dev->max_vblank_count != 0) {
 		/* trust the hw counter when it's around */
@@ -275,6 +288,21 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_accurate_vblank_count);
 
+static void __disable_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+
+		if (crtc->funcs->disable_vblank) {
+			crtc->funcs->disable_vblank(crtc);
+			return;
+		}
+	}
+
+	/* fallback for legacy drivers */
+	dev->driver->disable_vblank(dev, pipe);
+}
+
 /*
  * Disable vblank irq's on crtc, make sure that last vblank count
  * of hardware and corresponding consistent software vblank counter
@@ -298,7 +326,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
 	 * hardware potentially runtime suspended.
 	 */
 	if (vblank->enabled) {
-		dev->driver->disable_vblank(dev, pipe);
+		__disable_vblank(dev, pipe);
 		vblank->enabled = false;
 	}
 
@@ -1054,6 +1082,19 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_send_vblank_event);
 
+static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+
+		if (crtc->funcs->enable_vblank)
+			return crtc->funcs->enable_vblank(crtc);
+	}
+
+	/* fallback for legacy drivers */
+	return dev->driver->enable_vblank(dev, pipe);
+}
+
 /**
  * drm_vblank_enable - enable the vblank interrupt on a CRTC
  * @dev: DRM device
@@ -1079,7 +1120,7 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
 		 * timestamps. Filtercode in drm_handle_vblank() will
 		 * prevent double-accounting of same vblank interval.
 		 */
-		ret = dev->driver->enable_vblank(dev, pipe);
+		ret = __enable_vblank(dev, pipe);
 		DRM_DEBUG("enabling vblank on crtc %u, ret: %d\n", pipe, ret);
 		if (ret)
 			atomic_dec(&vblank->refcount);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 06c943d1e04c..8b2f7266eada 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -600,6 +600,40 @@ struct drm_crtc_funcs {
 	 */
 	void (*atomic_print_state)(struct drm_printer *p,
 				   const struct drm_crtc_state *state);
+
+	/**
+	 * @get_vblank_counter:
+	 *
+	 * Driver callback for fetching a raw hardware vblank counter for the
+	 * CRTC. It's meant to be used by new drivers as the replacement of
+	 * &drm_driver.get_vblank_counter hook.
+	 *
+	 * Returns:
+	 *
+	 * Raw vblank counter value.
+	 */
+	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
+
+	/**
+	 * @enable_vblank:
+	 *
+	 * Enable vblank interrupts for the CRTC. It's meant to be used by
+	 * new drivers as the replacement of &drm_driver.enable_vblank hook.
+	 *
+	 * Returns:
+	 *
+	 * Zero on success, appropriate errno if the vblank interrupt cannot
+	 * be enabled.
+	 */
+	int (*enable_vblank)(struct drm_crtc *crtc);
+
+	/**
+	 * @disable_vblank:
+	 *
+	 * Disable vblank interrupts for the CRTC. It's meant to be used by
+	 * new drivers as the replacement of &drm_driver.disable_vblank hook.
+	 */
+	void (*disable_vblank)(struct drm_crtc *crtc);
 };
 
 /**
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 34ece393c639..6c325baddc40 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -123,6 +123,9 @@ struct drm_driver {
 	 * drm_crtc_vblank_off() and drm_crtc_vblank_on() when disabling or
 	 * enabling a CRTC.
 	 *
+	 * This is deprecated and should not be used by new drivers.
+	 * Use &drm_crtc_funcs.get_vblank_counter instead.
+	 *
 	 * Returns:
 	 *
 	 * Raw vblank counter value.
@@ -135,6 +138,9 @@ struct drm_driver {
 	 * Enable vblank interrupts for the CRTC specified with the pipe
 	 * argument.
 	 *
+	 * This is deprecated and should not be used by new drivers.
+	 * Use &drm_crtc_funcs.enable_vblank instead.
+	 *
 	 * Returns:
 	 *
 	 * Zero on success, appropriate errno if the given @crtc's vblank
@@ -147,6 +153,9 @@ struct drm_driver {
 	 *
 	 * Disable vblank interrupts for the CRTC specified with the pipe
 	 * argument.
+	 *
+	 * This is deprecated and should not be used by new drivers.
+	 * Use &drm_crtc_funcs.disable_vblank instead.
 	 */
 	void (*disable_vblank) (struct drm_device *dev, unsigned int pipe);
 
-- 
1.9.1

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

[RFC PATCH v2 2/5] drm: hdlcd: use vblank hooks in struct drm_crtc_funcs

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

The vblank hooks in struct drm_driver are deprecated and only meant for
legacy drivers.  For modern drivers with DRIVER_MODESET flag, the hooks
in struct drm_crtc_funcs should be used instead.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/arm/hdlcd_drv.c  | 20 --------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 20ebfb4fbdfa..798a3cc480a2 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -42,6 +42,24 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
+static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
+{
+	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+
+	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
+
+	return 0;
+}
+
+static void hdlcd_crtc_disable_vblank(struct drm_crtc *crtc)
+{
+	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+
+	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
+}
+
 static const struct drm_crtc_funcs hdlcd_crtc_funcs = {
 	.destroy = hdlcd_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
@@ -49,6 +67,8 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.enable_vblank = hdlcd_crtc_enable_vblank,
+	.disable_vblank = hdlcd_crtc_disable_vblank,
 };
 
 static struct simplefb_format supported_formats[] = SIMPLEFB_FORMATS;
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e5f4f4a6546d..6598ba4cc958 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -199,24 +199,6 @@ static void hdlcd_irq_uninstall(struct drm_device *drm)
 	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
 }
 
-static int hdlcd_enable_vblank(struct drm_device *drm, unsigned int crtc)
-{
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
-	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
-
-	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
-
-	return 0;
-}
-
-static void hdlcd_disable_vblank(struct drm_device *drm, unsigned int crtc)
-{
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
-	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
-
-	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
-}
-
 #ifdef CONFIG_DEBUG_FS
 static int hdlcd_show_underrun_count(struct seq_file *m, void *arg)
 {
@@ -285,8 +267,6 @@ static void hdlcd_debugfs_cleanup(struct drm_minor *minor)
 	.irq_postinstall = hdlcd_irq_postinstall,
 	.irq_uninstall = hdlcd_irq_uninstall,
 	.get_vblank_counter = drm_vblank_no_hw_counter,
-	.enable_vblank = hdlcd_enable_vblank,
-	.disable_vblank = hdlcd_disable_vblank,
 	.gem_free_object_unlocked = drm_gem_cma_free_object,
 	.gem_vm_ops = &drm_gem_cma_vm_ops,
 	.dumb_create = drm_gem_cma_dumb_create,
-- 
1.9.1

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

[RFC PATCH v2 3/5] drm: zte: zx_vou_enable[disable]_vblank can be static

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

With the vblank hooks in struct drm_crtc_funcs, we can directly use the
drm_crtc pointer passed in as parameter and make the functions static.

The functions are moved around to save forward delarations.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpu/drm/zte/zx_drm_drv.c |  2 --
 drivers/gpu/drm/zte/zx_vou.c     | 61 +++++++++++++++-------------------------
 drivers/gpu/drm/zte/zx_vou.h     |  3 --
 3 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/zte/zx_drm_drv.c b/drivers/gpu/drm/zte/zx_drm_drv.c
index 3e76f72c92ff..ff2fa3971d74 100644
--- a/drivers/gpu/drm/zte/zx_drm_drv.c
+++ b/drivers/gpu/drm/zte/zx_drm_drv.c
@@ -72,8 +72,6 @@ static void zx_drm_lastclose(struct drm_device *drm)
 			   DRIVER_ATOMIC,
 	.lastclose = zx_drm_lastclose,
 	.get_vblank_counter = drm_vblank_no_hw_counter,
-	.enable_vblank = zx_vou_enable_vblank,
-	.disable_vblank = zx_vou_disable_vblank,
 	.gem_free_object = drm_gem_cma_free_object,
 	.gem_vm_ops = &drm_gem_cma_vm_ops,
 	.dumb_create = drm_gem_cma_dumb_create,
diff --git a/drivers/gpu/drm/zte/zx_vou.c b/drivers/gpu/drm/zte/zx_vou.c
index a86e3a5852a2..424f4f133067 100644
--- a/drivers/gpu/drm/zte/zx_vou.c
+++ b/drivers/gpu/drm/zte/zx_vou.c
@@ -281,6 +281,27 @@ static void zx_crtc_atomic_flush(struct drm_crtc *crtc,
 	.atomic_flush = zx_crtc_atomic_flush,
 };
 
+static int zx_vou_enable_vblank(struct drm_crtc *crtc)
+{
+	struct zx_crtc *zcrtc = to_zx_crtc(crtc);
+	struct zx_vou_hw *vou = crtc_to_vou(crtc);
+	u32 int_frame_mask = zcrtc->bits->int_frame_mask;
+
+	zx_writel_mask(vou->timing + TIMING_INT_CTRL, int_frame_mask,
+		       int_frame_mask);
+
+	return 0;
+}
+
+static void zx_vou_disable_vblank(struct drm_crtc *crtc)
+{
+	struct zx_crtc *zcrtc = to_zx_crtc(crtc);
+	struct zx_vou_hw *vou = crtc_to_vou(crtc);
+
+	zx_writel_mask(vou->timing + TIMING_INT_CTRL,
+		       zcrtc->bits->int_frame_mask, 0);
+}
+
 static const struct drm_crtc_funcs zx_crtc_funcs = {
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
@@ -288,6 +309,8 @@ static void zx_crtc_atomic_flush(struct drm_crtc *crtc,
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.enable_vblank = zx_vou_enable_vblank,
+	.disable_vblank = zx_vou_disable_vblank,
 };
 
 static int zx_crtc_init(struct drm_device *drm, struct zx_vou_hw *vou,
@@ -355,44 +378,6 @@ static int zx_crtc_init(struct drm_device *drm, struct zx_vou_hw *vou,
 	return 0;
 }
 
-int zx_vou_enable_vblank(struct drm_device *drm, unsigned int pipe)
-{
-	struct drm_crtc *crtc;
-	struct zx_crtc *zcrtc;
-	struct zx_vou_hw *vou;
-	u32 int_frame_mask;
-
-	crtc = drm_crtc_from_index(drm, pipe);
-	if (!crtc)
-		return 0;
-
-	vou = crtc_to_vou(crtc);
-	zcrtc = to_zx_crtc(crtc);
-	int_frame_mask = zcrtc->bits->int_frame_mask;
-
-	zx_writel_mask(vou->timing + TIMING_INT_CTRL, int_frame_mask,
-		       int_frame_mask);
-
-	return 0;
-}
-
-void zx_vou_disable_vblank(struct drm_device *drm, unsigned int pipe)
-{
-	struct drm_crtc *crtc;
-	struct zx_crtc *zcrtc;
-	struct zx_vou_hw *vou;
-
-	crtc = drm_crtc_from_index(drm, pipe);
-	if (!crtc)
-		return;
-
-	vou = crtc_to_vou(crtc);
-	zcrtc = to_zx_crtc(crtc);
-
-	zx_writel_mask(vou->timing + TIMING_INT_CTRL,
-		       zcrtc->bits->int_frame_mask, 0);
-}
-
 static irqreturn_t vou_irq_handler(int irq, void *dev_id)
 {
 	struct zx_vou_hw *vou = dev_id;
diff --git a/drivers/gpu/drm/zte/zx_vou.h b/drivers/gpu/drm/zte/zx_vou.h
index 349e06cd86f4..644e1d81a393 100644
--- a/drivers/gpu/drm/zte/zx_vou.h
+++ b/drivers/gpu/drm/zte/zx_vou.h
@@ -40,7 +40,4 @@ struct vou_inf {
 void vou_inf_enable(const struct vou_inf *inf, struct drm_crtc *crtc);
 void vou_inf_disable(const struct vou_inf *inf, struct drm_crtc *crtc);
 
-int zx_vou_enable_vblank(struct drm_device *drm, unsigned int pipe);
-void zx_vou_disable_vblank(struct drm_device *drm, unsigned int pipe);
-
 #endif /* __ZX_VOU_H__ */
-- 
1.9.1

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

[RFC PATCH v2 4/5] drm: rockchip: remove struct rockchip_crtc_funcs

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

With the vblank hooks in struct drm_crtc_funcs, we do not need to
maintain struct rockchip_crtc_funcs and the related registration
functions.  Remove them.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 51 -----------------------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 14 --------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  9 ++---
 3 files changed, 2 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index c30d649cb147..0de1cb8a61b5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -74,55 +74,6 @@ void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 	arm_iommu_detach_device(dev);
 }
 
-int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
-				 const struct rockchip_crtc_funcs *crtc_funcs)
-{
-	int pipe = drm_crtc_index(crtc);
-	struct rockchip_drm_private *priv = crtc->dev->dev_private;
-
-	if (pipe >= ROCKCHIP_MAX_CRTC)
-		return -EINVAL;
-
-	priv->crtc_funcs[pipe] = crtc_funcs;
-
-	return 0;
-}
-
-void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc)
-{
-	int pipe = drm_crtc_index(crtc);
-	struct rockchip_drm_private *priv = crtc->dev->dev_private;
-
-	if (pipe >= ROCKCHIP_MAX_CRTC)
-		return;
-
-	priv->crtc_funcs[pipe] = NULL;
-}
-
-static int rockchip_drm_crtc_enable_vblank(struct drm_device *dev,
-					   unsigned int pipe)
-{
-	struct rockchip_drm_private *priv = dev->dev_private;
-	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
-
-	if (crtc && priv->crtc_funcs[pipe] &&
-	    priv->crtc_funcs[pipe]->enable_vblank)
-		return priv->crtc_funcs[pipe]->enable_vblank(crtc);
-
-	return 0;
-}
-
-static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev,
-					     unsigned int pipe)
-{
-	struct rockchip_drm_private *priv = dev->dev_private;
-	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
-
-	if (crtc && priv->crtc_funcs[pipe] &&
-	    priv->crtc_funcs[pipe]->enable_vblank)
-		priv->crtc_funcs[pipe]->disable_vblank(crtc);
-}
-
 static int rockchip_drm_bind(struct device *dev)
 {
 	struct drm_device *drm_dev;
@@ -271,8 +222,6 @@ static void rockchip_drm_lastclose(struct drm_device *dev)
 				  DRIVER_PRIME | DRIVER_ATOMIC,
 	.lastclose		= rockchip_drm_lastclose,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
-	.enable_vblank		= rockchip_drm_crtc_enable_vblank,
-	.disable_vblank		= rockchip_drm_crtc_disable_vblank,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
 	.gem_free_object_unlocked = rockchip_gem_free_object,
 	.dumb_create		= rockchip_gem_dumb_create,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index fb6226cf84b7..9f9bc959b108 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -31,16 +31,6 @@
 struct drm_device;
 struct drm_connector;
 
-/*
- * Rockchip drm private crtc funcs.
- * @enable_vblank: enable crtc vblank irq.
- * @disable_vblank: disable crtc vblank irq.
- */
-struct rockchip_crtc_funcs {
-	int (*enable_vblank)(struct drm_crtc *crtc);
-	void (*disable_vblank)(struct drm_crtc *crtc);
-};
-
 struct rockchip_crtc_state {
 	struct drm_crtc_state base;
 	int output_type;
@@ -58,16 +48,12 @@ struct rockchip_crtc_state {
 struct rockchip_drm_private {
 	struct drm_fb_helper fbdev_helper;
 	struct drm_gem_object *fbdev_bo;
-	const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
 	struct drm_atomic_state *state;
 
 	struct list_head psr_list;
 	spinlock_t psr_list_lock;
 };
 
-int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
-				 const struct rockchip_crtc_funcs *crtc_funcs);
-void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..ffee8d8c3794 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -853,11 +853,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
-static const struct rockchip_crtc_funcs private_crtc_funcs = {
-	.enable_vblank = vop_crtc_enable_vblank,
-	.disable_vblank = vop_crtc_disable_vblank,
-};
-
 static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 				const struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode)
@@ -1112,6 +1107,8 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
 	.reset = vop_crtc_reset,
 	.atomic_duplicate_state = vop_crtc_duplicate_state,
 	.atomic_destroy_state = vop_crtc_destroy_state,
+	.enable_vblank = vop_crtc_enable_vblank,
+	.disable_vblank = vop_crtc_disable_vblank,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
@@ -1283,7 +1280,6 @@ static int vop_create_crtc(struct vop *vop)
 	init_completion(&vop->dsp_hold_completion);
 	init_completion(&vop->line_flag_completion);
 	crtc->port = port;
-	rockchip_register_crtc_funcs(crtc, &private_crtc_funcs);
 
 	return 0;
 
@@ -1302,7 +1298,6 @@ static void vop_destroy_crtc(struct vop *vop)
 	struct drm_device *drm_dev = vop->drm_dev;
 	struct drm_plane *plane, *tmp;
 
-	rockchip_unregister_crtc_funcs(crtc);
 	of_node_put(crtc->port);
 
 	/*
-- 
1.9.1

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

[RFC PATCH v2 5/5] drm: imx: remove struct imx_drm_crtc and imx_drm_crtc_helper_funcs

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

With the vblank hooks in struct drm_crtc_funcs, we do not need to
maintain the CRTC specific vblank callbacks with struct
imx_drm_crtc_helper_funcs any more.  By moving the stuff that we
currently do in imx_drm_add_crtc(), like of_node setting and
drm_crtc_helper_add()/drm_crtc_init_with_planes() invoking, we can kill
things like struct imx_drm_crtc, imx_drm_crtc_helper_funcs and related
functions completely.

Functions ipu_enable_vblank() and ipu_disable_vblank() are moved around
without changes, only for saving the forward declarations.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 101 -------------------------------------
 drivers/gpu/drm/imx/imx-drm.h      |  13 -----
 drivers/gpu/drm/imx/ipuv3-crtc.c   |  58 ++++++++-------------
 3 files changed, 22 insertions(+), 150 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 33404295b447..3640c018aee4 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -40,17 +40,11 @@ struct imx_drm_component {
 
 struct imx_drm_device {
 	struct drm_device			*drm;
-	struct imx_drm_crtc			*crtc[MAX_CRTC];
 	unsigned int				pipes;
 	struct drm_fbdev_cma			*fbhelper;
 	struct drm_atomic_state			*state;
 };
 
-struct imx_drm_crtc {
-	struct drm_crtc				*crtc;
-	struct imx_drm_crtc_helper_funcs	imx_drm_helper_funcs;
-};
-
 #if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)
 static int legacyfb_depth = 16;
 module_param(legacyfb_depth, int, 0444);
@@ -63,38 +57,6 @@ static void imx_drm_driver_lastclose(struct drm_device *drm)
 	drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
 }
 
-static int imx_drm_enable_vblank(struct drm_device *drm, unsigned int pipe)
-{
-	struct imx_drm_device *imxdrm = drm->dev_private;
-	struct imx_drm_crtc *imx_drm_crtc = imxdrm->crtc[pipe];
-	int ret;
-
-	if (!imx_drm_crtc)
-		return -EINVAL;
-
-	if (!imx_drm_crtc->imx_drm_helper_funcs.enable_vblank)
-		return -ENOSYS;
-
-	ret = imx_drm_crtc->imx_drm_helper_funcs.enable_vblank(
-			imx_drm_crtc->crtc);
-
-	return ret;
-}
-
-static void imx_drm_disable_vblank(struct drm_device *drm, unsigned int pipe)
-{
-	struct imx_drm_device *imxdrm = drm->dev_private;
-	struct imx_drm_crtc *imx_drm_crtc = imxdrm->crtc[pipe];
-
-	if (!imx_drm_crtc)
-		return;
-
-	if (!imx_drm_crtc->imx_drm_helper_funcs.disable_vblank)
-		return;
-
-	imx_drm_crtc->imx_drm_helper_funcs.disable_vblank(imx_drm_crtc->crtc);
-}
-
 static const struct file_operations imx_drm_driver_fops = {
 	.owner = THIS_MODULE,
 	.open = drm_open,
@@ -180,67 +142,6 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 	.atomic_commit_tail = imx_drm_atomic_commit_tail,
 };
 
-/*
- * imx_drm_add_crtc - add a new crtc
- */
-int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
-		struct imx_drm_crtc **new_crtc, struct drm_plane *primary_plane,
-		const struct imx_drm_crtc_helper_funcs *imx_drm_helper_funcs,
-		struct device_node *port)
-{
-	struct imx_drm_device *imxdrm = drm->dev_private;
-	struct imx_drm_crtc *imx_drm_crtc;
-
-	/*
-	 * The vblank arrays are dimensioned by MAX_CRTC - we can't
-	 * pass IDs greater than this to those functions.
-	 */
-	if (imxdrm->pipes >= MAX_CRTC)
-		return -EINVAL;
-
-	if (imxdrm->drm->open_count)
-		return -EBUSY;
-
-	imx_drm_crtc = kzalloc(sizeof(*imx_drm_crtc), GFP_KERNEL);
-	if (!imx_drm_crtc)
-		return -ENOMEM;
-
-	imx_drm_crtc->imx_drm_helper_funcs = *imx_drm_helper_funcs;
-	imx_drm_crtc->crtc = crtc;
-
-	crtc->port = port;
-
-	imxdrm->crtc[imxdrm->pipes++] = imx_drm_crtc;
-
-	*new_crtc = imx_drm_crtc;
-
-	drm_crtc_helper_add(crtc,
-			imx_drm_crtc->imx_drm_helper_funcs.crtc_helper_funcs);
-
-	drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
-			imx_drm_crtc->imx_drm_helper_funcs.crtc_funcs, NULL);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(imx_drm_add_crtc);
-
-/*
- * imx_drm_remove_crtc - remove a crtc
- */
-int imx_drm_remove_crtc(struct imx_drm_crtc *imx_drm_crtc)
-{
-	struct imx_drm_device *imxdrm = imx_drm_crtc->crtc->dev->dev_private;
-	unsigned int pipe = drm_crtc_index(imx_drm_crtc->crtc);
-
-	drm_crtc_cleanup(imx_drm_crtc->crtc);
-
-	imxdrm->crtc[pipe] = NULL;
-
-	kfree(imx_drm_crtc);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(imx_drm_remove_crtc);
 
 int imx_drm_encoder_parse_of(struct drm_device *drm,
 	struct drm_encoder *encoder, struct device_node *np)
@@ -289,8 +190,6 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
 	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
-	.enable_vblank		= imx_drm_enable_vblank,
-	.disable_vblank		= imx_drm_disable_vblank,
 	.ioctls			= imx_drm_ioctls,
 	.num_ioctls		= ARRAY_SIZE(imx_drm_ioctls),
 	.fops			= &imx_drm_driver_fops,
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index 5a91cb16c8fa..cc003334505d 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -25,19 +25,6 @@ static inline struct imx_crtc_state *to_imx_crtc_state(struct drm_crtc_state *s)
 {
 	return container_of(s, struct imx_crtc_state, base);
 }
-
-struct imx_drm_crtc_helper_funcs {
-	int (*enable_vblank)(struct drm_crtc *crtc);
-	void (*disable_vblank)(struct drm_crtc *crtc);
-	const struct drm_crtc_helper_funcs *crtc_helper_funcs;
-	const struct drm_crtc_funcs *crtc_funcs;
-};
-
-int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
-		struct imx_drm_crtc **new_crtc, struct drm_plane *primary_plane,
-		const struct imx_drm_crtc_helper_funcs *imx_helper_funcs,
-		struct device_node *port);
-int imx_drm_remove_crtc(struct imx_drm_crtc *);
 int imx_drm_init_drm(struct platform_device *pdev,
 		int preferred_bpp);
 int imx_drm_exit_drm(void);
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 6be515a9fb69..a3f2843b78cd 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -129,18 +129,31 @@ static void imx_drm_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(to_imx_crtc_state(state));
 }
 
-static void imx_drm_crtc_destroy(struct drm_crtc *crtc)
+static int ipu_enable_vblank(struct drm_crtc *crtc)
+{
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+
+	enable_irq(ipu_crtc->irq);
+
+	return 0;
+}
+
+static void ipu_disable_vblank(struct drm_crtc *crtc)
 {
-	imx_drm_remove_crtc(to_ipu_crtc(crtc)->imx_crtc);
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+
+	disable_irq_nosync(ipu_crtc->irq);
 }
 
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
-	.destroy = imx_drm_crtc_destroy,
+	.destroy = drm_crtc_cleanup,
 	.page_flip = drm_atomic_helper_page_flip,
 	.reset = imx_drm_crtc_reset,
 	.atomic_duplicate_state = imx_drm_crtc_duplicate_state,
 	.atomic_destroy_state = imx_drm_crtc_destroy_state,
+	.enable_vblank = ipu_enable_vblank,
+	.disable_vblank = ipu_disable_vblank,
 };
 
 static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
@@ -261,29 +274,6 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	.enable = ipu_crtc_enable,
 };
 
-static int ipu_enable_vblank(struct drm_crtc *crtc)
-{
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-
-	enable_irq(ipu_crtc->irq);
-
-	return 0;
-}
-
-static void ipu_disable_vblank(struct drm_crtc *crtc)
-{
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-
-	disable_irq_nosync(ipu_crtc->irq);
-}
-
-static const struct imx_drm_crtc_helper_funcs ipu_crtc_helper_funcs = {
-	.enable_vblank = ipu_enable_vblank,
-	.disable_vblank = ipu_disable_vblank,
-	.crtc_funcs = &ipu_crtc_funcs,
-	.crtc_helper_funcs = &ipu_helper_funcs,
-};
-
 static void ipu_put_resources(struct ipu_crtc *ipu_crtc)
 {
 	if (!IS_ERR_OR_NULL(ipu_crtc->dc))
@@ -321,6 +311,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 	struct ipu_client_platformdata *pdata, struct drm_device *drm)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
+	struct drm_crtc *crtc = &ipu_crtc->base;
 	int dp = -EINVAL;
 	int ret;
 
@@ -340,19 +331,16 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 		goto err_put_resources;
 	}
 
-	ret = imx_drm_add_crtc(drm, &ipu_crtc->base, &ipu_crtc->imx_crtc,
-			&ipu_crtc->plane[0]->base, &ipu_crtc_helper_funcs,
-			pdata->of_node);
-	if (ret) {
-		dev_err(ipu_crtc->dev, "adding crtc failed with %d.\n", ret);
-		goto err_put_resources;
-	}
+	crtc->port = pdata->of_node;
+	drm_crtc_helper_add(crtc, &ipu_helper_funcs);
+	drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base, NULL,
+				  &ipu_crtc_funcs, NULL);
 
 	ret = ipu_plane_get_resources(ipu_crtc->plane[0]);
 	if (ret) {
 		dev_err(ipu_crtc->dev, "getting plane 0 resources failed with %d.\n",
 			ret);
-		goto err_remove_crtc;
+		goto err_put_resources;
 	}
 
 	/* If this crtc is using the DP, add an overlay plane */
@@ -390,8 +378,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 		ipu_plane_put_resources(ipu_crtc->plane[1]);
 err_put_plane0_res:
 	ipu_plane_put_resources(ipu_crtc->plane[0]);
-err_remove_crtc:
-	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
 err_put_resources:
 	ipu_put_resources(ipu_crtc);
 
-- 
1.9.1

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

Re: [RFC PATCH v2 1/5] drm: add vblank hooks to struct drm_crtc_funcs

[Daniel Vetter]

On Sun, Jan 22, 2017 at 02:09:02PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The vblank is mostly CRTC specific and implemented as part of CRTC
> driver.  Let's keep the vblank hooks struct drm_driver for legacy
> drivers, and add corresponding hooks in struct drm_crtc_funcs.  These
> hooks take struct drm_crtc pointer as argument, and will be called by
> core vblank handling code for MODESET drivers.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Looks great. Just a few tiny nits below.

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++------
>  include/drm/drm_crtc.h    | 34 ++++++++++++++++++++++++++++++
>  include/drm/drm_drv.h     |  9 ++++++++
>  3 files changed, 90 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 88c69e71102e..604a2acda9d6 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -89,6 +89,19 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  	write_sequnlock(&vblank->seqlock);
>  }
>  
> +static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +{
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +
> +		if (crtc->funcs->get_vblank_counter)
> +			return crtc->funcs->get_vblank_counter(crtc);
> +	}
> +
> +	/* fallback for legacy drivers */

Bikeshed: I'd drop these comments, it's kinda obvious what's going on.

> +	return dev->driver->get_vblank_counter(dev, pipe);

I think a great follow-up here would be to do

	if (dev->driver->get_vblank_counter)
		return dev->driver->get_vblank_counter(dev, pipe);
	
	return drm_vblank_no_hw_counter(dev,pipe);

here (i.e. making get_vblank_counter optional), and then removing
drm_vblank_no_hw_counter from all drivers. We might even want to unexport
it, and adjust the help-text to state that hw without a vblank counter
should leave this hook as NULL.

> +}
> +
>  /*
>   * Reset the stored timestamp for the current vblank count to correspond
>   * to the last vblank occurred.
> @@ -112,9 +125,9 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
>  	 * when drm_vblank_enable() applies the diff
>  	 */
>  	do {
> -		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
> +		cur_vblank = __get_vblank_counter(dev, pipe);
>  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0);
> -	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
> +	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
>  
>  	/*
>  	 * Only reinitialize corresponding vblank timestamp if high-precision query
> @@ -168,9 +181,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	 * corresponding vblank timestamp.
>  	 */
>  	do {
> -		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
> +		cur_vblank = __get_vblank_counter(dev, pipe);
>  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags);
> -	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
> +	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
>  
>  	if (dev->max_vblank_count != 0) {
>  		/* trust the hw counter when it's around */
> @@ -275,6 +288,21 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_accurate_vblank_count);
>  
> +static void __disable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +
> +		if (crtc->funcs->disable_vblank) {
> +			crtc->funcs->disable_vblank(crtc);
> +			return;
> +		}
> +	}
> +
> +	/* fallback for legacy drivers */
> +	dev->driver->disable_vblank(dev, pipe);
> +}
> +
>  /*
>   * Disable vblank irq's on crtc, make sure that last vblank count
>   * of hardware and corresponding consistent software vblank counter
> @@ -298,7 +326,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
>  	 * hardware potentially runtime suspended.
>  	 */
>  	if (vblank->enabled) {
> -		dev->driver->disable_vblank(dev, pipe);
> +		__disable_vblank(dev, pipe);
>  		vblank->enabled = false;
>  	}
>  
> @@ -1054,6 +1082,19 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
>  
> +static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +
> +		if (crtc->funcs->enable_vblank)
> +			return crtc->funcs->enable_vblank(crtc);
> +	}
> +
> +	/* fallback for legacy drivers */
> +	return dev->driver->enable_vblank(dev, pipe);
> +}
> +
>  /**
>   * drm_vblank_enable - enable the vblank interrupt on a CRTC
>   * @dev: DRM device
> @@ -1079,7 +1120,7 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
>  		 * timestamps. Filtercode in drm_handle_vblank() will
>  		 * prevent double-accounting of same vblank interval.
>  		 */
> -		ret = dev->driver->enable_vblank(dev, pipe);
> +		ret = __enable_vblank(dev, pipe);
>  		DRM_DEBUG("enabling vblank on crtc %u, ret: %d\n", pipe, ret);
>  		if (ret)
>  			atomic_dec(&vblank->refcount);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 06c943d1e04c..8b2f7266eada 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -600,6 +600,40 @@ struct drm_crtc_funcs {
>  	 */
>  	void (*atomic_print_state)(struct drm_printer *p,
>  				   const struct drm_crtc_state *state);
> +
> +	/**
> +	 * @get_vblank_counter:
> +	 *
> +	 * Driver callback for fetching a raw hardware vblank counter for the
> +	 * CRTC. It's meant to be used by new drivers as the replacement of
> +	 * &drm_driver.get_vblank_counter hook.


Compared to the kerneldoc in drm_drv.h it misses the paragraph about
wrap-around handling. Also this is missing the note about what to do if
you don't have a vblank counter, but I think making the hook optional like
I suggested above would be even better. If you do decided to make the hook
optional, then please state that in the kernel-doc here too.

> +	 *
> +	 * Returns:
> +	 *
> +	 * Raw vblank counter value.
> +	 */
> +	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
> +
> +	/**
> +	 * @enable_vblank:
> +	 *
> +	 * Enable vblank interrupts for the CRTC. It's meant to be used by
> +	 * new drivers as the replacement of &drm_driver.enable_vblank hook.
> +	 *
> +	 * Returns:
> +	 *
> +	 * Zero on success, appropriate errno if the vblank interrupt cannot
> +	 * be enabled.
> +	 */
> +	int (*enable_vblank)(struct drm_crtc *crtc);
> +
> +	/**
> +	 * @disable_vblank:
> +	 *
> +	 * Disable vblank interrupts for the CRTC. It's meant to be used by
> +	 * new drivers as the replacement of &drm_driver.disable_vblank hook.
> +	 */
> +	void (*disable_vblank)(struct drm_crtc *crtc);
>  };
>  
>  /**
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 34ece393c639..6c325baddc40 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -123,6 +123,9 @@ struct drm_driver {
>  	 * drm_crtc_vblank_off() and drm_crtc_vblank_on() when disabling or
>  	 * enabling a CRTC.
>  	 *
> +	 * This is deprecated and should not be used by new drivers.
> +	 * Use &drm_crtc_funcs.get_vblank_counter instead.
> +	 *
>  	 * Returns:
>  	 *
>  	 * Raw vblank counter value.
> @@ -135,6 +138,9 @@ struct drm_driver {
>  	 * Enable vblank interrupts for the CRTC specified with the pipe
>  	 * argument.
>  	 *
> +	 * This is deprecated and should not be used by new drivers.
> +	 * Use &drm_crtc_funcs.enable_vblank instead.
> +	 *
>  	 * Returns:
>  	 *
>  	 * Zero on success, appropriate errno if the given @crtc's vblank
> @@ -147,6 +153,9 @@ struct drm_driver {
>  	 *
>  	 * Disable vblank interrupts for the CRTC specified with the pipe
>  	 * argument.
> +	 *
> +	 * This is deprecated and should not be used by new drivers.
> +	 * Use &drm_crtc_funcs.disable_vblank instead.
>  	 */
>  	void (*disable_vblank) (struct drm_device *dev, unsigned int pipe);
>  
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v2 0/5] Add vblank hooks to struct drm_crtc_funcs

[Daniel Vetter]

On Sun, Jan 22, 2017 at 02:09:01PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The vblank is mostly CRTC specific and implemented as part of CRTC
> driver.  The first patch adds 3 vblank core-driver hooks into struct
> drm_crtc_funcs, and wraps around core vblank handling code to use the
> new hooks for modern MODESET drivers and the ones in struct drm_driver
> as fallback for legacy drivers.
> 
> The other patches in the series are to demonstrate how the new hooks
> are going to influence the driver code.  There are more drivers than
> the ones included here can be converted.  But before doing that, I would
> like to get some feedbacks first, expecially on how .get_vblank_counter
> should be converted when it's being drm_vblank_no_hw_counter().
> 
> 	.get_vblank_counter = drm_vblank_no_hw_counter

I dropped some suggestions about this onto patch 3. Thanks for doing this,
I think it looks rather pretty.
-Daniel

> The series is generated against branch drm-misc-next.
> 
> Changes for v2:
>  - Wrap around core vblank handling code to save
>    drm_crtc_enable[disable]_vblank() helpers
>  - Add .get_vblank_counter to struct drm_crtc_funcs
>  - Add some comments to link between two sets of hooks
>  - Add one hdlcd driver patch for example
> 
> Shawn Guo (5):
>   drm: add vblank hooks to struct drm_crtc_funcs
>   drm: hdlcd: use vblank hooks in struct drm_crtc_funcs
>   drm: zte: zx_vou_enable[disable]_vblank can be static
>   drm: rockchip: remove struct rockchip_crtc_funcs
>   drm: imx: remove struct imx_drm_crtc and imx_drm_crtc_helper_funcs
> 
>  drivers/gpu/drm/arm/hdlcd_crtc.c            |  20 ++++++
>  drivers/gpu/drm/arm/hdlcd_drv.c             |  20 ------
>  drivers/gpu/drm/drm_irq.c                   |  53 +++++++++++++--
>  drivers/gpu/drm/imx/imx-drm-core.c          | 101 ----------------------------
>  drivers/gpu/drm/imx/imx-drm.h               |  13 ----
>  drivers/gpu/drm/imx/ipuv3-crtc.c            |  58 ++++++----------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  51 --------------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  14 ----
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   9 +--
>  drivers/gpu/drm/zte/zx_drm_drv.c            |   2 -
>  drivers/gpu/drm/zte/zx_vou.c                |  61 +++++++----------
>  drivers/gpu/drm/zte/zx_vou.h                |   3 -
>  include/drm/drm_crtc.h                      |  34 ++++++++++
>  include/drm/drm_drv.h                       |   9 +++
>  14 files changed, 157 insertions(+), 291 deletions(-)
> 
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v2 2/5] drm: hdlcd: use vblank hooks in struct drm_crtc_funcs

[Liviu Dudau]

On Sun, Jan 22, 2017 at 02:09:03PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The vblank hooks in struct drm_driver are deprecated and only meant for
> legacy drivers.  For modern drivers with DRIVER_MODESET flag, the hooks
> in struct drm_crtc_funcs should be used instead.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/arm/hdlcd_drv.c  | 20 --------------------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 20ebfb4fbdfa..798a3cc480a2 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -42,6 +42,24 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
>  	drm_crtc_cleanup(crtc);
>  }
>  
> +static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> +	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> +
> +	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
> +
> +	return 0;
> +}
> +
> +static void hdlcd_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> +	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> +
> +	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
> +}
> +
>  static const struct drm_crtc_funcs hdlcd_crtc_funcs = {
>  	.destroy = hdlcd_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
> @@ -49,6 +67,8 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
>  	.reset = drm_atomic_helper_crtc_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.enable_vblank = hdlcd_crtc_enable_vblank,
> +	.disable_vblank = hdlcd_crtc_disable_vblank,
>  };
>  
>  static struct simplefb_format supported_formats[] = SIMPLEFB_FORMATS;
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index e5f4f4a6546d..6598ba4cc958 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -199,24 +199,6 @@ static void hdlcd_irq_uninstall(struct drm_device *drm)
>  	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
>  }
>  
> -static int hdlcd_enable_vblank(struct drm_device *drm, unsigned int crtc)
> -{
> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> -	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> -
> -	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
> -
> -	return 0;
> -}
> -
> -static void hdlcd_disable_vblank(struct drm_device *drm, unsigned int crtc)
> -{
> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> -	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> -
> -	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
> -}
> -
>  #ifdef CONFIG_DEBUG_FS
>  static int hdlcd_show_underrun_count(struct seq_file *m, void *arg)
>  {
> @@ -285,8 +267,6 @@ static void hdlcd_debugfs_cleanup(struct drm_minor *minor)
>  	.irq_postinstall = hdlcd_irq_postinstall,
>  	.irq_uninstall = hdlcd_irq_uninstall,
>  	.get_vblank_counter = drm_vblank_no_hw_counter,
> -	.enable_vblank = hdlcd_enable_vblank,
> -	.disable_vblank = hdlcd_disable_vblank,
>  	.gem_free_object_unlocked = drm_gem_cma_free_object,
>  	.gem_vm_ops = &drm_gem_cma_vm_ops,
>  	.dumb_create = drm_gem_cma_dumb_create,
> -- 
> 1.9.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v2 0/5] Add vblank hooks to struct drm_crtc_funcs

[Daniel Vetter]

On Tue, Jan 24, 2017 at 08:55:35AM +0100, Daniel Vetter wrote:
> On Sun, Jan 22, 2017 at 02:09:01PM +0800, Shawn Guo wrote:
> > From: Shawn Guo <shawn.guo@linaro.org>
> > 
> > The vblank is mostly CRTC specific and implemented as part of CRTC
> > driver.  The first patch adds 3 vblank core-driver hooks into struct
> > drm_crtc_funcs, and wraps around core vblank handling code to use the
> > new hooks for modern MODESET drivers and the ones in struct drm_driver
> > as fallback for legacy drivers.
> > 
> > The other patches in the series are to demonstrate how the new hooks
> > are going to influence the driver code.  There are more drivers than
> > the ones included here can be converted.  But before doing that, I would
> > like to get some feedbacks first, expecially on how .get_vblank_counter
> > should be converted when it's being drm_vblank_no_hw_counter().
> > 
> > 	.get_vblank_counter = drm_vblank_no_hw_counter
> 
> I dropped some suggestions about this onto patch 3. Thanks for doing this,
> I think it looks rather pretty.

Just to check: Do you plan to resend this series with my comments on the
first patch addressed? I really like this, so I guess you could drop the
RFC part and then we give maintainers a week or two for reviews and land
it into drm-misc for 4.12 ...
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v2 0/5] Add vblank hooks to struct drm_crtc_funcs

[Shawn Guo]

On Tue, Feb 07, 2017 at 08:32:46AM +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 08:55:35AM +0100, Daniel Vetter wrote:
> > On Sun, Jan 22, 2017 at 02:09:01PM +0800, Shawn Guo wrote:
> > > From: Shawn Guo <shawn.guo@linaro.org>
> > > 
> > > The vblank is mostly CRTC specific and implemented as part of CRTC
> > > driver.  The first patch adds 3 vblank core-driver hooks into struct
> > > drm_crtc_funcs, and wraps around core vblank handling code to use the
> > > new hooks for modern MODESET drivers and the ones in struct drm_driver
> > > as fallback for legacy drivers.
> > > 
> > > The other patches in the series are to demonstrate how the new hooks
> > > are going to influence the driver code.  There are more drivers than
> > > the ones included here can be converted.  But before doing that, I would
> > > like to get some feedbacks first, expecially on how .get_vblank_counter
> > > should be converted when it's being drm_vblank_no_hw_counter().
> > > 
> > > 	.get_vblank_counter = drm_vblank_no_hw_counter
> > 
> > I dropped some suggestions about this onto patch 3. Thanks for doing this,
> > I think it looks rather pretty.
> 
> Just to check: Do you plan to resend this series with my comments on the
> first patch addressed? I really like this, so I guess you could drop the
> RFC part and then we give maintainers a week or two for reviews and land
> it into drm-misc for 4.12 ...

Sorry for being late due to Chinese New Year Holidays.  The new version
is on the way, and will be on the list in a couple of hours.

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