[PATCH 0/4] Add vblank hooks to struct drm_crtc_funcs

[PATCH 0/4] Add vblank hooks to struct drm_crtc_funcs

[Shawn Guo]

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

Although it can help to clean up driver code quite a bit, I'm not sure
it's been done in the right way.  So the series can be treated as RFC.

When I was going through DRM drivers for candidates of using
drm_crtc_from_index() helper, I found vblank handling is mostly a CRTC
specific thing.  A few drivers (imx and rockchip) even have defined
their own hooks and registration functions to handle per-CRTC vblank
callbacks.  The series is trying to demonstrate how these driver code
can be cleaned up by simply adding a couple of vblank hooks to
struct drm_crtc_funcs.

The ZTE DRM driver doesn't have per-CRTC vblank callbacks.  It uses the
same pair of vblank functions for both CRTCs.  But it's being included
here to show the benefit of saving vblank function exporting.

Rockchip is compile-tested, while ZTE and IMX drivers are tested on
hardware.

Shawn Guo (4):
  drm: add vblank hooks to 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/drm_crtc.c                  |  36 ++++++++++
 drivers/gpu/drm/imx/imx-drm-core.c          | 103 +---------------------------
 drivers/gpu/drm/imx/imx-drm.h               |  13 ----
 drivers/gpu/drm/imx/ipuv3-crtc.c            |  58 ++++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  53 +-------------
 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            |   4 +-
 drivers/gpu/drm/zte/zx_vou.c                |  61 +++++++---------
 drivers/gpu/drm/zte/zx_vou.h                |   3 -
 include/drm/drm_crtc.h                      |  21 ++++++
 11 files changed, 110 insertions(+), 265 deletions(-)

-- 
1.9.1

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

[PATCH 1/4] 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.  So having vblank hooks in struct drm_crtc_funcs should
generally help to reduce code from client drivers in implementing
drm_driver's vblank callbacks.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     | 21 +++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 85a7452d0fb4..59ff00f48101 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx)
 EXPORT_SYMBOL(drm_crtc_from_index);
 
 /**
+ * drm_crtc_enable_vblank - vblank enable callback helper
+ * @dev: DRM device
+ * @pipe: CRTC index
+ *
+ * It's a helper function as the generic vblank enable callback implementation,
+ * which calls into &drm_crtc_funcs.enable_vblank function.
+ */
+int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+
+	if (crtc && crtc->funcs && crtc->funcs->enable_vblank)
+		return crtc->funcs->enable_vblank(crtc);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_enable_vblank);
+
+/**
+ * drm_crtc_disable_vblank - vblank disable callback helper
+ * @dev: DRM device
+ * @pipe: CRTC index
+ *
+ * It's a helper function as the generic vblank disable callback implementation,
+ * which calls into &drm_crtc_funcs.disable_vblank function.
+ */
+void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+
+	if (crtc && crtc->funcs && crtc->funcs->disable_vblank)
+		return crtc->funcs->disable_vblank(crtc);
+}
+EXPORT_SYMBOL(drm_crtc_disable_vblank);
+
+/**
  * drm_crtc_force_disable - Forcibly turn off a CRTC
  * @crtc: CRTC to turn off
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a5627eb8621f..20874b3903a6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -599,6 +599,25 @@ struct drm_crtc_funcs {
 	 */
 	void (*atomic_print_state)(struct drm_printer *p,
 				   const struct drm_crtc_state *state);
+
+	/**
+	 * @enable_vblank:
+	 *
+	 * Enable vblank interrupts for the CRTC.
+	 *
+	 * 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.
+	 */
+	void (*disable_vblank)(struct drm_crtc *crtc);
 };
 
 /**
@@ -831,6 +850,8 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
 
 int drm_mode_set_config_internal(struct drm_mode_set *set);
 struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
+int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
+void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
 
 /* Helpers */
 static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
-- 
1.9.1

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

