Re: [PATCH v3] drm/msm: validate display and event threads

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, &param);
  

• 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, &param);
  

• 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, &param);
  

• 			 SCHED_FIFO, &param);
  

  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