* [PATCH v3] drm/msm: validate display and event threads
@ 2018-10-08 23:55 Jeykumar Sankaran
[not found] ` <1539042945-28811-1-git-send-email-jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
0 siblings, 1 reply; 3+ messages in thread
From: Jeykumar Sankaran @ 2018-10-08 23:55 UTC (permalink / raw)
To: dri-devel, freedreno, linux-arm-msm; +Cc: abhinavk, hoegsberg, seanpaul
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(-)
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);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread[parent not found: <1539042945-28811-1-git-send-email-jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH v3] drm/msm: validate display and event threads [not found] ` <1539042945-28811-1-git-send-email-jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> @ 2018-10-09 14:24 ` Sean Paul 2018-10-09 18:13 ` Jeykumar Sankaran 0 siblings, 1 reply; 3+ messages in thread From: Sean Paul @ 2018-10-09 14:24 UTC (permalink / raw) To: Jeykumar Sankaran Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, jcrouse-sgV2jX0FEOL9JmXXK+q4OQ, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, robdclark-Re5JQEeQqe8AvxtiuMwx3w, seanpaul-F7+t8E8rja9g9hUCZPvPmw, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-hpIqsD4AKlfQT0dZR+AlfA, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 > -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] drm/msm: validate display and event threads 2018-10-09 14:24 ` Sean Paul @ 2018-10-09 18:13 ` Jeykumar Sankaran 0 siblings, 0 replies; 3+ messages in thread From: Jeykumar Sankaran @ 2018-10-09 18:13 UTC (permalink / raw) To: Sean Paul Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, jcrouse-sgV2jX0FEOL9JmXXK+q4OQ, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, robdclark-Re5JQEeQqe8AvxtiuMwx3w, seanpaul-F7+t8E8rja9g9hUCZPvPmw, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-hpIqsD4AKlfQT0dZR+AlfA, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 >> -- Jeykumar S _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-09 18:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-08 23:55 [PATCH v3] drm/msm: validate display and event threads Jeykumar Sankaran
[not found] ` <1539042945-28811-1-git-send-email-jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-09 14:24 ` Sean Paul
2018-10-09 18:13 ` Jeykumar Sankaran
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).