[PATCH drm/hisilicon v2 0/3] hibmc clean up and code refactoring

[PATCH drm/hisilicon v2 0/3] hibmc clean up and code refactoring

[Tian Tao]

patch #1 and #3 is clean up, patch #2 is for code refactoring

Changes since v1:
- Rewrite the commits messages and patch name in #1
- Rewrite the commits message in #2.
- Add the new patch #3

Tian Tao (3):
  drm/hisilicon: Remove the unused include statements
  drm/hisilicon: Code refactoring for hibmc_drv_de
  drm/hisilicon: Rename variables to represent the correct meaning

 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c   | 58 ++++++------------------
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c  |  5 --
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  |  2 +
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c |  2 -
 4 files changed, 15 insertions(+), 52 deletions(-)

-- 
2.7.4

[PATCH drm/hisilicon v2 1/3] drm/hisilicon: Remove the unused include statements

[Tian Tao]

Remove some unused include statements.

v2:
edit patch name and commit message.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c   | 3 ---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c  | 5 -----
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 2 --
 3 files changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index cc70e83..66132eb 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -17,9 +17,6 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_vram_helper.h>
-#include <drm/drm_plane_helper.h>
-#include <drm/drm_print.h>
-#include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "hibmc_drm_drv.h"
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index b8d839a..54f6144 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -11,18 +11,13 @@
  *	Jianhua Li <lijianhua@huawei.com>
  */
 
-#include <linux/console.h>
-#include <linux/module.h>
 #include <linux/pci.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
-#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_irq.h>
 #include <drm/drm_managed.h>
-#include <drm/drm_print.h>
-#include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "hibmc_drm_drv.h"
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 2ca69c3..ed12f61 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -11,10 +11,8 @@
  *	Jianhua Li <lijianhua@huawei.com>
  */
 
-#include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_crtc_helper.h>
 #include <drm/drm_print.h>
 
 #include "hibmc_drm_drv.h"
-- 
2.7.4

[PATCH drm/hisilicon v2 2/3] drm/hisilicon: Code refactoring for hibmc_drv_de

[Tian Tao]

The memory used to be allocated with devres helpers and released
automatically. In rare circumstances, the memory's release could
have happened before the DRM device got released, which would have
caused memory corruption of some kind. Now we're embedding the data
structures in struct hibmc_drm_private. The whole release problem
has been resolved, because struct hibmc_drm_private is allocated
with drmm_kzalloc and always released with the DRM device.

v2:
edit commit message.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 55 ++++++-------------------
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |  2 +
 2 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 66132eb..af24c72 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -157,37 +157,6 @@ static const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
 	.atomic_update = hibmc_plane_atomic_update,
 };
 
-static struct drm_plane *hibmc_plane_init(struct hibmc_drm_private *priv)
-{
-	struct drm_device *dev = priv->dev;
-	struct drm_plane *plane;
-	int ret = 0;
-
-	plane = devm_kzalloc(dev->dev, sizeof(*plane), GFP_KERNEL);
-	if (!plane) {
-		DRM_ERROR("failed to alloc memory when init plane\n");
-		return ERR_PTR(-ENOMEM);
-	}
-	/*
-	 * plane init
-	 * TODO: Now only support primary plane, overlay planes
-	 * need to do.
-	 */
-	ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
-				       channel_formats1,
-				       ARRAY_SIZE(channel_formats1),
-				       NULL,
-				       DRM_PLANE_TYPE_PRIMARY,
-				       NULL);
-	if (ret) {
-		DRM_ERROR("failed to init plane: %d\n", ret);
-		return ERR_PTR(ret);
-	}
-
-	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
-	return plane;
-}
-
 static void hibmc_crtc_dpms(struct drm_crtc *crtc, int dpms)
 {
 	struct hibmc_drm_private *priv = crtc->dev->dev_private;
@@ -534,22 +503,24 @@ static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
 int hibmc_de_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = priv->dev;
-	struct drm_crtc *crtc;
-	struct drm_plane *plane;
+	struct drm_crtc *crtc = &priv->crtc;
+	struct drm_plane *plane = &priv->plane;
 	int ret;
 
-	plane = hibmc_plane_init(priv);
-	if (IS_ERR(plane)) {
-		DRM_ERROR("failed to create plane: %ld\n", PTR_ERR(plane));
-		return PTR_ERR(plane);
-	}
+	ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
+				       channel_formats1,
+				       ARRAY_SIZE(channel_formats1),
+				       NULL,
+				       DRM_PLANE_TYPE_PRIMARY,
+				       NULL);
 
