2020-03-06 03:44:21

by Tian Tao

[permalink] [raw]
Subject: [PATCH 2/4] drm/hisilicon: Code cleanup for hibmc_drv_vdac

code cleanup for hibmc_drv_vdac.c, no actual function changes.

Signed-off-by: Tian Tao <[email protected]>
Signed-off-by: Gong junjie <[email protected]>
---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 49 ++++++++----------------
1 file changed, 16 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 678ac2e..f0e6bb8 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -52,32 +52,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};

-static struct drm_connector *
-hibmc_connector_init(struct hibmc_drm_private *priv)
-{
- struct drm_device *dev = priv->dev;
- struct drm_connector *connector;
- int ret;
-
- connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
- if (!connector) {
- DRM_ERROR("failed to alloc memory when init connector\n");
- return ERR_PTR(-ENOMEM);
- }
-
- ret = drm_connector_init(dev, connector,
- &hibmc_connector_funcs,
- DRM_MODE_CONNECTOR_VGA);
- if (ret) {
- DRM_ERROR("failed to init connector: %d\n", ret);
- return ERR_PTR(ret);
- }
- drm_connector_helper_add(connector,
- &hibmc_connector_helper_funcs);
-
- return connector;
-}
-
static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,
struct drm_display_mode *adj_mode)
@@ -109,13 +83,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
struct drm_connector *connector;
int ret;

- connector = hibmc_connector_init(priv);
- if (IS_ERR(connector)) {
- DRM_ERROR("failed to create connector: %ld\n",
- PTR_ERR(connector));
- return PTR_ERR(connector);
- }
-
encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
if (!encoder) {
DRM_ERROR("failed to alloc memory when init encoder\n");
@@ -131,6 +98,22 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
}

drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
+ connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
+ if (!connector) {
+ DRM_ERROR("failed to alloc memory when init connector\n");
+ return -ENOMEM;
+ }
+
+ ret = drm_connector_init(dev, connector,
+ &hibmc_connector_funcs,
+ DRM_MODE_CONNECTOR_VGA);
+ if (ret) {
+ DRM_ERROR("failed to init connector: %d\n", ret);
+ return ret;
+ }
+
+ drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
+ drm_connector_register(connector);
drm_connector_attach_encoder(connector, encoder);

return 0;
--
2.7.4


2020-04-03 02:26:52

by Xinliang Liu

[permalink] [raw]
Subject: Re: [PATCH 2/4] drm/hisilicon: Code cleanup for hibmc_drv_vdac

Hi Tao,

On Fri, 6 Mar 2020 at 11:44, Tian Tao <[email protected]> wrote:
>
> code cleanup for hibmc_drv_vdac.c, no actual function changes.
>
> Signed-off-by: Tian Tao <[email protected]>
> Signed-off-by: Gong junjie <[email protected]>
> ---
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 49 ++++++++----------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index 678ac2e..f0e6bb8 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -52,32 +52,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> -static struct drm_connector *
> -hibmc_connector_init(struct hibmc_drm_private *priv)
> -{
> - struct drm_device *dev = priv->dev;
> - struct drm_connector *connector;
> - int ret;
> -
> - connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
> - if (!connector) {
> - DRM_ERROR("failed to alloc memory when init connector\n");
> - return ERR_PTR(-ENOMEM);
> - }
> -
> - ret = drm_connector_init(dev, connector,
> - &hibmc_connector_funcs,
> - DRM_MODE_CONNECTOR_VGA);
> - if (ret) {
> - DRM_ERROR("failed to init connector: %d\n", ret);
> - return ERR_PTR(ret);
> - }
> - drm_connector_helper_add(connector,
> - &hibmc_connector_helper_funcs);
> -
> - return connector;
> -}
> -
> static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
> struct drm_display_mode *mode,
> struct drm_display_mode *adj_mode)
> @@ -109,13 +83,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
> struct drm_connector *connector;
> int ret;
>
> - connector = hibmc_connector_init(priv);
> - if (IS_ERR(connector)) {
> - DRM_ERROR("failed to create connector: %ld\n",
> - PTR_ERR(connector));
> - return PTR_ERR(connector);
> - }
> -
> encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
> if (!encoder) {
> DRM_ERROR("failed to alloc memory when init encoder\n");
> @@ -131,6 +98,22 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
> }
>
> drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
> + connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
> + if (!connector) {
> + DRM_ERROR("failed to alloc memory when init connector\n");
> + return -ENOMEM;
> + }
> +
> + ret = drm_connector_init(dev, connector,
> + &hibmc_connector_funcs,
> + DRM_MODE_CONNECTOR_VGA);
> + if (ret) {
> + DRM_ERROR("failed to init connector: %d\n", ret);
> + return ret;
> + }
> +
> + drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
> + drm_connector_register(connector);

