| Message ID | 1539042945-28811-1-git-send-email-jsanka@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/msm: validate display and event threads | expand |
On Mon, Oct 08, 2018 at 04:55:45PM -0700, Jeykumar Sankaran wrote: > While creating display and event threads per crtc, validate > them before setting their priorities. > > changes in v2: > - use dev_warn (Abhinav Kumar) > changes in v3: > - fix compilation error > > Change-Id: I1dda805286df981c0f0e2b26507d089d3a21ff6c No Change-Id's please! FWIW, checkpatch.pl would have caught this and the nits I found below. It might be worth running patches through it before sending. > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > --- > drivers/gpu/drm/msm/msm_drv.c | 49 ++++++++++++++----------------------------- > 1 file changed, 16 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 4904d0d..ab1b0a9 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -553,17 +553,18 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > kthread_run(kthread_worker_fn, > &priv->disp_thread[i].worker, > "crtc_commit:%d", priv->disp_thread[i].crtc_id); > - ret = sched_setscheduler(priv->disp_thread[i].thread, > - SCHED_FIFO, ¶m); > - if (ret) > - pr_warn("display thread priority update failed: %d\n", > - ret); > - > if (IS_ERR(priv->disp_thread[i].thread)) { > dev_err(dev, "failed to create crtc_commit kthread\n"); > priv->disp_thread[i].thread = NULL; > + goto err_msm_uninit; > } > > + ret = sched_setscheduler(priv->disp_thread[i].thread, > + SCHED_FIFO, ¶m); > + if (ret) > + dev_warn(dev, "display thread priority update failed: %d\n", Although this is wrapped, the line still exceeds 80 chars. Perhaps: dev_warn(dev, "disp_thread set priority failed %d\n", ret); > + ret); > + > /* initialize event thread */ > priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id; > kthread_init_worker(&priv->event_thread[i].worker); > @@ -572,6 +573,12 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > kthread_run(kthread_worker_fn, > &priv->event_thread[i].worker, > "crtc_event:%d", priv->event_thread[i].crtc_id); > + if (IS_ERR(priv->event_thread[i].thread)) { > + dev_err(dev, "failed to create crtc_event kthread\n"); > + priv->event_thread[i].thread = NULL; > + goto err_msm_uninit; > + } > + > /** > * event thread should also run at same priority as disp_thread > * because it is handling frame_done events. A lower priority > @@ -580,34 +587,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > * failure at crtc commit level. > */ > ret = sched_setscheduler(priv->event_thread[i].thread, > - SCHED_FIFO, ¶m); > + SCHED_FIFO, ¶m); > if (ret) > - pr_warn("display event thread priority update failed: %d\n", > - ret); > - > - if (IS_ERR(priv->event_thread[i].thread)) { > - dev_err(dev, "failed to create crtc_event kthread\n"); > - priv->event_thread[i].thread = NULL; > - } > - > - if ((!priv->disp_thread[i].thread) || > - !priv->event_thread[i].thread) { > - /* clean up previously created threads if any */ > - for ( ; i >= 0; i--) { > - if (priv->disp_thread[i].thread) { > - kthread_stop( > - priv->disp_thread[i].thread); > - priv->disp_thread[i].thread = NULL; > - } > - > - if (priv->event_thread[i].thread) { > - kthread_stop( > - priv->event_thread[i].thread); > - priv->event_thread[i].thread = NULL; > - } > - } > - goto err_msm_uninit; > - } > + dev_warn(dev, "display event thread priority update failed:%d\n", Same problem here, dev_warn(dev, "event thread set priority failed %d\n", > + ret); > } > > ret = drm_vblank_init(ddev, priv->num_crtcs); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 2018-10-09 07:24, Sean Paul wrote: > On Mon, Oct 08, 2018 at 04:55:45PM -0700, Jeykumar Sankaran wrote: >> While creating display and event threads per crtc, validate >> them before setting their priorities. >> >> changes in v2: >> - use dev_warn (Abhinav Kumar) >> changes in v3: >> - fix compilation error >> >> Change-Id: I1dda805286df981c0f0e2b26507d089d3a21ff6c > > No Change-Id's please! > > FWIW, checkpatch.pl would have caught this and the nits I found below. > It > might > be worth running patches through it before sending. > >> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> >> --- >> drivers/gpu/drm/msm/msm_drv.c | 49 > ++++++++++++++----------------------------- >> 1 file changed, 16 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_drv.c > b/drivers/gpu/drm/msm/msm_drv.c >> index 4904d0d..ab1b0a9 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.c >> +++ b/drivers/gpu/drm/msm/msm_drv.c >> @@ -553,17 +553,18 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> kthread_run(kthread_worker_fn, >> &priv->disp_thread[i].worker, >> "crtc_commit:%d", > priv->disp_thread[i].crtc_id); >> - ret = sched_setscheduler(priv->disp_thread[i].thread, >> - SCHED_FIFO, > ¶m); >> - if (ret) >> - pr_warn("display thread priority update failed: > %d\n", >> - > ret); >> - >> if (IS_ERR(priv->disp_thread[i].thread)) { >> dev_err(dev, "failed to create crtc_commit > kthread\n"); >> priv->disp_thread[i].thread = NULL; >> + goto err_msm_uninit; >> } >> >> + ret = sched_setscheduler(priv->disp_thread[i].thread, >> + SCHED_FIFO, ¶m); >> + if (ret) >> + dev_warn(dev, "display thread priority update > failed: %d\n", > > Although this is wrapped, the line still exceeds 80 chars. Perhaps: > > dev_warn(dev, "disp_thread set priority failed > %d\n", > ret); > I did run the checkpath.pl on this patch. Check patch usually will not complain if the >80 exceeding line is the logging string. https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html#breaking-long-lines-and-strings https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/scripts/checkpatch.pl#L3058 Anyway, rewording will be a good idea to avoid nits! >> + ret); >> + >> /* initialize event thread */ >> priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id; >> kthread_init_worker(&priv->event_thread[i].worker); >> @@ -572,6 +573,12 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> kthread_run(kthread_worker_fn, >> &priv->event_thread[i].worker, >> "crtc_event:%d", > priv->event_thread[i].crtc_id); >> + if (IS_ERR(priv->event_thread[i].thread)) { >> + dev_err(dev, "failed to create crtc_event > kthread\n"); >> + priv->event_thread[i].thread = NULL; >> + goto err_msm_uninit; >> + } >> + >> /** >> * event thread should also run at same priority as > disp_thread >> * because it is handling frame_done events. A lower > priority >> @@ -580,34 +587,10 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> * failure at crtc commit level. >> */ >> ret = sched_setscheduler(priv->event_thread[i].thread, >> - SCHED_FIFO, > ¶m); >> + SCHED_FIFO, ¶m); >> if (ret) >> - pr_warn("display event thread priority update > failed: %d\n", >> - > ret); >> - >> - if (IS_ERR(priv->event_thread[i].thread)) { >> - dev_err(dev, "failed to create crtc_event > kthread\n"); >> - priv->event_thread[i].thread = NULL; >> - } >> - >> - if ((!priv->disp_thread[i].thread) || >> - !priv->event_thread[i].thread) { >> - /* clean up previously created threads if any */ >> - for ( ; i >= 0; i--) { >> - if (priv->disp_thread[i].thread) { >> - kthread_stop( >> - > priv->disp_thread[i].thread); >> - priv->disp_thread[i].thread = > NULL; >> - } >> - >> - if (priv->event_thread[i].thread) { >> - kthread_stop( >> - > priv->event_thread[i].thread); >> - priv->event_thread[i].thread = > NULL; >> - } >> - } >> - goto err_msm_uninit; >> - } >> + dev_warn(dev, "display event thread priority > update failed:%d\n", > > Same problem here, > dev_warn(dev, "event thread set priority failed > %d\n", > > >> + ret); >> } >> >> ret = drm_vblank_init(ddev, priv->num_crtcs); >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > Forum, >> a Linux Foundation Collaborative Project >>
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 4904d0d..ab1b0a9 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -553,17 +553,18 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) kthread_run(kthread_worker_fn, &priv->disp_thread[i].worker, "crtc_commit:%d", priv->disp_thread[i].crtc_id); - ret = sched_setscheduler(priv->disp_thread[i].thread, - SCHED_FIFO, ¶m); - if (ret) - pr_warn("display thread priority update failed: %d\n", - ret); - if (IS_ERR(priv->disp_thread[i].thread)) { dev_err(dev, "failed to create crtc_commit kthread\n"); priv->disp_thread[i].thread = NULL; + goto err_msm_uninit; } + ret = sched_setscheduler(priv->disp_thread[i].thread, + SCHED_FIFO, ¶m); + if (ret) + dev_warn(dev, "display thread priority update failed: %d\n", + ret); + /* initialize event thread */ priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id; kthread_init_worker(&priv->event_thread[i].worker); @@ -572,6 +573,12 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) kthread_run(kthread_worker_fn, &priv->event_thread[i].worker, "crtc_event:%d", priv->event_thread[i].crtc_id); + if (IS_ERR(priv->event_thread[i].thread)) { + dev_err(dev, "failed to create crtc_event kthread\n"); + priv->event_thread[i].thread = NULL; + goto err_msm_uninit; + } + /** * event thread should also run at same priority as disp_thread * because it is handling frame_done events. A lower priority @@ -580,34 +587,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) * failure at crtc commit level. */ ret = sched_setscheduler(priv->event_thread[i].thread, - SCHED_FIFO, ¶m); + SCHED_FIFO, ¶m); if (ret) - pr_warn("display event thread priority update failed: %d\n", - ret); - - if (IS_ERR(priv->event_thread[i].thread)) { - dev_err(dev, "failed to create crtc_event kthread\n"); - priv->event_thread[i].thread = NULL; - } - - if ((!priv->disp_thread[i].thread) || - !priv->event_thread[i].thread) { - /* clean up previously created threads if any */ - for ( ; i >= 0; i--) { - if (priv->disp_thread[i].thread) { - kthread_stop( - priv->disp_thread[i].thread); - priv->disp_thread[i].thread = NULL; - } - - if (priv->event_thread[i].thread) { - kthread_stop( - priv->event_thread[i].thread); - priv->event_thread[i].thread = NULL; - } - } - goto err_msm_uninit; - } + dev_warn(dev, "display event thread priority update failed:%d\n", + ret); } ret = drm_vblank_init(ddev, priv->num_crtcs);
While creating display and event threads per crtc, validate them before setting their priorities. changes in v2: - use dev_warn (Abhinav Kumar) changes in v3: - fix compilation error Change-Id: I1dda805286df981c0f0e2b26507d089d3a21ff6c Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> --- drivers/gpu/drm/msm/msm_drv.c | 49 ++++++++++++++----------------------------- 1 file changed, 16 insertions(+), 33 deletions(-)