-	crtc = devm_kzalloc(dev->dev, sizeof(*crtc), GFP_KERNEL);
-	if (!crtc) {
-		DRM_ERROR("failed to alloc memory when init crtc\n");
-		return -ENOMEM;
+	if (ret) {
+		DRM_ERROR("failed to init plane: %d\n", ret);
+		return ret;
 	}
 
+	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
+
 	ret = drm_crtc_init_with_planes(dev, crtc, plane,
 					NULL, &hibmc_crtc_funcs, NULL);
 	if (ret) {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index a683763..91ef15c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -28,6 +28,8 @@ struct hibmc_drm_private {
 
 	/* drm */
 	struct drm_device  *dev;
+	struct drm_plane plane;
+	struct drm_crtc crtc;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
 	bool mode_config_initialized;
-- 
2.7.4

[PATCH drm/hisilicon v2 3/3] drm/hisilicon: Rename variables to represent the correct meaning

[Tian Tao]

Rename plane to primary_plane in the structure hibmc_drm_private.
so it's clear which plane it represents.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index af24c72..d9062a3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -504,7 +504,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = priv->dev;
 	struct drm_crtc *crtc = &priv->crtc;
-	struct drm_plane *plane = &priv->plane;
+	struct drm_plane *plane = &priv->primary_plane;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index 91ef15c..197485e 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -28,7 +28,7 @@ struct hibmc_drm_private {
 
 	/* drm */
 	struct drm_device  *dev;
-	struct drm_plane plane;
+	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
-- 
2.7.4

Re: [PATCH drm/hisilicon v2 0/3] hibmc clean up and code refactoring

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1316 bytes --]

Hi

Am 03.08.20 um 02:38 schrieb Tian Tao:
> patch #1 and #3 is clean up, patch #2 is for code refactoring

Sorry for all my reviews taking so long. Please merge patch #3 into
patch #2 and then the series is

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

I noticed that hibmc use DRM_ERROR in several places. A good follow-up
patchset would be the conversion to drm_info/drm_warn/drm_error/etc.

Best regards
Thomas

> 
> Changes since v1:
> - Rewrite the commits messages and patch name in #1
> - Rewrite the commits message in #2.
> - Add the new patch #3
> 
> Tian Tao (3):
>   drm/hisilicon: Remove the unused include statements
>   drm/hisilicon: Code refactoring for hibmc_drv_de
>   drm/hisilicon: Rename variables to represent the correct meaning
> 
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c   | 58 ++++++------------------
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c  |  5 --
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  |  2 +
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c |  2 -
>  4 files changed, 15 insertions(+), 52 deletions(-)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

Re: [PATCH drm/hisilicon v2 0/3] hibmc clean up and code refactoring

[tiantao (H)]

在 2020/8/12 15:04, Thomas Zimmermann 写道:
> Hi
> 
> Am 03.08.20 um 02:38 schrieb Tian Tao:
>> patch #1 and #3 is clean up, patch #2 is for code refactoring
> 
> Sorry for all my reviews taking so long. Please merge patch #3 into
> patch #2 and then the series is
> 
thanks,I will send v3 to fix this.
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I noticed that hibmc use DRM_ERROR in several places. A good follow-up
> patchset would be the conversion to drm_info/drm_warn/drm_error/etc.
> 
I will send another patchset to fix this.
> Best regards
> Thomas
> 
>>
>> Changes since v1:
>> - Rewrite the commits messages and patch name in #1
>> - Rewrite the commits message in #2.
>> - Add the new patch #3
>>
>> Tian Tao (3):
>>    drm/hisilicon: Remove the unused include statements
>>    drm/hisilicon: Code refactoring for hibmc_drv_de
>>    drm/hisilicon: Rename variables to represent the correct meaning
>>
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c   | 58 ++++++------------------
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c  |  5 --
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  |  2 +
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c |  2 -
>>   4 files changed, 15 insertions(+), 52 deletions(-)
>>
>