| Message ID | 20240420-mdp4-fixes-v1-0-96a70f64fa85@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/mdp4: fix probe deferral issues | expand |
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > Correct c&p error from the conversion of LCDC regulators to the bulk > API. > > Fixes: 54f1fbcb47d4 ("drm/msm/mdp4: use bulk regulators API for LCDC encoder") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Indeed ! I should have caught this during review :( Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: > > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > > MSM display drivers provide kms structure allocated during probe(). > > Don't clean up priv->kms field in case of an error. Otherwise probe > > functions might fail after KMS probe deferral. > > > > So just to understand this more, this will happen when master component > probe (dpu) succeeded but other sub-component probe (dsi) deferred? > > Because if master component probe itself deferred it will allocate priv->kms > again isnt it and we will not even hit here. Master probing succeeds (so priv->kms is set), then kms_init fails at runtime, during binding of the master device. This results in probe deferral from the last component's component_add() function and reprobe attempt when possible (once the next device is added or probed). However as priv->kms is NULL, probe crashes. > > > Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/msm_kms.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > > index af6a6fcb1173..6749f0fbca96 100644 > > --- a/drivers/gpu/drm/msm/msm_kms.c > > +++ b/drivers/gpu/drm/msm/msm_kms.c > > @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) > > ret = priv->kms_init(ddev); > > if (ret) { > > DRM_DEV_ERROR(dev, "failed to load kms\n"); > > - priv->kms = NULL; > > return ret; > > } > >
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: >> >> >> On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: >>> MSM display drivers provide kms structure allocated during probe(). >>> Don't clean up priv->kms field in case of an error. Otherwise probe >>> functions might fail after KMS probe deferral. >>> >> >> So just to understand this more, this will happen when master component >> probe (dpu) succeeded but other sub-component probe (dsi) deferred? >> >> Because if master component probe itself deferred it will allocate priv->kms >> again isnt it and we will not even hit here. > > Master probing succeeds (so priv->kms is set), then kms_init fails at > runtime, during binding of the master device. This results in probe > deferral from the last component's component_add() function and reprobe > attempt when possible (once the next device is added or probed). However > as priv->kms is NULL, probe crashes. > >> >>> Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") Actually, Is this Fixes tag correct? OR is this one better Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/msm_kms.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c >>> index af6a6fcb1173..6749f0fbca96 100644 >>> --- a/drivers/gpu/drm/msm/msm_kms.c >>> +++ b/drivers/gpu/drm/msm/msm_kms.c >>> @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) >>> ret = priv->kms_init(ddev); >>> if (ret) { >>> DRM_DEV_ERROR(dev, "failed to load kms\n"); >>> - priv->kms = NULL; >>> return ret; >>> } >>> >
On Mon, Apr 22, 2024 at 10:23:18AM -0700, Abhinav Kumar wrote: > > > On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: > > > > > > > > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > > > > MSM display drivers provide kms structure allocated during probe(). > > > > Don't clean up priv->kms field in case of an error. Otherwise probe > > > > functions might fail after KMS probe deferral. > > > > > > > > > > So just to understand this more, this will happen when master component > > > probe (dpu) succeeded but other sub-component probe (dsi) deferred? > > > > > > Because if master component probe itself deferred it will allocate priv->kms > > > again isnt it and we will not even hit here. > > > > Master probing succeeds (so priv->kms is set), then kms_init fails at > > runtime, during binding of the master device. This results in probe > > deferral from the last component's component_add() function and reprobe > > attempt when possible (once the next device is added or probed). However > > as priv->kms is NULL, probe crashes. > > > > > > > > > Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") > > Actually, Is this Fixes tag correct? > > OR is this one better > > Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c") No. The issue existed even before the carve-out. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/gpu/drm/msm/msm_kms.c | 1 - > > > > 1 file changed, 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > > > > index af6a6fcb1173..6749f0fbca96 100644 > > > > --- a/drivers/gpu/drm/msm/msm_kms.c > > > > +++ b/drivers/gpu/drm/msm/msm_kms.c > > > > @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) > > > > ret = priv->kms_init(ddev); > > > > if (ret) { > > > > DRM_DEV_ERROR(dev, "failed to load kms\n"); > > > > - priv->kms = NULL; > > > > return ret; > > > > } > > > > > >
While testing MDP4 LVDS support I noticed several issues (two are related to probe deferral case and last one is a c&p error in LCDC part). Fix those issues. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Dmitry Baryshkov (3): drm/msm: don't clean up priv->kms prematurely drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path drm/msm/mdp4: correct LCDC regulator name drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 ++++++++--------------- drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 2 +- drivers/gpu/drm/msm/msm_kms.c | 1 - 3 files changed, 10 insertions(+), 21 deletions(-) --- base-commit: 7b4f2bc91c15fdcf948bb2d9741a9d7d54303f8d change-id: 20240420-mdp4-fixes-f33b5a21308b Best regards,