Re: [PATCH 1/4] drm: add vblank hooks to struct drm_crtc_funcs

[Daniel Vetter]

On Mon, Jan 09, 2017 at 07:56:24PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The vblank is mostly CRTC specific and implemented as part of CRTC
> driver.  So having vblank hooks in struct drm_crtc_funcs should
> generally help to reduce code from client drivers in implementing
> drm_driver's vblank callbacks.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 21 +++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 85a7452d0fb4..59ff00f48101 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx)
>  EXPORT_SYMBOL(drm_crtc_from_index);
>  
>  /**
> + * drm_crtc_enable_vblank - vblank enable callback helper
> + * @dev: DRM device
> + * @pipe: CRTC index
> + *
> + * It's a helper function as the generic vblank enable callback implementation,
> + * which calls into &drm_crtc_funcs.enable_vblank function.
> + */
> +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +
> +	if (crtc && crtc->funcs && crtc->funcs->enable_vblank)
> +		return crtc->funcs->enable_vblank(crtc);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_enable_vblank);

With the helper approach here there's still a pile of boilerplate in
drivers (well, 2 lines to fill out the legacy helpers). What if instead we
wrap all callers of enable/disable_vblank in drm_irq.c into something like

__enable_vblank(dev, pipe)
{
	if (DRIVER_MODESET) /* otherwise we'll oops on legacy drivers */
	{
		/* above code to call the new hook, if it's there. */

		if (crtc->funcs->enable_vblank)
			return crtc->funcs->enable_vblank(crtc);
	}

	/* fallback for everyone else */

	dev->driver->enable_vblank(dev, pipe);
}

> +
> +/**
> + * drm_crtc_disable_vblank - vblank disable callback helper
> + * @dev: DRM device
> + * @pipe: CRTC index
> + *
> + * It's a helper function as the generic vblank disable callback implementation,
> + * which calls into &drm_crtc_funcs.disable_vblank function.
> + */
> +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +
> +	if (crtc && crtc->funcs && crtc->funcs->disable_vblank)
> +		return crtc->funcs->disable_vblank(crtc);
> +}
> +EXPORT_SYMBOL(drm_crtc_disable_vblank);
> +
> +/**
>   * drm_crtc_force_disable - Forcibly turn off a CRTC
>   * @crtc: CRTC to turn off
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a5627eb8621f..20874b3903a6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -599,6 +599,25 @@ struct drm_crtc_funcs {
>  	 */
>  	void (*atomic_print_state)(struct drm_printer *p,
>  				   const struct drm_crtc_state *state);
> +
> +	/**
> +	 * @enable_vblank:
> +	 *
> +	 * Enable vblank interrupts for the CRTC.
> +	 *
> +	 * 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.
> +	 */
> +	void (*disable_vblank)(struct drm_crtc *crtc);

Please also update the kerneldoc for these two hooks in drm_drv.h, and
link between the two (using the &drm_driver.enable_vblank syntax). And
then mention that the drm_driver one is deprecated.

Also I think it'd would make sense to do the same for
get_vblank_counter(), since those are the 3 core->driver interface
functions. The other 2 are kinda just helpers for precise vblank
timestamp support.
-Daniel

>  };
>  
>  /**
> @@ -831,6 +850,8 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
>  
>  int drm_mode_set_config_internal(struct drm_mode_set *set);
>  struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
> +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
> +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
>  
>  /* Helpers */
>  static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
> -- 
> 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: [PATCH 1/4] drm: add vblank hooks to struct drm_crtc_funcs

[Laurent Pinchart]

