| Message ID | 20220504154909.1.Iaebd35e60160fc0f2a50fac3a0bf3b298c0637c8@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Fix shutdown | expand |
On 5/4/2022 3:49 PM, Douglas Anderson wrote: > When rebooting on my sc7280-herobrine based device, I got a > crash. Upon debugging, I found that I was in msm_drv_shutdown() and my > "pdev" was the one associated with mdss_probe(). > > From source, I found that mdss_probe() has the line: > platform_set_drvdata(pdev, mdss); > ...where "mdss" is of type "struct msm_mdss *". > > Also from source, I saw that in msm_drv_shutdown() we have the line: > struct msm_drm_private *priv = platform_get_drvdata(pdev); > > This is a mismatch and is the root of the problem. > > Further digging made it apparent that msm_drv_shutdown() is only > supposed to be used for parts of the msm display framework that also > call msm_drv_probe() but mdss_probe() doesn't call > msm_drv_probe(). Let's remove the shutdown functon from msm_mdss.c. > > Digging a little further, code inspection found that two drivers that > use msm_drv_probe() weren't calling msm_drv_shutdown(). Let's add it > to them. > > Fixes: ecb23f2e3009 ("drm/msm: split the main platform driver") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Makes sense to me, and issue should happen everytime we shutdown so not sure how it didnt hit? > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > drivers/gpu/drm/msm/msm_mdss.c | 1 - > 3 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 143d6643be53..2b9d931474e0 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1350,6 +1350,7 @@ MODULE_DEVICE_TABLE(of, dpu_dt_match); > static struct platform_driver dpu_driver = { > .probe = dpu_dev_probe, > .remove = dpu_dev_remove, > + .shutdown = msm_drv_shutdown, > .driver = { > .name = "msm_dpu", > .of_match_table = dpu_dt_match, > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index 9b7bbc3adb97..3d5621a68f85 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -1009,6 +1009,7 @@ MODULE_DEVICE_TABLE(of, mdp5_dt_match); > static struct platform_driver mdp5_driver = { > .probe = mdp5_dev_probe, > .remove = mdp5_dev_remove, > + .shutdown = msm_drv_shutdown, > .driver = { > .name = "msm_mdp", > .of_match_table = mdp5_dt_match, > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > index 20f154dda9cf..0454a571adf7 100644 > --- a/drivers/gpu/drm/msm/msm_mdss.c > +++ b/drivers/gpu/drm/msm/msm_mdss.c > @@ -397,7 +397,6 @@ MODULE_DEVICE_TABLE(of, mdss_dt_match); > static struct platform_driver mdss_platform_driver = { > .probe = mdss_probe, > .remove = mdss_remove, > - .shutdown = msm_drv_shutdown, Question to doug/dmitry: Now that we removed msm_drv_shutdown, perhaps we should have a mdss_shutdown instead and call msm_mdss_destroy() internally? > .driver = { > .name = "msm-mdss", > .of_match_table = mdss_dt_match,
On Thu, 5 May 2022 at 01:49, Douglas Anderson <dianders@chromium.org> wrote: > > When rebooting on my sc7280-herobrine based device, I got a > crash. Upon debugging, I found that I was in msm_drv_shutdown() and my > "pdev" was the one associated with mdss_probe(). > > From source, I found that mdss_probe() has the line: > platform_set_drvdata(pdev, mdss); > ...where "mdss" is of type "struct msm_mdss *". > > Also from source, I saw that in msm_drv_shutdown() we have the line: > struct msm_drm_private *priv = platform_get_drvdata(pdev); > > This is a mismatch and is the root of the problem. > > Further digging made it apparent that msm_drv_shutdown() is only > supposed to be used for parts of the msm display framework that also > call msm_drv_probe() but mdss_probe() doesn't call > msm_drv_probe(). Let's remove the shutdown functon from msm_mdss.c. > > Digging a little further, code inspection found that two drivers that > use msm_drv_probe() weren't calling msm_drv_shutdown(). Let's add it > to them. > > Fixes: ecb23f2e3009 ("drm/msm: split the main platform driver") More likely: Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components") With that fixed: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > drivers/gpu/drm/msm/msm_mdss.c | 1 - > 3 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 143d6643be53..2b9d931474e0 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1350,6 +1350,7 @@ MODULE_DEVICE_TABLE(of, dpu_dt_match); > static struct platform_driver dpu_driver = { > .probe = dpu_dev_probe, > .remove = dpu_dev_remove, > + .shutdown = msm_drv_shutdown, > .driver = { > .name = "msm_dpu", > .of_match_table = dpu_dt_match, > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index 9b7bbc3adb97..3d5621a68f85 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -1009,6 +1009,7 @@ MODULE_DEVICE_TABLE(of, mdp5_dt_match); > static struct platform_driver mdp5_driver = { > .probe = mdp5_dev_probe, > .remove = mdp5_dev_remove, > + .shutdown = msm_drv_shutdown, > .driver = { > .name = "msm_mdp", > .of_match_table = mdp5_dt_match, > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > index 20f154dda9cf..0454a571adf7 100644 > --- a/drivers/gpu/drm/msm/msm_mdss.c > +++ b/drivers/gpu/drm/msm/msm_mdss.c > @@ -397,7 +397,6 @@ MODULE_DEVICE_TABLE(of, mdss_dt_match); > static struct platform_driver mdss_platform_driver = { > .probe = mdss_probe, > .remove = mdss_remove, > - .shutdown = msm_drv_shutdown, > .driver = { > .name = "msm-mdss", > .of_match_table = mdss_dt_match, > -- > 2.36.0.464.gb9c8b46e94-goog >
On Thu, 5 May 2022 at 02:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 5/4/2022 3:49 PM, Douglas Anderson wrote: > > When rebooting on my sc7280-herobrine based device, I got a > > crash. Upon debugging, I found that I was in msm_drv_shutdown() and my > > "pdev" was the one associated with mdss_probe(). > > > > From source, I found that mdss_probe() has the line: > > platform_set_drvdata(pdev, mdss); > > ...where "mdss" is of type "struct msm_mdss *". > > > > Also from source, I saw that in msm_drv_shutdown() we have the line: > > struct msm_drm_private *priv = platform_get_drvdata(pdev); > > > > This is a mismatch and is the root of the problem. > > > > Further digging made it apparent that msm_drv_shutdown() is only > > supposed to be used for parts of the msm display framework that also > > call msm_drv_probe() but mdss_probe() doesn't call > > msm_drv_probe(). Let's remove the shutdown functon from msm_mdss.c. > > > > Digging a little further, code inspection found that two drivers that > > use msm_drv_probe() weren't calling msm_drv_shutdown(). Let's add it > > to them. > > > > Fixes: ecb23f2e3009 ("drm/msm: split the main platform driver") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Makes sense to me, and issue should happen everytime we shutdown so not > sure how it didnt hit? > > > --- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > > drivers/gpu/drm/msm/msm_mdss.c | 1 - > > 3 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index 143d6643be53..2b9d931474e0 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -1350,6 +1350,7 @@ MODULE_DEVICE_TABLE(of, dpu_dt_match); > > static struct platform_driver dpu_driver = { > > .probe = dpu_dev_probe, > > .remove = dpu_dev_remove, > > + .shutdown = msm_drv_shutdown, > > .driver = { > > .name = "msm_dpu", > > .of_match_table = dpu_dt_match, > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > > index 9b7bbc3adb97..3d5621a68f85 100644 > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > > @@ -1009,6 +1009,7 @@ MODULE_DEVICE_TABLE(of, mdp5_dt_match); > > static struct platform_driver mdp5_driver = { > > .probe = mdp5_dev_probe, > > .remove = mdp5_dev_remove, > > + .shutdown = msm_drv_shutdown, > > .driver = { > > .name = "msm_mdp", > > .of_match_table = mdp5_dt_match, > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > > index 20f154dda9cf..0454a571adf7 100644 > > --- a/drivers/gpu/drm/msm/msm_mdss.c > > +++ b/drivers/gpu/drm/msm/msm_mdss.c > > @@ -397,7 +397,6 @@ MODULE_DEVICE_TABLE(of, mdss_dt_match); > > static struct platform_driver mdss_platform_driver = { > > .probe = mdss_probe, > > .remove = mdss_remove, > > - .shutdown = msm_drv_shutdown, > > Question to doug/dmitry: > > Now that we removed msm_drv_shutdown, perhaps we should have a > mdss_shutdown instead and call msm_mdss_destroy() internally? No need to. msm-mdss driver doesn't really need to be shutdown. It doesn't setup DMA, it doesn't setup video pipes, etc. > > > .driver = { > > .name = "msm-mdss", > > .of_match_table = mdss_dt_match,
On 5/4/2022 4:30 PM, Dmitry Baryshkov wrote: > On Thu, 5 May 2022 at 02:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 5/4/2022 3:49 PM, Douglas Anderson wrote: >>> When rebooting on my sc7280-herobrine based device, I got a >>> crash. Upon debugging, I found that I was in msm_drv_shutdown() and my >>> "pdev" was the one associated with mdss_probe(). >>> >>> From source, I found that mdss_probe() has the line: >>> platform_set_drvdata(pdev, mdss); >>> ...where "mdss" is of type "struct msm_mdss *". >>> >>> Also from source, I saw that in msm_drv_shutdown() we have the line: >>> struct msm_drm_private *priv = platform_get_drvdata(pdev); >>> >>> This is a mismatch and is the root of the problem. >>> >>> Further digging made it apparent that msm_drv_shutdown() is only >>> supposed to be used for parts of the msm display framework that also >>> call msm_drv_probe() but mdss_probe() doesn't call >>> msm_drv_probe(). Let's remove the shutdown functon from msm_mdss.c. >>> >>> Digging a little further, code inspection found that two drivers that >>> use msm_drv_probe() weren't calling msm_drv_shutdown(). Let's add it >>> to them. >>> >>> Fixes: ecb23f2e3009 ("drm/msm: split the main platform driver") >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> >> Makes sense to me, and issue should happen everytime we shutdown so not >> sure how it didnt hit? >> >>> --- >>> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + >>> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + >>> drivers/gpu/drm/msm/msm_mdss.c | 1 - >>> 3 files changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> index 143d6643be53..2b9d931474e0 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> @@ -1350,6 +1350,7 @@ MODULE_DEVICE_TABLE(of, dpu_dt_match); >>> static struct platform_driver dpu_driver = { >>> .probe = dpu_dev_probe, >>> .remove = dpu_dev_remove, >>> + .shutdown = msm_drv_shutdown, >>> .driver = { >>> .name = "msm_dpu", >>> .of_match_table = dpu_dt_match, >>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c >>> index 9b7bbc3adb97..3d5621a68f85 100644 >>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c >>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c >>> @@ -1009,6 +1009,7 @@ MODULE_DEVICE_TABLE(of, mdp5_dt_match); >>> static struct platform_driver mdp5_driver = { >>> .probe = mdp5_dev_probe, >>> .remove = mdp5_dev_remove, >>> + .shutdown = msm_drv_shutdown, >>> .driver = { >>> .name = "msm_mdp", >>> .of_match_table = mdp5_dt_match, >>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c >>> index 20f154dda9cf..0454a571adf7 100644 >>> --- a/drivers/gpu/drm/msm/msm_mdss.c >>> +++ b/drivers/gpu/drm/msm/msm_mdss.c >>> @@ -397,7 +397,6 @@ MODULE_DEVICE_TABLE(of, mdss_dt_match); >>> static struct platform_driver mdss_platform_driver = { >>> .probe = mdss_probe, >>> .remove = mdss_remove, >>> - .shutdown = msm_drv_shutdown, >> >> Question to doug/dmitry: >> >> Now that we removed msm_drv_shutdown, perhaps we should have a >> mdss_shutdown instead and call msm_mdss_destroy() internally? > > No need to. msm-mdss driver doesn't really need to be shutdown. It > doesn't setup DMA, it doesn't setup video pipes, etc. Alright, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> >>> .driver = { >>> .name = "msm-mdss", >>> .of_match_table = mdss_dt_match, > > >
Hi, On Wed, May 4, 2022 at 4:29 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 5 May 2022 at 01:49, Douglas Anderson <dianders@chromium.org> wrote: > > > > When rebooting on my sc7280-herobrine based device, I got a > > crash. Upon debugging, I found that I was in msm_drv_shutdown() and my > > "pdev" was the one associated with mdss_probe(). > > > > From source, I found that mdss_probe() has the line: > > platform_set_drvdata(pdev, mdss); > > ...where "mdss" is of type "struct msm_mdss *".msm_drv_probe > > > > Also from source, I saw that in msm_drv_shutdown() we have the line: > > struct msm_drm_private *priv = platform_get_drvdata(pdev); > > > > This is a mismatch and is the root of the problem. > > > > Further digging made it apparent that msm_drv_shutdown() is only > > supposed to be used for parts of the msm display framework that also > > call msm_drv_probe() but mdss_probe() doesn't call > > msm_drv_probe(). Let's remove the shutdown functon from msm_mdss.c. > > > > Digging a little further, code inspection found that two drivers that > > use msm_drv_probe() weren't calling msm_drv_shutdown(). Let's add it > > to them. > > > > Fixes: ecb23f2e3009 ("drm/msm: split the main platform driver") > > More likely: > Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components") Oh, I see! The commit I tagged is the one that set the shutdown in msm_mdss.c, but at the time of that commit it actually _did_ call msm_drv_probe(). v2 coming right up. -Doug
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 143d6643be53..2b9d931474e0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1350,6 +1350,7 @@ MODULE_DEVICE_TABLE(of, dpu_dt_match); static struct platform_driver dpu_driver = { .probe = dpu_dev_probe, .remove = dpu_dev_remove, + .shutdown = msm_drv_shutdown, .driver = { .name = "msm_dpu", .of_match_table = dpu_dt_match, diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 9b7bbc3adb97..3d5621a68f85 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -1009,6 +1009,7 @@ MODULE_DEVICE_TABLE(of, mdp5_dt_match); static struct platform_driver mdp5_driver = { .probe = mdp5_dev_probe, .remove = mdp5_dev_remove, + .shutdown = msm_drv_shutdown, .driver = { .name = "msm_mdp", .of_match_table = mdp5_dt_match, diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c index 20f154dda9cf..0454a571adf7 100644 --- a/drivers/gpu/drm/msm/msm_mdss.c +++ b/drivers/gpu/drm/msm/msm_mdss.c @@ -397,7 +397,6 @@ MODULE_DEVICE_TABLE(of, mdss_dt_match); static struct platform_driver mdss_platform_driver = { .probe = mdss_probe, .remove = mdss_remove, - .shutdown = msm_drv_shutdown, .driver = { .name = "msm-mdss", .of_match_table = mdss_dt_match,
When rebooting on my sc7280-herobrine based device, I got a crash. Upon debugging, I found that I was in msm_drv_shutdown() and my "pdev" was the one associated with mdss_probe(). From source, I found that mdss_probe() has the line: platform_set_drvdata(pdev, mdss); ...where "mdss" is of type "struct msm_mdss *". Also from source, I saw that in msm_drv_shutdown() we have the line: struct msm_drm_private *priv = platform_get_drvdata(pdev); This is a mismatch and is the root of the problem. Further digging made it apparent that msm_drv_shutdown() is only supposed to be used for parts of the msm display framework that also call msm_drv_probe() but mdss_probe() doesn't call msm_drv_probe(). Let's remove the shutdown functon from msm_mdss.c. Digging a little further, code inspection found that two drivers that use msm_drv_probe() weren't calling msm_drv_shutdown(). Let's add it to them. Fixes: ecb23f2e3009 ("drm/msm: split the main platform driver") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + drivers/gpu/drm/msm/msm_mdss.c | 1 - 3 files changed, 2 insertions(+), 1 deletion(-)