You don't need to register a non-hotplug connector as it will be
registered at drm_dev_register automatically.

See function definition:

488 /**
489 * drm_connector_register - register a connector
490 * @connector: the connector to register
491 *
492 * Register userspace interfaces for a connector. Only call this
for connectors
493 * which can be hotplugged after drm_dev_register() has been
called already,
494 * e.g. DP MST connectors. All other connectors will be
registered automatically
495 * when calling drm_dev_register().
496 *
497 * Returns:
498 * Zero on success, error code on failure.
499 */
500 int drm_connector_register(struct drm_connector *connector)
501 {


Besides, I don't think this patch cleans much things.

-Xinliang

> drm_connector_attach_encoder(connector, encoder);
>
> return 0;
> --
> 2.7.4
>

2020-04-03 07:14:06

by Thomas Zimmermann

[permalink] [raw]
Subject: Re: [PATCH 2/4] drm/hisilicon: Code cleanup for hibmc_drv_vdac

Hi

Am 03.04.20 um 04:26 schrieb Xinliang Liu:
> Hi Tao,
>
> On Fri, 6 Mar 2020 at 11:44, Tian Tao <[email protected]> wrote:
>>
>> code cleanup for hibmc_drv_vdac.c, no actual function changes.
>>
>> Signed-off-by: Tian Tao <[email protected]>
>> Signed-off-by: Gong junjie <[email protected]>
>> ---
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 49 ++++++++----------------
>> 1 file changed, 16 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 678ac2e..f0e6bb8 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -52,32 +52,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> };
>>
>> -static struct drm_connector *
>> -hibmc_connector_init(struct hibmc_drm_private *priv)
>> -{
>> - struct drm_device *dev = priv->dev;
>> - struct drm_connector *connector;
>> - int ret;
>> -
>> - connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
>> - if (!connector) {
>> - DRM_ERROR("failed to alloc memory when init connector\n");
>> - return ERR_PTR(-ENOMEM);
>> - }
>> -
>> - ret = drm_connector_init(dev, connector,
>> - &hibmc_connector_funcs,
>> - DRM_MODE_CONNECTOR_VGA);
>> - if (ret) {
>> - DRM_ERROR("failed to init connector: %d\n", ret);
>> - return ERR_PTR(ret);
>> - }
>> - drm_connector_helper_add(connector,
>> - &hibmc_connector_helper_funcs);
>> -
>> - return connector;
>> -}
>> -
>> static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
>> struct drm_display_mode *mode,
>> struct drm_display_mode *adj_mode)
>> @@ -109,13 +83,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>> struct drm_connector *connector;
>> int ret;
>>
>> - connector = hibmc_connector_init(priv);
>> - if (IS_ERR(connector)) {
>> - DRM_ERROR("failed to create connector: %ld\n",
>> - PTR_ERR(connector));
>> - return PTR_ERR(connector);
>> - }
>> -
>> encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
>> if (!encoder) {
>> DRM_ERROR("failed to alloc memory when init encoder\n");
>> @@ -131,6 +98,22 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>> }
>>
>> drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>> + connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
>> + if (!connector) {
>> + DRM_ERROR("failed to alloc memory when init connector\n");
>> + return -ENOMEM;
>> + }
>> +
>> + ret = drm_connector_init(dev, connector,
>> + &hibmc_connector_funcs,
>> + DRM_MODE_CONNECTOR_VGA);
>> + if (ret) {
>> + DRM_ERROR("failed to init connector: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
>> + drm_connector_register(connector);
>
> You don't need to register a non-hotplug connector as it will be
> registered at drm_dev_register automatically.
>
> See function definition:
>
> 488 /**
> 489 * drm_connector_register - register a connector
> 490 * @connector: the connector to register
> 491 *
> 492 * Register userspace interfaces for a connector. Only call this
> for connectors
> 493 * which can be hotplugged after drm_dev_register() has been
> called already,
> 494 * e.g. DP MST connectors. All other connectors will be
> registered automatically
> 495 * when calling drm_dev_register().
> 496 *
> 497 * Returns:
> 498 * Zero on success, error code on failure.
> 499 */
> 500 int drm_connector_register(struct drm_connector *connector)
> 501 {
>
>
> Besides, I don't think this patch cleans much things.

True. Although the auto-kfree of devm_kzalloc() does not work correctly
with DRM and should be replaced.

I'd like to suggest to either allocate the connector as part of struct
hibmc_drm_private, or implement drmm_connector_create() on top of DRM's
new auto-cleanup functionality.

Best regards
Thomas

>
> -Xinliang
>
>> drm_connector_attach_encoder(connector, encoder);
>>
>> return 0;
>> --
>> 2.7.4
>>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

--
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


Attachments:
signature.asc (499.00 B)
OpenPGP digital signature