| Message ID | 1583466184-7060-5-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/hisilicon: Enforce 128-byte stride alignment to fix the hardware limitation | expand |
Hi Tao, On Fri, 6 Mar 2020 at 11:44, Tian Tao <tiantao6@hisilicon.com> wrote: > > code cleanup for hibmc_drv_vdac.c, no actual function changes. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > Signed-off-by: Gong junjie <gongjunjie2@huawei.com> > --- > 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 >
Hi Am 03.04.20 um 04:26 schrieb Xinliang Liu: > Hi Tao, > > On Fri, 6 Mar 2020 at 11:44, Tian Tao <tiantao6@hisilicon.com> wrote: >> >> code cleanup for hibmc_drv_vdac.c, no actual function changes. >> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >> Signed-off-by: Gong junjie <gongjunjie2@huawei.com> >> --- >> 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 > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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;