[v2] drm/msm: validate display and event threads

Submitted by Jeykumar Sankaran on Oct. 8, 2018, 11:32 p.m.

Details

Message ID 1539041538-18152-1-git-send-email-jsanka@codeaurora.org
State New
Headers show
Series "drm/msm: validate display and event threads" ( rev: 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Jeykumar Sankaran Oct. 8, 2018, 11:32 p.m.
While creating display and event threads per crtc, validate
them before setting their priorities.

changes in v2:
	- use dev_warn (Abhinav Kumar)

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(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4904d0d..9df4047 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("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, &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("display event thread priority update failed:%d\n",
+				ret);
 	}
 
 	ret = drm_vblank_init(ddev, priv->num_crtcs);

Comments

Hi Jeykumar,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robclark/msm-next]
[also build test ERROR on v4.19-rc7 next-20181009]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jeykumar-Sankaran/drm-msm-validate-display-and-event-threads/20181009-203659
base:   git://people.freedesktop.org/~robclark/linux msm-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/hdmi.h:28:0,
                    from include/drm/drm_modes.h:30,
                    from include/drm/drm_bridge.h:29,
                    from include/drm/drm_of.h:7,
                    from drivers/gpu//drm/msm/msm_drv.c:21:
   drivers/gpu//drm/msm/msm_drv.c: In function 'msm_drm_init':
>> drivers/gpu//drm/msm/msm_drv.c:565:13: error: passing argument 1 of '_dev_warn' from incompatible pointer type [-Werror=incompatible-pointer-types]
       dev_warn("display thread priority update failed: %d\n",
                ^
   include/linux/device.h:1420:12: note: in definition of macro 'dev_warn'
     _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
               ^~~
   include/linux/device.h:1358:6: note: expected 'const struct device *' but argument is of type 'char *'
    void _dev_warn(const struct device *dev, const char *fmt, ...);
         ^~~~~~~~~
>> drivers/gpu//drm/msm/msm_drv.c:566:5: warning: passing argument 2 of '_dev_warn' makes pointer from integer without a cast [-Wint-conversion]
        ret);
        ^
   include/linux/device.h:1335:22: note: in definition of macro 'dev_fmt'
    #define dev_fmt(fmt) fmt
                         ^~~
