| Message ID | 20230430235732.3341119-3-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: simplify DPU encoder init | expand |
On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: > There is no need to clean up debugfs manually, it will be done by the > DRM core on device deregistration. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- There are two reasons to have the debugfs removed in the early_unregister: 1) Today, registration happens in late_register(), hence to balance the the call in _dpu_encoder_init_debugfs(), this one is present. 2) In drm_modeset_register_all(), if drm_connector_register_all() fails, it calls drm_encoder_unregister_all() first which calls early_unregister(). So to balance these out, dont we need to keep it? > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 32785cb1b079..8c45c949ec39 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder) > return _dpu_encoder_init_debugfs(encoder); > } > > -static void dpu_encoder_early_unregister(struct drm_encoder *encoder) > -{ > - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder); > - > - debugfs_remove_recursive(dpu_enc->debugfs_root); > -} > - > static int dpu_encoder_virt_add_phys_encs( > struct msm_display_info *disp_info, > struct dpu_encoder_virt *dpu_enc, > @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { > static const struct drm_encoder_funcs dpu_encoder_funcs = { > .destroy = dpu_encoder_destroy, > .late_register = dpu_encoder_late_register, > - .early_unregister = dpu_encoder_early_unregister, > }; > > struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
On Tue, 2 May 2023 at 23:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: > > There is no need to clean up debugfs manually, it will be done by the > > DRM core on device deregistration. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > There are two reasons to have the debugfs removed in the early_unregister: > > 1) Today, registration happens in late_register(), hence to balance the > the call in _dpu_encoder_init_debugfs(), this one is present. > > 2) In drm_modeset_register_all(), if drm_connector_register_all() fails, > it calls drm_encoder_unregister_all() first which calls early_unregister(). > > So to balance these out, dont we need to keep it? Please correct me if I'm wrong, drm_debugfs_cleanup() should take care of that. > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 32785cb1b079..8c45c949ec39 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder) > > return _dpu_encoder_init_debugfs(encoder); > > } > > > > -static void dpu_encoder_early_unregister(struct drm_encoder *encoder) > > -{ > > - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder); > > - > > - debugfs_remove_recursive(dpu_enc->debugfs_root); > > -} > > - > > static int dpu_encoder_virt_add_phys_encs( > > struct msm_display_info *disp_info, > > struct dpu_encoder_virt *dpu_enc, > > @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { > > static const struct drm_encoder_funcs dpu_encoder_funcs = { > > .destroy = dpu_encoder_destroy, > > .late_register = dpu_encoder_late_register, > > - .early_unregister = dpu_encoder_early_unregister, > > }; > > > > struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
On 5/2/2023 1:54 PM, Dmitry Baryshkov wrote: > On Tue, 2 May 2023 at 23:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >>> There is no need to clean up debugfs manually, it will be done by the >>> DRM core on device deregistration. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >> >> There are two reasons to have the debugfs removed in the early_unregister: >> >> 1) Today, registration happens in late_register(), hence to balance the >> the call in _dpu_encoder_init_debugfs(), this one is present. >> >> 2) In drm_modeset_register_all(), if drm_connector_register_all() fails, >> it calls drm_encoder_unregister_all() first which calls early_unregister(). >> >> So to balance these out, dont we need to keep it? > > Please correct me if I'm wrong, drm_debugfs_cleanup() should take care of that. > Not from what I am seeing, drm_debugfs_cleanup() is getting called from the error handling path of drm_dev_register() and separately from drm_dev_unregister() but not from the error handling path of drm_modeset_register_all(). So that case will be unbalanced with this change. >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 -------- >>> 1 file changed, 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 32785cb1b079..8c45c949ec39 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder) >>> return _dpu_encoder_init_debugfs(encoder); >>> } >>> >>> -static void dpu_encoder_early_unregister(struct drm_encoder *encoder) >>> -{ >>> - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder); >>> - >>> - debugfs_remove_recursive(dpu_enc->debugfs_root); >>> -} >>> - >>> static int dpu_encoder_virt_add_phys_encs( >>> struct msm_display_info *disp_info, >>> struct dpu_encoder_virt *dpu_enc, >>> @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { >>> static const struct drm_encoder_funcs dpu_encoder_funcs = { >>> .destroy = dpu_encoder_destroy, >>> .late_register = dpu_encoder_late_register, >>> - .early_unregister = dpu_encoder_early_unregister, >>> }; >>> >>> struct drm_encoder *dpu_encoder_init(struct drm_device *dev, >
On 02/05/2023 23:59, Abhinav Kumar wrote: > > > On 5/2/2023 1:54 PM, Dmitry Baryshkov wrote: >> On Tue, 2 May 2023 at 23:45, Abhinav Kumar <quic_abhinavk@quicinc.com> >> wrote: >>> >>> >>> >>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >>>> There is no need to clean up debugfs manually, it will be done by the >>>> DRM core on device deregistration. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>> >>> There are two reasons to have the debugfs removed in the >>> early_unregister: >>> >>> 1) Today, registration happens in late_register(), hence to balance the >>> the call in _dpu_encoder_init_debugfs(), this one is present. >>> >>> 2) In drm_modeset_register_all(), if drm_connector_register_all() fails, >>> it calls drm_encoder_unregister_all() first which calls >>> early_unregister(). >>> >>> So to balance these out, dont we need to keep it? >> >> Please correct me if I'm wrong, drm_debugfs_cleanup() should take care >> of that. >> > > Not from what I am seeing, drm_debugfs_cleanup() is getting called from > the error handling path of drm_dev_register() and separately from > drm_dev_unregister() but not from the error handling path of > drm_modeset_register_all(). > > So that case will be unbalanced with this change. But for the practical reasons the drm_modeset_register_all() can not fail. But I see your point. I'll check why drm_dev_register() doesn't respect an error return from drm_modeset_register_all(). Until that point we can drop this patch. > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 -------- >>>> 1 file changed, 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index 32785cb1b079..8c45c949ec39 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct >>>> drm_encoder *encoder) >>>> return _dpu_encoder_init_debugfs(encoder); >>>> } >>>> >>>> -static void dpu_encoder_early_unregister(struct drm_encoder *encoder) >>>> -{ >>>> - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder); >>>> - >>>> - debugfs_remove_recursive(dpu_enc->debugfs_root); >>>> -} >>>> - >>>> static int dpu_encoder_virt_add_phys_encs( >>>> struct msm_display_info *disp_info, >>>> struct dpu_encoder_virt *dpu_enc, >>>> @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs >>>> dpu_encoder_helper_funcs = { >>>> static const struct drm_encoder_funcs dpu_encoder_funcs = { >>>> .destroy = dpu_encoder_destroy, >>>> .late_register = dpu_encoder_late_register, >>>> - .early_unregister = dpu_encoder_early_unregister, >>>> }; >>>> >>>> struct drm_encoder *dpu_encoder_init(struct drm_device *dev, >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 32785cb1b079..8c45c949ec39 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder) return _dpu_encoder_init_debugfs(encoder); } -static void dpu_encoder_early_unregister(struct drm_encoder *encoder) -{ - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder); - - debugfs_remove_recursive(dpu_enc->debugfs_root); -} - static int dpu_encoder_virt_add_phys_encs( struct msm_display_info *disp_info, struct dpu_encoder_virt *dpu_enc, @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { static const struct drm_encoder_funcs dpu_encoder_funcs = { .destroy = dpu_encoder_destroy, .late_register = dpu_encoder_late_register, - .early_unregister = dpu_encoder_early_unregister, }; struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
There is no need to clean up debugfs manually, it will be done by the DRM core on device deregistration. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 -------- 1 file changed, 8 deletions(-)