[PATCH 0/3] Add CRTC helper drm_crtc_from_index()

[PATCH 0/3] Add CRTC helper drm_crtc_from_index()

[Shawn Guo]

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

This is a suggestion from Sean Paul.  ZTE and Rockchip DRM drivers have
the similar code to find registered CRTC by given index.  The series
create a CRTC helper drm_crtc_from_index() to do the job, just like
drm_plane_from_index() to search plane, and change ZTE, Rockchip driver
to use the helper.

Shawn Guo (3):
  drm: add crtc helper drm_crtc_from_index()
  drm: zte: use crtc helper drm_crtc_from_index()
  drm: rockchip: use crtc helper drm_crtc_from_index()

 drivers/gpu/drm/drm_crtc.c                  | 20 ++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 17 ++---------------
 drivers/gpu/drm/zte/zx_vou.c                | 15 ++-------------
 include/drm/drm_crtc.h                      |  1 +
 4 files changed, 25 insertions(+), 28 deletions(-)

-- 
1.9.1

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

[PATCH 1/3] drm: add crtc helper drm_crtc_from_index()

[Shawn Guo]

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

It adds a crtc helper drm_crtc_from_index() to find the registered CRTC
with a given index, just like drm_plane_from_index().

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e75f62cd8a65..5c1bb1f34697 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -47,6 +47,26 @@
 #include "drm_internal.h"
 
 /**
+ * drm_crtc_from_index - find the registered CRTC at an index
+ * @dev: DRM device
+ * @idx: index of registered CRTC to find for
+ *
+ * Given a CRTC index, return the registered CRTC from DRM device's
+ * list of CRTCs with matching index.
+ */
+struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx)
+{
+	struct drm_crtc *crtc;
+
+	drm_for_each_crtc(crtc, dev)
+		if (idx == crtc->index)
+			return crtc;
+
+	return NULL;
+}
+EXPORT_SYMBOL(drm_crtc_from_index);
+
+/**
  * 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 946672f97e1e..a5627eb8621f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -830,6 +830,7 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
 int drm_crtc_force_disable_all(struct drm_device *dev);
 
 int drm_mode_set_config_internal(struct drm_mode_set *set);
+struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
 
 /* 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

[PATCH 2/3] drm: zte: use crtc helper drm_crtc_from_index()

[Shawn Guo]

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

Function zx_find_crtc() does the exactly same thing as what crtc helper
drm_crtc_from_index() provides.  Use the helper to save some code.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpu/drm/zte/zx_vou.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/zte/zx_vou.c b/drivers/gpu/drm/zte/zx_vou.c
index 73fe15c17c32..a86e3a5852a2 100644
--- a/drivers/gpu/drm/zte/zx_vou.c
+++ b/drivers/gpu/drm/zte/zx_vou.c
@@ -355,17 +355,6 @@ static int zx_crtc_init(struct drm_device *drm, struct zx_vou_hw *vou,
 	return 0;
 }
 
-static inline struct drm_crtc *zx_find_crtc(struct drm_device *drm, int pipe)
-{
-	struct drm_crtc *crtc;
-
-	list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
-		if (crtc->index == pipe)
-			return crtc;
-
-	return NULL;
-}
-
 int zx_vou_enable_vblank(struct drm_device *drm, unsigned int pipe)
 {
 	struct drm_crtc *crtc;
@@ -373,7 +362,7 @@ int zx_vou_enable_vblank(struct drm_device *drm, unsigned int pipe)
 	struct zx_vou_hw *vou;
 	u32 int_frame_mask;
 
-	crtc = zx_find_crtc(drm, pipe);
+	crtc = drm_crtc_from_index(drm, pipe);
 	if (!crtc)
 		return 0;
 
@@ -393,7 +382,7 @@ void zx_vou_disable_vblank(struct drm_device *drm, unsigned int pipe)
 	struct zx_crtc *zcrtc;
 	struct zx_vou_hw *vou;
 
-	crtc = zx_find_crtc(drm, pipe);
+	crtc = drm_crtc_from_index(drm, pipe);
 	if (!crtc)
 		return;
 
-- 
1.9.1

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

[PATCH 3/3] drm: rockchip: use crtc helper drm_crtc_from_index()

[Shawn Guo]

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

Function rockchip_crtc_from_pipe() does the exactly same thing as what
crtc helper drm_crtc_from_index() provides.  Use the helper to save
some code.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 2390c8577617..c30d649cb147 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -99,24 +99,11 @@ void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc)
 	priv->crtc_funcs[pipe] = NULL;
 }
 
-static struct drm_crtc *rockchip_crtc_from_pipe(struct drm_device *drm,
-						int pipe)
-{
-	struct drm_crtc *crtc;
-	int i = 0;
-
-	list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
-		if (i++ == pipe)
-			return crtc;
-
-	return 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 = rockchip_crtc_from_pipe(dev, pipe);
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
 
 	if (crtc && priv->crtc_funcs[pipe] &&
 	    priv->crtc_funcs[pipe]->enable_vblank)
@@ -129,7 +116,7 @@ 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 = rockchip_crtc_from_pipe(dev, pipe);
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
 
 	if (crtc && priv->crtc_funcs[pipe] &&
 	    priv->crtc_funcs[pipe]->enable_vblank)
-- 
1.9.1

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

Re: [PATCH 1/3] drm: add crtc helper drm_crtc_from_index()

[Daniel Vetter]

On Thu, Dec 29, 2016 at 08:41:28PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> It adds a crtc helper drm_crtc_from_index() to find the registered CRTC
> with a given index, just like drm_plane_from_index().
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/gpu/drm/drm_crtc.c | 20 ++++++++++++++++++++
>  include/drm/drm_crtc.h     |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e75f62cd8a65..5c1bb1f34697 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -47,6 +47,26 @@
>  #include "drm_internal.h"
>  
>  /**
> + * drm_crtc_from_index - find the registered CRTC at an index
> + * @dev: DRM device
> + * @idx: index of registered CRTC to find for
> + *
> + * Given a CRTC index, return the registered CRTC from DRM device's
> + * list of CRTCs with matching index.
> + */