On Tuesday 10 Jan 2017 11:39:03 Daniel Vetter wrote:
> On Mon, Jan 09, 2017 at 07:56:24PM +0800, Shawn Guo wrote:
> > From: Shawn Guo <shawn.guo@linaro.org>
> > 
> > The vblank is mostly CRTC specific and implemented as part of CRTC
> > driver.  So having vblank hooks in struct drm_crtc_funcs should
> > generally help to reduce code from client drivers in implementing
> > drm_driver's vblank callbacks.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > 
> >  drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h     | 21 +++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 85a7452d0fb4..59ff00f48101 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device
> > *dev, int idx)> 
> >  EXPORT_SYMBOL(drm_crtc_from_index);
> >  
> >  /**
> > 
> > + * drm_crtc_enable_vblank - vblank enable callback helper
> > + * @dev: DRM device
> > + * @pipe: CRTC index
> > + *
> > + * It's a helper function as the generic vblank enable callback
> > implementation, + * which calls into &drm_crtc_funcs.enable_vblank
> > function.
> > + */
> > +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > +{
> > +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> > +
> > +	if (crtc && crtc->funcs && crtc->funcs->enable_vblank)
> > +		return crtc->funcs->enable_vblank(crtc);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_crtc_enable_vblank);
> 
> With the helper approach here there's still a pile of boilerplate in
> drivers (well, 2 lines to fill out the legacy helpers). What if instead we
> wrap all callers of enable/disable_vblank in drm_irq.c into something like
> 
> __enable_vblank(dev, pipe)
> {
> 	if (DRIVER_MODESET) /* otherwise we'll oops on legacy drivers */
> 	{
> 		/* above code to call the new hook, if it's there. */
> 
> 		if (crtc->funcs->enable_vblank)
> 			return crtc->funcs->enable_vblank(crtc);
> 	}
> 
> 	/* fallback for everyone else */
> 
> 	dev->driver->enable_vblank(dev, pipe);
> }

FWIW I like that approach much better. I'd even go as far as saying that 
DRIVER_MODESET drivers should be mass-converted.

> > +
> > +/**
> > + * drm_crtc_disable_vblank - vblank disable callback helper
> > + * @dev: DRM device
> > + * @pipe: CRTC index
> > + *
> > + * It's a helper function as the generic vblank disable callback
> > implementation,
> > + * which calls into &drm_crtc_funcs.disable_vblank function.
> > + */
> > +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > +{
> > +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> > +
> > +	if (crtc && crtc->funcs && crtc->funcs->disable_vblank)
> > +		return crtc->funcs->disable_vblank(crtc);
> > +}
> > +EXPORT_SYMBOL(drm_crtc_disable_vblank);
> > +
> > +/**
> >   * drm_crtc_force_disable - Forcibly turn off a CRTC
> >   * @crtc: CRTC to turn off
> >   *

-- 
Regards,

Laurent Pinchart

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

[PATCH 2/4] 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 |  4 +--
 drivers/gpu/drm/zte/zx_vou.c     | 61 +++++++++++++++-------------------------
 drivers/gpu/drm/zte/zx_vou.h     |  3 --
 3 files changed, 25 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..6489d720baef 100644
--- a/drivers/gpu/drm/zte/zx_drm_drv.c
+++ b/drivers/gpu/drm/zte/zx_drm_drv.c
@@ -72,8 +72,8 @@ 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,
+	.enable_vblank = drm_crtc_enable_vblank,
+	.disable_vblank = drm_crtc_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

[PATCH 3/4] 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 | 53 ++---------------------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 14 --------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  9 ++---
 3 files changed, 4 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..51a21c5e3b78 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,8 @@ 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,
+	.enable_vblank		= drm_crtc_enable_vblank,
+	.disable_vblank		= 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 c7eba305c488..a61a7e1ce9a4 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

[PATCH 4/4] 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 | 103 +------------------------------------
 drivers/gpu/drm/imx/imx-drm.h      |  13 -----
 drivers/gpu/drm/imx/ipuv3-crtc.c   |  58 ++++++++-------------
 3 files changed, 24 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..934ca32cf8f4 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,8 @@ 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,
+	.enable_vblank		= drm_crtc_enable_vblank,
+	.disable_vblank		= drm_crtc_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