| Message ID | 1586586315-12764-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/hisilicon: Code refactoring for hibmc_drv_vdac | expand |
Hi, sorry for letting these patches sit around for so long. I'd say that if you waited for a week or two without further responses, it's ok to ping people. Am 11.04.20 um 08:25 schrieb Tian Tao: > code refactoring for hibmc_drv_vdac.c, no actual function changes. > > v2: > remove the debug message. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > Reviewed-by: Joe Perches <joe@perches.com> > --- > drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 50 +++++++----------------- > 1 file changed, 14 insertions(+), 36 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..077b7996 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,18 +83,9 @@ 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"); > + if (!encoder) > return -ENOMEM; > - } > > encoder->possible_crtcs = 0x1; > ret = drm_encoder_init(dev, encoder, &hibmc_encoder_funcs, > @@ -131,6 +96,19 @@ 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); Overall, the patch seems correct, expect for this line. DRM data structures should now be allocated with drmm_kzalloc() in drm_managed.h. It was added recently. With devm_kzalloc(), the memory will be released when the HW device goes away. If userspace still holds a reference, the driver can segfault. Using drmm_kzalloc() delays the release until the memory is really unused. However, hibmc's modesetting pipeline appears to be always the same. I think you should consider embedding connector and encoder (and crtc) in struct hibmc_drm_private. Would make the code simpler and more robust. Best regards Thomas > + if (!connector) > + 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_attach_encoder(connector, encoder); > > return 0; >
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c index 678ac2e..077b7996 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,18 +83,9 @@ 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"); + if (!encoder) return -ENOMEM; - } encoder->possible_crtcs = 0x1; ret = drm_encoder_init(dev, encoder, &hibmc_encoder_funcs, @@ -131,6 +96,19 @@ 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) + 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_attach_encoder(connector, encoder); return 0;