Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On Mon, Apr 22, 2024 at 09:12:20AM -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. > > > > Got it, a better commit text would have helped here. Either way, I'll update the commit text with the text above. > > Reviewed-by: Abhinav Kumar -- With best wishes Dmitry
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
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
> > > > ---
> > > >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;
> > > > }
> > > >
> >
--
With best wishes
Dmitry
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
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
---
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;
}
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
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. Got it, a better commit text would have helped here. Either way, Reviewed-by: Abhinav Kumar
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
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
> > ---
> > 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;
> > }
> >
--
With best wishes
Dmitry
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
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.
Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()")
Signed-off-by: Dmitry Baryshkov
---
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;
}