>> drivers/gpu//drm/msm/msm_drv.c:565:4: note: in expansion of macro 'dev_warn'
       dev_warn("display thread priority update failed: %d\n",
       ^~~~~~~~
   include/linux/device.h:1358:6: note: expected 'const char *' but argument is of type 'int'
    void _dev_warn(const struct device *dev, const char *fmt, ...);
         ^~~~~~~~~
   drivers/gpu//drm/msm/msm_drv.c:592:13: error: passing argument 1 of '_dev_warn' from incompatible pointer type [-Werror=incompatible-pointer-types]
       dev_warn("display event thread priority update failed:%d\n",
                ^
   include/linux/device.h:1420:12: note: in definition of macro 'dev_warn'
     _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
               ^~~
   include/linux/device.h:1358:6: note: expected 'const struct device *' but argument is of type 'char *'
    void _dev_warn(const struct device *dev, const char *fmt, ...);
         ^~~~~~~~~
   drivers/gpu//drm/msm/msm_drv.c:593:5: warning: passing argument 2 of '_dev_warn' makes pointer from integer without a cast [-Wint-conversion]
        ret);
        ^
   include/linux/device.h:1335:22: note: in definition of macro 'dev_fmt'
    #define dev_fmt(fmt) fmt
                         ^~~
   drivers/gpu//drm/msm/msm_drv.c:592:4: note: in expansion of macro 'dev_warn'
       dev_warn("display event thread priority update failed:%d\n",
       ^~~~~~~~
   include/linux/device.h:1358:6: note: expected 'const char *' but argument is of type 'int'
    void _dev_warn(const struct device *dev, const char *fmt, ...);
         ^~~~~~~~~
   cc1: some warnings being treated as errors

vim +/dev_warn +565 drivers/gpu//drm/msm/msm_drv.c

   433	
   434	static int msm_drm_init(struct device *dev, struct drm_driver *drv)
   435	{
   436		struct platform_device *pdev = to_platform_device(dev);
   437		struct drm_device *ddev;
   438		struct msm_drm_private *priv;
   439		struct msm_kms *kms;
   440		struct msm_mdss *mdss;
   441		int ret, i;
   442		struct sched_param param;
   443	
   444		ddev = drm_dev_alloc(drv, dev);
   445		if (IS_ERR(ddev)) {
   446			dev_err(dev, "failed to allocate drm_device\n");
   447			return PTR_ERR(ddev);
   448		}
   449	
   450		platform_set_drvdata(pdev, ddev);
   451	
   452		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
   453		if (!priv) {
   454			ret = -ENOMEM;
   455			goto err_put_drm_dev;
   456		}
   457	
   458		ddev->dev_private = priv;
   459		priv->dev = ddev;
   460	
   461		switch (get_mdp_ver(pdev)) {
   462		case KMS_MDP5:
   463			ret = mdp5_mdss_init(ddev);
   464			break;
   465		case KMS_DPU:
   466			ret = dpu_mdss_init(ddev);
   467			break;
   468		default:
   469			ret = 0;
   470			break;
   471		}
   472		if (ret)
   473			goto err_free_priv;
   474	
   475		mdss = priv->mdss;
   476	
   477		priv->wq = alloc_ordered_workqueue("msm", 0);
   478	
   479		INIT_LIST_HEAD(&priv->inactive_list);
   480		INIT_LIST_HEAD(&priv->vblank_ctrl.event_list);
   481		kthread_init_work(&priv->vblank_ctrl.work, vblank_ctrl_worker);
   482		spin_lock_init(&priv->vblank_ctrl.lock);
   483	
   484		drm_mode_config_init(ddev);
   485	
   486		/* Bind all our sub-components: */
   487		ret = component_bind_all(dev, ddev);
   488		if (ret)
   489			goto err_destroy_mdss;
   490	
   491		ret = msm_init_vram(ddev);
   492		if (ret)
   493			goto err_msm_uninit;
   494	
   495		msm_gem_shrinker_init(ddev);
   496	
   497		switch (get_mdp_ver(pdev)) {
   498		case KMS_MDP4:
   499			kms = mdp4_kms_init(ddev);
   500			priv->kms = kms;
   501			break;
   502		case KMS_MDP5:
   503			kms = mdp5_kms_init(ddev);
   504			break;
   505		case KMS_DPU:
   506			kms = dpu_kms_init(ddev);
   507			priv->kms = kms;
   508			break;
   509		default:
   510			kms = ERR_PTR(-ENODEV);
   511			break;
   512		}
   513	
   514		if (IS_ERR(kms)) {
   515			/*
   516			 * NOTE: once we have GPU support, having no kms should not
   517			 * be considered fatal.. ideally we would still support gpu
   518			 * and (for example) use dmabuf/prime to share buffers with
   519			 * imx drm driver on iMX5
   520			 */
   521			dev_err(dev, "failed to load kms\n");
   522			ret = PTR_ERR(kms);
   523			goto err_msm_uninit;
   524		}
   525	
   526		/* Enable normalization of plane zpos */
   527		ddev->mode_config.normalize_zpos = true;
   528	
   529		if (kms) {
   530			ret = kms->funcs->hw_init(kms);
   531			if (ret) {
   532				dev_err(dev, "kms hw init failed: %d\n", ret);
   533				goto err_msm_uninit;
   534			}
   535		}
   536	
   537		ddev->mode_config.funcs = &mode_config_funcs;
   538		ddev->mode_config.helper_private = &mode_config_helper_funcs;
   539	
   540		/**
   541		 * this priority was found during empiric testing to have appropriate
   542		 * realtime scheduling to process display updates and interact with
   543		 * other real time and normal priority task
   544		 */
   545		param.sched_priority = 16;
   546		for (i = 0; i < priv->num_crtcs; i++) {
   547	
   548			/* initialize display thread */
   549			priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id;
   550			kthread_init_worker(&priv->disp_thread[i].worker);
   551			priv->disp_thread[i].dev = ddev;
   552			priv->disp_thread[i].thread =
   553				kthread_run(kthread_worker_fn,
   554					&priv->disp_thread[i].worker,
   555					"crtc_commit:%d", priv->disp_thread[i].crtc_id);
   556			if (IS_ERR(priv->disp_thread[i].thread)) {
   557				dev_err(dev, "failed to create crtc_commit kthread\n");
   558				priv->disp_thread[i].thread = NULL;
   559				goto err_msm_uninit;
   560			}
   561	
   562			ret = sched_setscheduler(priv->disp_thread[i].thread,
   563						 SCHED_FIFO, &param);
   564			if (ret)
 > 565				dev_warn("display thread priority update failed: %d\n",
 > 566					ret);
   567	
   568			/* initialize event thread */
   569			priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
   570			kthread_init_worker(&priv->event_thread[i].worker);
   571			priv->event_thread[i].dev = ddev;
   572			priv->event_thread[i].thread =
   573				kthread_run(kthread_worker_fn,
   574					&priv->event_thread[i].worker,
   575					"crtc_event:%d", priv->event_thread[i].crtc_id);
   576			if (IS_ERR(priv->event_thread[i].thread)) {
   577				dev_err(dev, "failed to create crtc_event kthread\n");
   578				priv->event_thread[i].thread = NULL;
   579				goto err_msm_uninit;
   580			}
   581	
   582			/**
   583			 * event thread should also run at same priority as disp_thread
   584			 * because it is handling frame_done events. A lower priority
   585			 * event thread and higher priority disp_thread can causes
   586			 * frame_pending counters beyond 2. This can lead to commit
   587			 * failure at crtc commit level.
   588			 */
   589			ret = sched_setscheduler(priv->event_thread[i].thread,
   590						 SCHED_FIFO, &param);
   591			if (ret)
   592				dev_warn("display event thread priority update failed:%d\n",
   593					ret);
   594		}
   595	
   596		ret = drm_vblank_init(ddev, priv->num_crtcs);
   597		if (ret < 0) {
   598			dev_err(dev, "failed to initialize vblank\n");
   599			goto err_msm_uninit;
   600		}
   601	
   602		if (kms) {
   603			pm_runtime_get_sync(dev);
   604			ret = drm_irq_install(ddev, kms->irq);
   605			pm_runtime_put_sync(dev);
   606			if (ret < 0) {
   607				dev_err(dev, "failed to install IRQ handler\n");
   608				goto err_msm_uninit;
   609			}
   610		}
   611	
   612		ret = drm_dev_register(ddev, 0);
   613		if (ret)
   614			goto err_msm_uninit;
   615	
   616		drm_mode_config_reset(ddev);
   617	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation