| Message ID | 1345197059-25583-3-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/17/2012 06:50 PM, Inki Dae wrote: > this patch separates sub driver's probe call and encoder/connector creation > so that exynos drm core module can take exception when some operation was > failed properly. Which exceptions? I don't know this patch gives any benefit to us. > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_core.c | 93 +++++++++++++++++++++--------- > 1 files changed, 65 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c > index 84dd099..1c8d5fe 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_core.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c > @@ -34,30 +34,15 @@ > > static LIST_HEAD(exynos_drm_subdrv_list); > > -static int exynos_drm_subdrv_probe(struct drm_device *dev, > +static int exynos_drm_create_enc_conn(struct drm_device *dev, > struct exynos_drm_subdrv *subdrv) > { > struct drm_encoder *encoder; > struct drm_connector *connector; > + int ret; > > DRM_DEBUG_DRIVER("%s\n", __FILE__); > > - if (subdrv->probe) { > - int ret; > - > - /* > - * this probe callback would be called by sub driver > - * after setting of all resources to this sub driver, > - * such as clock, irq and register map are done or by load() > - * of exynos drm driver. > - * > - * P.S. note that this driver is considered for modularization. > - */ > - ret = subdrv->probe(dev, subdrv->dev); > - if (ret) > - return ret; > - } > - > if (!subdrv->manager) > return 0; > > @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct drm_device *dev, > connector = exynos_drm_connector_create(dev, encoder); > if (!connector) { > DRM_ERROR("failed to create connector\n"); > - encoder->funcs->destroy(encoder); > - return -EFAULT; > + ret = -EFAULT; > + goto err_destroy_encoder; > } > > subdrv->encoder = encoder; > subdrv->connector = connector; > > return 0; > + > +err_destroy_encoder: > + encoder->funcs->destroy(encoder); > + return ret; > } > > -static void exynos_drm_subdrv_remove(struct drm_device *dev, > - struct exynos_drm_subdrv *subdrv) > +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv *subdrv) > { > - DRM_DEBUG_DRIVER("%s\n", __FILE__); > - > - if (subdrv->remove) > - subdrv->remove(dev); > - > if (subdrv->encoder) { > struct drm_encoder *encoder = subdrv->encoder; > encoder->funcs->destroy(encoder); > @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct drm_device *dev, > } > } > > +static int exynos_drm_subdrv_probe(struct drm_device *dev, > + struct exynos_drm_subdrv *subdrv) > +{ > + if (subdrv->probe) { > + int ret; > + > + subdrv->drm_dev = dev; > + > + /* > + * this probe callback would be called by sub driver > + * after setting of all resources to this sub driver, > + * such as clock, irq and register map are done or by load() > + * of exynos drm driver. > + * > + * P.S. note that this driver is considered for modularization. > + */ > + ret = subdrv->probe(dev, subdrv->dev); > + if (ret) > + return ret; > + } > + > + if (!subdrv->manager) > + return -EINVAL; > + > + subdrv->manager->dev = subdrv->dev; > + > + return 0; > +} > + > +static void exynos_drm_subdrv_remove(struct drm_device *dev, > + struct exynos_drm_subdrv *subdrv) > +{ > + DRM_DEBUG_DRIVER("%s\n", __FILE__); > + > + if (subdrv->remove) > + subdrv->remove(dev, subdrv->dev); > +} > + > int exynos_drm_device_register(struct drm_device *dev) > { > struct exynos_drm_subdrv *subdrv, *n; > + unsigned int fine_cnt = 0; > int err; > > DRM_DEBUG_DRIVER("%s\n", __FILE__); > @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device *dev) > return -EINVAL; > > list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, list) { > - subdrv->drm_dev = dev; > err = exynos_drm_subdrv_probe(dev, subdrv); > if (err) { > DRM_DEBUG("exynos drm subdrv probe failed.\n"); > list_del(&subdrv->list); > + continue; > } > + > + err = exynos_drm_create_enc_conn(dev, subdrv); > + if (err) { > + DRM_DEBUG("failed to create encoder and connector.\n"); > + exynos_drm_subdrv_remove(dev, subdrv); > + list_del(&subdrv->list); > + continue; > + } > + > + fine_cnt++; > } > > + if (!fine_cnt) > + return -EINVAL; > + > return 0; > } > EXPORT_SYMBOL_GPL(exynos_drm_device_register); > @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device *dev) > return -EINVAL; > } > > - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) > + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { > exynos_drm_subdrv_remove(dev, subdrv); > + exynos_drm_destroy_enc_conn(subdrv); > + } > > return 0; > }
2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: > On 08/17/2012 06:50 PM, Inki Dae wrote: >> >> this patch separates sub driver's probe call and encoder/connector >> creation >> so that exynos drm core module can take exception when some operation was >> failed properly. > > > Which exceptions? I don't know this patch gives any benefit to us. > previous code didn't take exception when exynos_drm_encoder_create() is falied. and for now, exynos_drm_encoder/connector_create functions was called at exynos_drm_subdrv_probe() so know that this patch is for code clean by separating them into two parts also. > >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_core.c | 93 >> +++++++++++++++++++++--------- >> 1 files changed, 65 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c >> b/drivers/gpu/drm/exynos/exynos_drm_core.c >> index 84dd099..1c8d5fe 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c >> @@ -34,30 +34,15 @@ >> static LIST_HEAD(exynos_drm_subdrv_list); >> -static int exynos_drm_subdrv_probe(struct drm_device *dev, >> +static int exynos_drm_create_enc_conn(struct drm_device *dev, >> struct exynos_drm_subdrv *subdrv) >> { >> struct drm_encoder *encoder; >> struct drm_connector *connector; >> + int ret; >> DRM_DEBUG_DRIVER("%s\n", __FILE__); >> - if (subdrv->probe) { >> - int ret; >> - >> - /* >> - * this probe callback would be called by sub driver >> - * after setting of all resources to this sub driver, >> - * such as clock, irq and register map are done or by >> load() >> - * of exynos drm driver. >> - * >> - * P.S. note that this driver is considered for >> modularization. >> - */ >> - ret = subdrv->probe(dev, subdrv->dev); >> - if (ret) >> - return ret; >> - } >> - >> if (!subdrv->manager) >> return 0; >> @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct drm_device >> *dev, >> connector = exynos_drm_connector_create(dev, encoder); >> if (!connector) { >> DRM_ERROR("failed to create connector\n"); >> - encoder->funcs->destroy(encoder); >> - return -EFAULT; >> + ret = -EFAULT; >> + goto err_destroy_encoder; >> } >> subdrv->encoder = encoder; >> subdrv->connector = connector; >> return 0; >> + >> +err_destroy_encoder: >> + encoder->funcs->destroy(encoder); >> + return ret; >> } >> -static void exynos_drm_subdrv_remove(struct drm_device *dev, >> - struct exynos_drm_subdrv *subdrv) >> +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv *subdrv) >> { >> - DRM_DEBUG_DRIVER("%s\n", __FILE__); >> - >> - if (subdrv->remove) >> - subdrv->remove(dev); >> - >> if (subdrv->encoder) { >> struct drm_encoder *encoder = subdrv->encoder; >> encoder->funcs->destroy(encoder); >> @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct drm_device >> *dev, >> } >> } >> +static int exynos_drm_subdrv_probe(struct drm_device *dev, >> + struct exynos_drm_subdrv *subdrv) >> +{ >> + if (subdrv->probe) { >> + int ret; >> + >> + subdrv->drm_dev = dev; >> + >> + /* >> + * this probe callback would be called by sub driver >> + * after setting of all resources to this sub driver, >> + * such as clock, irq and register map are done or by >> load() >> + * of exynos drm driver. >> + * >> + * P.S. note that this driver is considered for >> modularization. >> + */ >> + ret = subdrv->probe(dev, subdrv->dev); >> + if (ret) >> + return ret; >> + } >> + >> + if (!subdrv->manager) >> + return -EINVAL; >> + >> + subdrv->manager->dev = subdrv->dev; >> + >> + return 0; >> +} >> + >> +static void exynos_drm_subdrv_remove(struct drm_device *dev, >> + struct exynos_drm_subdrv *subdrv) >> +{ >> + DRM_DEBUG_DRIVER("%s\n", __FILE__); >> + >> + if (subdrv->remove) >> + subdrv->remove(dev, subdrv->dev); >> +} >> + >> int exynos_drm_device_register(struct drm_device *dev) >> { >> struct exynos_drm_subdrv *subdrv, *n; >> + unsigned int fine_cnt = 0; >> int err; >> DRM_DEBUG_DRIVER("%s\n", __FILE__); >> @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device >> *dev) >> return -EINVAL; >> list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, list) >> { >> - subdrv->drm_dev = dev; >> err = exynos_drm_subdrv_probe(dev, subdrv); >> if (err) { >> DRM_DEBUG("exynos drm subdrv probe failed.\n"); >> list_del(&subdrv->list); >> + continue; >> } >> + >> + err = exynos_drm_create_enc_conn(dev, subdrv); >> + if (err) { >> + DRM_DEBUG("failed to create encoder and >> connector.\n"); >> + exynos_drm_subdrv_remove(dev, subdrv); >> + list_del(&subdrv->list); >> + continue; >> + } >> + >> + fine_cnt++; >> } >> + if (!fine_cnt) >> + return -EINVAL; >> + >> return 0; >> } >> EXPORT_SYMBOL_GPL(exynos_drm_device_register); >> @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device >> *dev) >> return -EINVAL; >> } >> - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) >> + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { >> exynos_drm_subdrv_remove(dev, subdrv); >> + exynos_drm_destroy_enc_conn(subdrv); >> + } >> return 0; >> } > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 08/20/2012 10:52 AM, InKi Dae wrote: > 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: >> On 08/17/2012 06:50 PM, Inki Dae wrote: >>> this patch separates sub driver's probe call and encoder/connector >>> creation >>> so that exynos drm core module can take exception when some operation was >>> failed properly. >> >> Which exceptions? I don't know this patch gives any benefit to us. >> > previous code didn't take exception when exynos_drm_encoder_create() > is falied. No, it is considered. > and for now, exynos_drm_encoder/connector_create functions > was called at exynos_drm_subdrv_probe() so know that this patch is for > code clean by separating them into two parts also. It's ok, but it just splitting. > >>> Signed-off-by: Inki Dae <inki.dae@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_core.c | 93 >>> +++++++++++++++++++++--------- >>> 1 files changed, 65 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c >>> b/drivers/gpu/drm/exynos/exynos_drm_core.c >>> index 84dd099..1c8d5fe 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c >>> @@ -34,30 +34,15 @@ >>> static LIST_HEAD(exynos_drm_subdrv_list); >>> -static int exynos_drm_subdrv_probe(struct drm_device *dev, >>> +static int exynos_drm_create_enc_conn(struct drm_device *dev, >>> struct exynos_drm_subdrv *subdrv) >>> { >>> struct drm_encoder *encoder; >>> struct drm_connector *connector; >>> + int ret; >>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>> - if (subdrv->probe) { >>> - int ret; >>> - >>> - /* >>> - * this probe callback would be called by sub driver >>> - * after setting of all resources to this sub driver, >>> - * such as clock, irq and register map are done or by >>> load() >>> - * of exynos drm driver. >>> - * >>> - * P.S. note that this driver is considered for >>> modularization. >>> - */ >>> - ret = subdrv->probe(dev, subdrv->dev); >>> - if (ret) >>> - return ret; >>> - } >>> - >>> if (!subdrv->manager) >>> return 0; >>> @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct drm_device >>> *dev, >>> connector = exynos_drm_connector_create(dev, encoder); >>> if (!connector) { >>> DRM_ERROR("failed to create connector\n"); >>> - encoder->funcs->destroy(encoder); >>> - return -EFAULT; >>> + ret = -EFAULT; >>> + goto err_destroy_encoder; >>> } >>> subdrv->encoder = encoder; >>> subdrv->connector = connector; >>> return 0; >>> + >>> +err_destroy_encoder: >>> + encoder->funcs->destroy(encoder); >>> + return ret; >>> } >>> -static void exynos_drm_subdrv_remove(struct drm_device *dev, >>> - struct exynos_drm_subdrv *subdrv) >>> +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv *subdrv) >>> { >>> - DRM_DEBUG_DRIVER("%s\n", __FILE__); >>> - >>> - if (subdrv->remove) >>> - subdrv->remove(dev); >>> - >>> if (subdrv->encoder) { >>> struct drm_encoder *encoder = subdrv->encoder; >>> encoder->funcs->destroy(encoder); >>> @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct drm_device >>> *dev, >>> } >>> } >>> +static int exynos_drm_subdrv_probe(struct drm_device *dev, >>> + struct exynos_drm_subdrv *subdrv) >>> +{ >>> + if (subdrv->probe) { >>> + int ret; >>> + >>> + subdrv->drm_dev = dev; >>> + >>> + /* >>> + * this probe callback would be called by sub driver >>> + * after setting of all resources to this sub driver, >>> + * such as clock, irq and register map are done or by >>> load() >>> + * of exynos drm driver. >>> + * >>> + * P.S. note that this driver is considered for >>> modularization. >>> + */ >>> + ret = subdrv->probe(dev, subdrv->dev); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (!subdrv->manager) >>> + return -EINVAL; >>> + >>> + subdrv->manager->dev = subdrv->dev; >>> + >>> + return 0; >>> +} >>> + >>> +static void exynos_drm_subdrv_remove(struct drm_device *dev, >>> + struct exynos_drm_subdrv *subdrv) >>> +{ >>> + DRM_DEBUG_DRIVER("%s\n", __FILE__); >>> + >>> + if (subdrv->remove) >>> + subdrv->remove(dev, subdrv->dev); >>> +} >>> + >>> int exynos_drm_device_register(struct drm_device *dev) >>> { >>> struct exynos_drm_subdrv *subdrv, *n; >>> + unsigned int fine_cnt = 0; >>> int err; >>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>> @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device >>> *dev) >>> return -EINVAL; >>> list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, list) >>> { >>> - subdrv->drm_dev = dev; >>> err = exynos_drm_subdrv_probe(dev, subdrv); >>> if (err) { >>> DRM_DEBUG("exynos drm subdrv probe failed.\n"); >>> list_del(&subdrv->list); >>> + continue; >>> } >>> + >>> + err = exynos_drm_create_enc_conn(dev, subdrv); >>> + if (err) { >>> + DRM_DEBUG("failed to create encoder and >>> connector.\n"); >>> + exynos_drm_subdrv_remove(dev, subdrv); >>> + list_del(&subdrv->list); >>> + continue; >>> + } >>> + >>> + fine_cnt++; >>> } >>> + if (!fine_cnt) >>> + return -EINVAL; >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(exynos_drm_device_register); >>> @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device >>> *dev) >>> return -EINVAL; >>> } >>> - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) >>> + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { >>> exynos_drm_subdrv_remove(dev, subdrv); >>> + exynos_drm_destroy_enc_conn(subdrv); >>> + } >>> return 0; >>> } >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: > On 08/20/2012 10:52 AM, InKi Dae wrote: >> >> 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: >>> >>> On 08/17/2012 06:50 PM, Inki Dae wrote: >>>> >>>> this patch separates sub driver's probe call and encoder/connector >>>> creation >>>> so that exynos drm core module can take exception when some operation >>>> was >>>> failed properly. >>> >>> >>> Which exceptions? I don't know this patch gives any benefit to us. >>> >> previous code didn't take exception when exynos_drm_encoder_create() >> is falied. > > > No, it is considered. > where is subdrv->remove() called when exynos_drm_encoder_create() is failed? I think if failed then subdrv->remove() should be called and if exynos_drm_connector_create() is failed then not only encoder->funcs->destory() but also subdrv->remove() > >> and for now, exynos_drm_encoder/connector_create functions >> was called at exynos_drm_subdrv_probe() so know that this patch is for >> code clean by separating them into two parts also. > > > It's ok, but it just splitting. > > >> >>>> Signed-off-by: Inki Dae <inki.dae@samsung.com> >>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_drm_core.c | 93 >>>> +++++++++++++++++++++--------- >>>> 1 files changed, 65 insertions(+), 28 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>> b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>> index 84dd099..1c8d5fe 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>> @@ -34,30 +34,15 @@ >>>> static LIST_HEAD(exynos_drm_subdrv_list); >>>> -static int exynos_drm_subdrv_probe(struct drm_device *dev, >>>> +static int exynos_drm_create_enc_conn(struct drm_device *dev, >>>> struct exynos_drm_subdrv >>>> *subdrv) >>>> { >>>> struct drm_encoder *encoder; >>>> struct drm_connector *connector; >>>> + int ret; >>>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>> - if (subdrv->probe) { >>>> - int ret; >>>> - >>>> - /* >>>> - * this probe callback would be called by sub driver >>>> - * after setting of all resources to this sub driver, >>>> - * such as clock, irq and register map are done or by >>>> load() >>>> - * of exynos drm driver. >>>> - * >>>> - * P.S. note that this driver is considered for >>>> modularization. >>>> - */ >>>> - ret = subdrv->probe(dev, subdrv->dev); >>>> - if (ret) >>>> - return ret; >>>> - } >>>> - >>>> if (!subdrv->manager) >>>> return 0; >>>> @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct >>>> drm_device >>>> *dev, >>>> connector = exynos_drm_connector_create(dev, encoder); >>>> if (!connector) { >>>> DRM_ERROR("failed to create connector\n"); >>>> - encoder->funcs->destroy(encoder); >>>> - return -EFAULT; >>>> + ret = -EFAULT; >>>> + goto err_destroy_encoder; >>>> } >>>> subdrv->encoder = encoder; >>>> subdrv->connector = connector; >>>> return 0; >>>> + >>>> +err_destroy_encoder: >>>> + encoder->funcs->destroy(encoder); >>>> + return ret; >>>> } >>>> -static void exynos_drm_subdrv_remove(struct drm_device *dev, >>>> - struct exynos_drm_subdrv *subdrv) >>>> +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv >>>> *subdrv) >>>> { >>>> - DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>> - >>>> - if (subdrv->remove) >>>> - subdrv->remove(dev); >>>> - >>>> if (subdrv->encoder) { >>>> struct drm_encoder *encoder = subdrv->encoder; >>>> encoder->funcs->destroy(encoder); >>>> @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct >>>> drm_device >>>> *dev, >>>> } >>>> } >>>> +static int exynos_drm_subdrv_probe(struct drm_device *dev, >>>> + struct exynos_drm_subdrv >>>> *subdrv) >>>> +{ >>>> + if (subdrv->probe) { >>>> + int ret; >>>> + >>>> + subdrv->drm_dev = dev; >>>> + >>>> + /* >>>> + * this probe callback would be called by sub driver >>>> + * after setting of all resources to this sub driver, >>>> + * such as clock, irq and register map are done or by >>>> load() >>>> + * of exynos drm driver. >>>> + * >>>> + * P.S. note that this driver is considered for >>>> modularization. >>>> + */ >>>> + ret = subdrv->probe(dev, subdrv->dev); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + if (!subdrv->manager) >>>> + return -EINVAL; >>>> + >>>> + subdrv->manager->dev = subdrv->dev; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void exynos_drm_subdrv_remove(struct drm_device *dev, >>>> + struct exynos_drm_subdrv *subdrv) >>>> +{ >>>> + DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>> + >>>> + if (subdrv->remove) >>>> + subdrv->remove(dev, subdrv->dev); >>>> +} >>>> + >>>> int exynos_drm_device_register(struct drm_device *dev) >>>> { >>>> struct exynos_drm_subdrv *subdrv, *n; >>>> + unsigned int fine_cnt = 0; >>>> int err; >>>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>> @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device >>>> *dev) >>>> return -EINVAL; >>>> list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, >>>> list) >>>> { >>>> - subdrv->drm_dev = dev; >>>> err = exynos_drm_subdrv_probe(dev, subdrv); >>>> if (err) { >>>> DRM_DEBUG("exynos drm subdrv probe failed.\n"); >>>> list_del(&subdrv->list); >>>> + continue; >>>> } >>>> + >>>> + err = exynos_drm_create_enc_conn(dev, subdrv); >>>> + if (err) { >>>> + DRM_DEBUG("failed to create encoder and >>>> connector.\n"); >>>> + exynos_drm_subdrv_remove(dev, subdrv); >>>> + list_del(&subdrv->list); >>>> + continue; >>>> + } >>>> + >>>> + fine_cnt++; >>>> } >>>> + if (!fine_cnt) >>>> + return -EINVAL; >>>> + >>>> return 0; >>>> } >>>> EXPORT_SYMBOL_GPL(exynos_drm_device_register); >>>> @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device >>>> *dev) >>>> return -EINVAL; >>>> } >>>> - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) >>>> + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { >>>> exynos_drm_subdrv_remove(dev, subdrv); >>>> + exynos_drm_destroy_enc_conn(subdrv); >>>> + } >>>> return 0; >>>> } >>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 08/20/2012 01:31 PM, InKi Dae wrote: > 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: >> On 08/20/2012 10:52 AM, InKi Dae wrote: >>> 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: >>>> On 08/17/2012 06:50 PM, Inki Dae wrote: >>>>> this patch separates sub driver's probe call and encoder/connector >>>>> creation >>>>> so that exynos drm core module can take exception when some operation >>>>> was >>>>> failed properly. >>>> >>>> Which exceptions? I don't know this patch gives any benefit to us. >>>> >>> previous code didn't take exception when exynos_drm_encoder_create() >>> is falied. >> >> No, it is considered. >> > where is subdrv->remove() called when exynos_drm_encoder_create() is > failed? I think if failed then subdrv->remove() should be called and > if exynos_drm_connector_create() is failed then not only > encoder->funcs->destory() but also subdrv->remove() OK, but just add subdrv->remove(). Then if it needs, please split function. >>> and for now, exynos_drm_encoder/connector_create functions >>> was called at exynos_drm_subdrv_probe() so know that this patch is for >>> code clean by separating them into two parts also. >> >> It's ok, but it just splitting. >> >> >>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com> >>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>>>> --- >>>>> drivers/gpu/drm/exynos/exynos_drm_core.c | 93 >>>>> +++++++++++++++++++++--------- >>>>> 1 files changed, 65 insertions(+), 28 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> index 84dd099..1c8d5fe 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> @@ -34,30 +34,15 @@ >>>>> static LIST_HEAD(exynos_drm_subdrv_list); >>>>> -static int exynos_drm_subdrv_probe(struct drm_device *dev, >>>>> +static int exynos_drm_create_enc_conn(struct drm_device *dev, >>>>> struct exynos_drm_subdrv >>>>> *subdrv) >>>>> { >>>>> struct drm_encoder *encoder; >>>>> struct drm_connector *connector; >>>>> + int ret; >>>>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>> - if (subdrv->probe) { >>>>> - int ret; >>>>> - >>>>> - /* >>>>> - * this probe callback would be called by sub driver >>>>> - * after setting of all resources to this sub driver, >>>>> - * such as clock, irq and register map are done or by >>>>> load() >>>>> - * of exynos drm driver. >>>>> - * >>>>> - * P.S. note that this driver is considered for >>>>> modularization. >>>>> - */ >>>>> - ret = subdrv->probe(dev, subdrv->dev); >>>>> - if (ret) >>>>> - return ret; >>>>> - } >>>>> - >>>>> if (!subdrv->manager) >>>>> return 0; >>>>> @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct >>>>> drm_device >>>>> *dev, >>>>> connector = exynos_drm_connector_create(dev, encoder); >>>>> if (!connector) { >>>>> DRM_ERROR("failed to create connector\n"); >>>>> - encoder->funcs->destroy(encoder); >>>>> - return -EFAULT; >>>>> + ret = -EFAULT; >>>>> + goto err_destroy_encoder; >>>>> } >>>>> subdrv->encoder = encoder; >>>>> subdrv->connector = connector; >>>>> return 0; >>>>> + >>>>> +err_destroy_encoder: >>>>> + encoder->funcs->destroy(encoder); >>>>> + return ret; >>>>> } >>>>> -static void exynos_drm_subdrv_remove(struct drm_device *dev, >>>>> - struct exynos_drm_subdrv *subdrv) >>>>> +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv >>>>> *subdrv) >>>>> { >>>>> - DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>> - >>>>> - if (subdrv->remove) >>>>> - subdrv->remove(dev); >>>>> - >>>>> if (subdrv->encoder) { >>>>> struct drm_encoder *encoder = subdrv->encoder; >>>>> encoder->funcs->destroy(encoder); >>>>> @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct >>>>> drm_device >>>>> *dev, >>>>> } >>>>> } >>>>> +static int exynos_drm_subdrv_probe(struct drm_device *dev, >>>>> + struct exynos_drm_subdrv >>>>> *subdrv) >>>>> +{ >>>>> + if (subdrv->probe) { >>>>> + int ret; >>>>> + >>>>> + subdrv->drm_dev = dev; >>>>> + >>>>> + /* >>>>> + * this probe callback would be called by sub driver >>>>> + * after setting of all resources to this sub driver, >>>>> + * such as clock, irq and register map are done or by >>>>> load() >>>>> + * of exynos drm driver. >>>>> + * >>>>> + * P.S. note that this driver is considered for >>>>> modularization. >>>>> + */ >>>>> + ret = subdrv->probe(dev, subdrv->dev); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + >>>>> + if (!subdrv->manager) >>>>> + return -EINVAL; >>>>> + >>>>> + subdrv->manager->dev = subdrv->dev; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static void exynos_drm_subdrv_remove(struct drm_device *dev, >>>>> + struct exynos_drm_subdrv *subdrv) >>>>> +{ >>>>> + DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>> + >>>>> + if (subdrv->remove) >>>>> + subdrv->remove(dev, subdrv->dev); >>>>> +} >>>>> + >>>>> int exynos_drm_device_register(struct drm_device *dev) >>>>> { >>>>> struct exynos_drm_subdrv *subdrv, *n; >>>>> + unsigned int fine_cnt = 0; >>>>> int err; >>>>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>> @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device >>>>> *dev) >>>>> return -EINVAL; >>>>> list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, >>>>> list) >>>>> { >>>>> - subdrv->drm_dev = dev; >>>>> err = exynos_drm_subdrv_probe(dev, subdrv); >>>>> if (err) { >>>>> DRM_DEBUG("exynos drm subdrv probe failed.\n"); >>>>> list_del(&subdrv->list); >>>>> + continue; >>>>> } >>>>> + >>>>> + err = exynos_drm_create_enc_conn(dev, subdrv); >>>>> + if (err) { >>>>> + DRM_DEBUG("failed to create encoder and >>>>> connector.\n"); >>>>> + exynos_drm_subdrv_remove(dev, subdrv); >>>>> + list_del(&subdrv->list); >>>>> + continue; >>>>> + } >>>>> + >>>>> + fine_cnt++; >>>>> } >>>>> + if (!fine_cnt) >>>>> + return -EINVAL; >>>>> + >>>>> return 0; >>>>> } >>>>> EXPORT_SYMBOL_GPL(exynos_drm_device_register); >>>>> @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device >>>>> *dev) >>>>> return -EINVAL; >>>>> } >>>>> - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) >>>>> + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { >>>>> exynos_drm_subdrv_remove(dev, subdrv); >>>>> + exynos_drm_destroy_enc_conn(subdrv); >>>>> + } >>>>> return 0; >>>>> } >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: > On 08/20/2012 01:31 PM, InKi Dae wrote: >> >> 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: >>> >>> On 08/20/2012 10:52 AM, InKi Dae wrote: >>>> >>>> 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>: >>>>> >>>>> On 08/17/2012 06:50 PM, Inki Dae wrote: >>>>>> >>>>>> this patch separates sub driver's probe call and encoder/connector >>>>>> creation >>>>>> so that exynos drm core module can take exception when some operation >>>>>> was >>>>>> failed properly. >>>>> >>>>> >>>>> Which exceptions? I don't know this patch gives any benefit to us. >>>>> >>>> previous code didn't take exception when exynos_drm_encoder_create() >>>> is falied. >>> >>> >>> No, it is considered. >>> >> where is subdrv->remove() called when exynos_drm_encoder_create() is >> failed? I think if failed then subdrv->remove() should be called and >> if exynos_drm_connector_create() is failed then not only >> encoder->funcs->destory() but also subdrv->remove() > > > OK, but just add subdrv->remove(). Then if it needs, please split function. > now exynos_drm_subdrv_probe() creates encoder and connector so it should be separated into meaningful parts. > >>>> and for now, exynos_drm_encoder/connector_create functions >>>> was called at exynos_drm_subdrv_probe() so know that this patch is for >>>> code clean by separating them into two parts also. >>> >>> >>> It's ok, but it just splitting. >>> >>> >>>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com> >>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>>>>> --- >>>>>> drivers/gpu/drm/exynos/exynos_drm_core.c | 93 >>>>>> +++++++++++++++++++++--------- >>>>>> 1 files changed, 65 insertions(+), 28 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>>> b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>>> index 84dd099..1c8d5fe 100644 >>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>>> @@ -34,30 +34,15 @@ >>>>>> static LIST_HEAD(exynos_drm_subdrv_list); >>>>>> -static int exynos_drm_subdrv_probe(struct drm_device *dev, >>>>>> +static int exynos_drm_create_enc_conn(struct drm_device *dev, >>>>>> struct exynos_drm_subdrv >>>>>> *subdrv) >>>>>> { >>>>>> struct drm_encoder *encoder; >>>>>> struct drm_connector *connector; >>>>>> + int ret; >>>>>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>>> - if (subdrv->probe) { >>>>>> - int ret; >>>>>> - >>>>>> - /* >>>>>> - * this probe callback would be called by sub driver >>>>>> - * after setting of all resources to this sub driver, >>>>>> - * such as clock, irq and register map are done or by >>>>>> load() >>>>>> - * of exynos drm driver. >>>>>> - * >>>>>> - * P.S. note that this driver is considered for >>>>>> modularization. >>>>>> - */ >>>>>> - ret = subdrv->probe(dev, subdrv->dev); >>>>>> - if (ret) >>>>>> - return ret; >>>>>> - } >>>>>> - >>>>>> if (!subdrv->manager) >>>>>> return 0; >>>>>> @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct >>>>>> drm_device >>>>>> *dev, >>>>>> connector = exynos_drm_connector_create(dev, encoder); >>>>>> if (!connector) { >>>>>> DRM_ERROR("failed to create connector\n"); >>>>>> - encoder->funcs->destroy(encoder); >>>>>> - return -EFAULT; >>>>>> + ret = -EFAULT; >>>>>> + goto err_destroy_encoder; >>>>>> } >>>>>> subdrv->encoder = encoder; >>>>>> subdrv->connector = connector; >>>>>> return 0; >>>>>> + >>>>>> +err_destroy_encoder: >>>>>> + encoder->funcs->destroy(encoder); >>>>>> + return ret; >>>>>> } >>>>>> -static void exynos_drm_subdrv_remove(struct drm_device *dev, >>>>>> - struct exynos_drm_subdrv >>>>>> *subdrv) >>>>>> +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv >>>>>> *subdrv) >>>>>> { >>>>>> - DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>>> - >>>>>> - if (subdrv->remove) >>>>>> - subdrv->remove(dev); >>>>>> - >>>>>> if (subdrv->encoder) { >>>>>> struct drm_encoder *encoder = subdrv->encoder; >>>>>> encoder->funcs->destroy(encoder); >>>>>> @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct >>>>>> drm_device >>>>>> *dev, >>>>>> } >>>>>> } >>>>>> +static int exynos_drm_subdrv_probe(struct drm_device *dev, >>>>>> + struct exynos_drm_subdrv >>>>>> *subdrv) >>>>>> +{ >>>>>> + if (subdrv->probe) { >>>>>> + int ret; >>>>>> + >>>>>> + subdrv->drm_dev = dev; >>>>>> + >>>>>> + /* >>>>>> + * this probe callback would be called by sub driver >>>>>> + * after setting of all resources to this sub driver, >>>>>> + * such as clock, irq and register map are done or by >>>>>> load() >>>>>> + * of exynos drm driver. >>>>>> + * >>>>>> + * P.S. note that this driver is considered for >>>>>> modularization. >>>>>> + */ >>>>>> + ret = subdrv->probe(dev, subdrv->dev); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + if (!subdrv->manager) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + subdrv->manager->dev = subdrv->dev; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static void exynos_drm_subdrv_remove(struct drm_device *dev, >>>>>> + struct exynos_drm_subdrv >>>>>> *subdrv) >>>>>> +{ >>>>>> + DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>>> + >>>>>> + if (subdrv->remove) >>>>>> + subdrv->remove(dev, subdrv->dev); >>>>>> +} >>>>>> + >>>>>> int exynos_drm_device_register(struct drm_device *dev) >>>>>> { >>>>>> struct exynos_drm_subdrv *subdrv, *n; >>>>>> + unsigned int fine_cnt = 0; >>>>>> int err; >>>>>> DRM_DEBUG_DRIVER("%s\n", __FILE__); >>>>>> @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device >>>>>> *dev) >>>>>> return -EINVAL; >>>>>> list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, >>>>>> list) >>>>>> { >>>>>> - subdrv->drm_dev = dev; >>>>>> err = exynos_drm_subdrv_probe(dev, subdrv); >>>>>> if (err) { >>>>>> DRM_DEBUG("exynos drm subdrv probe >>>>>> failed.\n"); >>>>>> list_del(&subdrv->list); >>>>>> + continue; >>>>>> } >>>>>> + >>>>>> + err = exynos_drm_create_enc_conn(dev, subdrv); >>>>>> + if (err) { >>>>>> + DRM_DEBUG("failed to create encoder and >>>>>> connector.\n"); >>>>>> + exynos_drm_subdrv_remove(dev, subdrv); >>>>>> + list_del(&subdrv->list); >>>>>> + continue; >>>>>> + } >>>>>> + >>>>>> + fine_cnt++; >>>>>> } >>>>>> + if (!fine_cnt) >>>>>> + return -EINVAL; >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(exynos_drm_device_register); >>>>>> @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct >>>>>> drm_device >>>>>> *dev) >>>>>> return -EINVAL; >>>>>> } >>>>>> - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) >>>>>> + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { >>>>>> exynos_drm_subdrv_remove(dev, subdrv); >>>>>> + exynos_drm_destroy_enc_conn(subdrv); >>>>>> + } >>>>>> return 0; >>>>>> } >>>>> >>>>> >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c index 84dd099..1c8d5fe 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_core.c +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c @@ -34,30 +34,15 @@ static LIST_HEAD(exynos_drm_subdrv_list); -static int exynos_drm_subdrv_probe(struct drm_device *dev, +static int exynos_drm_create_enc_conn(struct drm_device *dev, struct exynos_drm_subdrv *subdrv) { struct drm_encoder *encoder; struct drm_connector *connector; + int ret; DRM_DEBUG_DRIVER("%s\n", __FILE__); - if (subdrv->probe) { - int ret; - - /* - * this probe callback would be called by sub driver - * after setting of all resources to this sub driver, - * such as clock, irq and register map are done or by load() - * of exynos drm driver. - * - * P.S. note that this driver is considered for modularization. - */ - ret = subdrv->probe(dev, subdrv->dev); - if (ret) - return ret; - } - if (!subdrv->manager) return 0; @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct drm_device *dev, connector = exynos_drm_connector_create(dev, encoder); if (!connector) { DRM_ERROR("failed to create connector\n"); - encoder->funcs->destroy(encoder); - return -EFAULT; + ret = -EFAULT; + goto err_destroy_encoder; } subdrv->encoder = encoder; subdrv->connector = connector; return 0; + +err_destroy_encoder: + encoder->funcs->destroy(encoder); + return ret; } -static void exynos_drm_subdrv_remove(struct drm_device *dev, - struct exynos_drm_subdrv *subdrv) +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv *subdrv) { - DRM_DEBUG_DRIVER("%s\n", __FILE__); - - if (subdrv->remove) - subdrv->remove(dev); - if (subdrv->encoder) { struct drm_encoder *encoder = subdrv->encoder; encoder->funcs->destroy(encoder); @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct drm_device *dev, } } +static int exynos_drm_subdrv_probe(struct drm_device *dev, + struct exynos_drm_subdrv *subdrv) +{ + if (subdrv->probe) { + int ret; + + subdrv->drm_dev = dev; + + /* + * this probe callback would be called by sub driver + * after setting of all resources to this sub driver, + * such as clock, irq and register map are done or by load() + * of exynos drm driver. + * + * P.S. note that this driver is considered for modularization. + */ + ret = subdrv->probe(dev, subdrv->dev); + if (ret) + return ret; + } + + if (!subdrv->manager) + return -EINVAL; + + subdrv->manager->dev = subdrv->dev; + + return 0; +} + +static void exynos_drm_subdrv_remove(struct drm_device *dev, + struct exynos_drm_subdrv *subdrv) +{ + DRM_DEBUG_DRIVER("%s\n", __FILE__); + + if (subdrv->remove) + subdrv->remove(dev, subdrv->dev); +} + int exynos_drm_device_register(struct drm_device *dev) { struct exynos_drm_subdrv *subdrv, *n; + unsigned int fine_cnt = 0; int err; DRM_DEBUG_DRIVER("%s\n", __FILE__); @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device *dev) return -EINVAL; list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, list) { - subdrv->drm_dev = dev; err = exynos_drm_subdrv_probe(dev, subdrv); if (err) { DRM_DEBUG("exynos drm subdrv probe failed.\n"); list_del(&subdrv->list); + continue; } + + err = exynos_drm_create_enc_conn(dev, subdrv); + if (err) { + DRM_DEBUG("failed to create encoder and connector.\n"); + exynos_drm_subdrv_remove(dev, subdrv); + list_del(&subdrv->list); + continue; + } + + fine_cnt++; } + if (!fine_cnt) + return -EINVAL; + return 0; } EXPORT_SYMBOL_GPL(exynos_drm_device_register); @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device *dev) return -EINVAL; } - list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) + list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) { exynos_drm_subdrv_remove(dev, subdrv); + exynos_drm_destroy_enc_conn(subdrv); + } return 0; }