Might be useful to go through al the drm_*_inde and drm_*_from_index
functions we have and a sentence to each likt "This is the inverse of
drm_crtc_index()." to cross link them a bit.

Also here a sentence explaining that this is useful in the vblank code,
e.g.

"This is useful in the vblank callbacks (like &drm_driver.enable_vblank or
&drm_driver.disable_vblank), since that still deals with indices instead
of pointers to &struct drm_crtc."

Follow-up pach, since I applied these here already.
-Daniel

> +struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx)
> +{
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_crtc(crtc, dev)
> +		if (idx == crtc->index)
> +			return crtc;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_crtc_from_index);
> +
> +/**
>   * 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 946672f97e1e..a5627eb8621f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -830,6 +830,7 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
>  int drm_crtc_force_disable_all(struct drm_device *dev);
>  
>  int drm_mode_set_config_internal(struct drm_mode_set *set);
> +struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
>  
>  /* 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 0/3] Add CRTC helper drm_crtc_from_index()

[Daniel Vetter]

On Thu, Dec 29, 2016 at 08:41:27PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> This is a suggestion from Sean Paul.  ZTE and Rockchip DRM drivers have
> the similar code to find registered CRTC by given index.  The series
> create a CRTC helper drm_crtc_from_index() to do the job, just like
> drm_plane_from_index() to search plane, and change ZTE, Rockchip driver
> to use the helper.
> 
> Shawn Guo (3):
>   drm: add crtc helper drm_crtc_from_index()
>   drm: zte: use crtc helper drm_crtc_from_index()
>   drm: rockchip: use crtc helper drm_crtc_from_index()

Entire series applied. I suspect that there's more drivers open-coding
something like this in their vblank code, might be worth it to grep for
them all and do a quick audit.

Thanks a lot,
-Daniel
-- 
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 0/3] Add CRTC helper drm_crtc_from_index()

[Shawn Guo]

On Fri, Dec 30, 2016 at 12:16:43PM +0100, Daniel Vetter wrote:
> Entire series applied. I suspect that there's more drivers open-coding
> something like this in their vblank code, might be worth it to grep for
> them all and do a quick audit.

I did a round of audit on all drivers vblank code, and found there are
basically 3 categories:

1. Drivers that only support one CRTC.  In this case, the 'pipe' is not
used at all in vblank code.  So we need to do thing about them.
Examples: arm, atmel-hlcdc, mxsfb, etc.

2. Drivers that maintain CRTC pointers and number in the private
structure.  And the fields are used in somewhere else beside vblank
code.  The cleanup will need a bit surgery on the driver code.  I would
leave them out for driver owners/maintainers.  Examples: armada, msm,
sti, etc.

3. Drivers that can use the helper for benefit, including exynos, kirin,
mediatek, nouveau, tegra and vc4.  I have already prepared a series for
these drivers, and will send it out for review shortly.

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

Re: [PATCH 0/3] Add CRTC helper drm_crtc_from_index()

[Daniel Vetter]

On Mon, Jan 09, 2017 at 05:03:35PM +0800, Shawn Guo wrote:
> On Fri, Dec 30, 2016 at 12:16:43PM +0100, Daniel Vetter wrote:
> > Entire series applied. I suspect that there's more drivers open-coding
> > something like this in their vblank code, might be worth it to grep for
> > them all and do a quick audit.
> 
> I did a round of audit on all drivers vblank code, and found there are
> basically 3 categories:
> 
> 1. Drivers that only support one CRTC.  In this case, the 'pipe' is not
> used at all in vblank code.  So we need to do thing about them.
> Examples: arm, atmel-hlcdc, mxsfb, etc.
> 
> 2. Drivers that maintain CRTC pointers and number in the private
> structure.  And the fields are used in somewhere else beside vblank
> code.  The cleanup will need a bit surgery on the driver code.  I would
> leave them out for driver owners/maintainers.  Examples: armada, msm,
> sti, etc.
> 
> 3. Drivers that can use the helper for benefit, including exynos, kirin,
> mediatek, nouveau, tegra and vc4.  I have already prepared a series for
> these drivers, and will send it out for review shortly.

Awesome, thanks a lot for doing this. One of my long-term goals is to
switch the vblank helper callbacks over from int pipe to struct drm_crtc
*, stuff like this helps a lot in prepping the ground.
-Daniel
-- 
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