| Message ID | 1541031545-20520-1-git-send-email-jsanka@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] drm/msm: use common display thread for dispatching vblank events | expand |
On Wed, Oct 31, 2018 at 05:19:04PM -0700, Jeykumar Sankaran wrote: > DPU was using one thread per display to dispatch async > commits and vblank requests. Since clean up already happened > in msm to use the common thread for all the display commits, > display threads are only used to cater vblank requests. Single > thread is sufficient to do the job without any performance hits. > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +--- > drivers/gpu/drm/msm/msm_drv.c | 50 ++++++++++++----------------- > drivers/gpu/drm/msm/msm_drv.h | 2 +- > 3 files changed, 23 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 82c55ef..aff20f5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc, > is_vid_mode = dpu_enc->disp_info.capabilities & > MSM_DISPLAY_CAP_VID_MODE; > > - if (drm_enc->crtc->index >= ARRAY_SIZE(priv->disp_thread)) { > - DPU_ERROR("invalid crtc index\n"); > - return -EINVAL; > - } > - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; > + disp_thread = &priv->disp_thread; > > /* > * when idle_pc is not supported, process only KICKOFF, STOP and MODESET > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 9c9f7ff..1f384b3 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv, > list_add_tail(&vbl_ev->node, &vbl_ctrl->event_list); > spin_unlock_irqrestore(&vbl_ctrl->lock, flags); > > - kthread_queue_work(&priv->disp_thread[crtc_id].worker, > - &vbl_ctrl->work); > + kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work); > > return 0; > } > @@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev) > kfree(vbl_ev); > } > > + kthread_flush_worker(&priv->disp_thread.worker); > + kthread_stop(priv->disp_thread.thread); I realize this is moving existing code, but is there a race here? You can't have work enqueued in between the flush and stop? You might also want to use kthread_destroy_worker to do this work (in a follow-up patch including the event threads too). > + priv->disp_thread.thread = NULL; > + > /* clean up display commit/event worker threads */ This comment needs updating now > for (i = 0; i < priv->num_crtcs; i++) { > - if (priv->disp_thread[i].thread) { > - kthread_flush_worker(&priv->disp_thread[i].worker); > - kthread_stop(priv->disp_thread[i].thread); > - priv->disp_thread[i].thread = NULL; > - } > - > if (priv->event_thread[i].thread) { > kthread_flush_worker(&priv->event_thread[i].worker); > kthread_stop(priv->event_thread[i].thread); > @@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > ddev->mode_config.funcs = &mode_config_funcs; > ddev->mode_config.helper_private = &mode_config_helper_funcs; > > + /* initialize display thread */ > + kthread_init_worker(&priv->disp_thread.worker); > + priv->disp_thread.dev = ddev; > + priv->disp_thread.thread = kthread_run(kthread_worker_fn, > + &priv->disp_thread.worker, > + "disp_thread"); > + if (IS_ERR(priv->disp_thread.thread)) { > + DRM_DEV_ERROR(dev, "failed to create crtc_commit kthread\n"); > + priv->disp_thread.thread = NULL; > + goto err_msm_uninit; > + } > + > + ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, ¶m); > + if (ret) > + pr_warn("display thread priority update failed: %d\n", ret); > + > /** > * this priority was found during empiric testing to have appropriate > * realtime scheduling to process display updates and interact with > @@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > */ > param.sched_priority = 16; > for (i = 0; i < priv->num_crtcs; i++) { > - > - /* initialize display thread */ > - priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id; > - kthread_init_worker(&priv->disp_thread[i].worker); > - priv->disp_thread[i].dev = ddev; > - priv->disp_thread[i].thread = > - kthread_run(kthread_worker_fn, > - &priv->disp_thread[i].worker, > - "crtc_commit:%d", priv->disp_thread[i].crtc_id); > - if (IS_ERR(priv->disp_thread[i].thread)) { > - DRM_DEV_ERROR(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, "disp_thread set priority 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); > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index 9d11f32..e81b1fa 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -197,7 +197,7 @@ struct msm_drm_private { > unsigned int num_crtcs; > struct drm_crtc *crtcs[MAX_CRTCS]; > > - struct msm_drm_thread disp_thread[MAX_CRTCS]; > + struct msm_drm_thread disp_thread; > struct msm_drm_thread event_thread[MAX_CRTCS]; > > unsigned int num_encoders; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
On Wed, Oct 31, 2018 at 05:19:04PM -0700, Jeykumar Sankaran wrote: > DPU was using one thread per display to dispatch async > commits and vblank requests. Since clean up already happened > in msm to use the common thread for all the display commits, > display threads are only used to cater vblank requests. Single > thread is sufficient to do the job without any performance hits. > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +--- > drivers/gpu/drm/msm/msm_drv.c | 50 ++++++++++++----------------- > drivers/gpu/drm/msm/msm_drv.h | 2 +- > 3 files changed, 23 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 82c55ef..aff20f5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc, > is_vid_mode = dpu_enc->disp_info.capabilities & > MSM_DISPLAY_CAP_VID_MODE; > > - if (drm_enc->crtc->index >= ARRAY_SIZE(priv->disp_thread)) { > - DPU_ERROR("invalid crtc index\n"); > - return -EINVAL; > - } > - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; > + disp_thread = &priv->disp_thread; > > /* > * when idle_pc is not supported, process only KICKOFF, STOP and MODESET > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 9c9f7ff..1f384b3 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv, > list_add_tail(&vbl_ev->node, &vbl_ctrl->event_list); > spin_unlock_irqrestore(&vbl_ctrl->lock, flags); > > - kthread_queue_work(&priv->disp_thread[crtc_id].worker, > - &vbl_ctrl->work); > + kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work); > > return 0; > } > @@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev) > kfree(vbl_ev); > } > > + kthread_flush_worker(&priv->disp_thread.worker); > + kthread_stop(priv->disp_thread.thread); > + priv->disp_thread.thread = NULL; > + kthread doesn't check for NULL or error - you'll want a if (!IS_ERR(priv->disp_thread.worker)) check for this block. > /* clean up display commit/event worker threads */ > for (i = 0; i < priv->num_crtcs; i++) { > - if (priv->disp_thread[i].thread) { > - kthread_flush_worker(&priv->disp_thread[i].worker); > - kthread_stop(priv->disp_thread[i].thread); > - priv->disp_thread[i].thread = NULL; > - } > - > if (priv->event_thread[i].thread) { > kthread_flush_worker(&priv->event_thread[i].worker); > kthread_stop(priv->event_thread[i].thread); > @@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > ddev->mode_config.funcs = &mode_config_funcs; > ddev->mode_config.helper_private = &mode_config_helper_funcs; > > + /* initialize display thread */ > + kthread_init_worker(&priv->disp_thread.worker); > + priv->disp_thread.dev = ddev; > + priv->disp_thread.thread = kthread_run(kthread_worker_fn, > + &priv->disp_thread.worker, > + "disp_thread"); This name should be more descriptive for the driver - disp_thread is a bit generic. > + if (IS_ERR(priv->disp_thread.thread)) { > + DRM_DEV_ERROR(dev, "failed to create crtc_commit kthread\n"); > + priv->disp_thread.thread = NULL; You don't need this if you check for !IS_ERR in msm_drm_uninit. > + goto err_msm_uninit; > + } > + > + ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, ¶m); > + if (ret) > + pr_warn("display thread priority update failed: %d\n", ret); > + <snip> Jordan
On 2018-11-01 12:09, Sean Paul wrote: > On Wed, Oct 31, 2018 at 05:19:04PM -0700, Jeykumar Sankaran wrote: >> DPU was using one thread per display to dispatch async >> commits and vblank requests. Since clean up already happened >> in msm to use the common thread for all the display commits, >> display threads are only used to cater vblank requests. Single >> thread is sufficient to do the job without any performance hits. >> >> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +--- >> drivers/gpu/drm/msm/msm_drv.c | 50 > ++++++++++++----------------- >> drivers/gpu/drm/msm/msm_drv.h | 2 +- >> 3 files changed, 23 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 82c55ef..aff20f5 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct > drm_encoder *drm_enc, >> is_vid_mode = dpu_enc->disp_info.capabilities & >> MSM_DISPLAY_CAP_VID_MODE; >> >> - if (drm_enc->crtc->index >= ARRAY_SIZE(priv->disp_thread)) { >> - DPU_ERROR("invalid crtc index\n"); >> - return -EINVAL; >> - } >> - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; >> + disp_thread = &priv->disp_thread; >> >> /* >> * when idle_pc is not supported, process only KICKOFF, STOP and > MODESET >> diff --git a/drivers/gpu/drm/msm/msm_drv.c > b/drivers/gpu/drm/msm/msm_drv.c >> index 9c9f7ff..1f384b3 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.c >> +++ b/drivers/gpu/drm/msm/msm_drv.c >> @@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct > msm_drm_private *priv, >> list_add_tail(&vbl_ev->node, &vbl_ctrl->event_list); >> spin_unlock_irqrestore(&vbl_ctrl->lock, flags); >> >> - kthread_queue_work(&priv->disp_thread[crtc_id].worker, >> - &vbl_ctrl->work); >> + kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work); >> >> return 0; >> } >> @@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev) >> kfree(vbl_ev); >> } >> >> + kthread_flush_worker(&priv->disp_thread.worker); >> + kthread_stop(priv->disp_thread.thread); > > I realize this is moving existing code, but is there a race here? You > can't have > work enqueued in between the flush and stop? Yes. I see only priv->kms is checked before queuing the work. I can move the thread cleanup after the drm_mode_config_cleanup which releases the CRTC objects. This way no one can make any further vblank requests. Thanks and Regards, Jeykumar S. > > You might also want to use kthread_destroy_worker to do this work (in a > follow-up patch including the event threads too). > >> + priv->disp_thread.thread = NULL; >> + >> /* clean up display commit/event worker threads */ > > This comment needs updating now > >> for (i = 0; i < priv->num_crtcs; i++) { >> - if (priv->disp_thread[i].thread) { >> - > kthread_flush_worker(&priv->disp_thread[i].worker); >> - kthread_stop(priv->disp_thread[i].thread); >> - priv->disp_thread[i].thread = NULL; >> - } >> - >> if (priv->event_thread[i].thread) { >> > kthread_flush_worker(&priv->event_thread[i].worker); >> kthread_stop(priv->event_thread[i].thread); >> @@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> ddev->mode_config.funcs = &mode_config_funcs; >> ddev->mode_config.helper_private = &mode_config_helper_funcs; >> >> + /* initialize display thread */ >> + kthread_init_worker(&priv->disp_thread.worker); >> + priv->disp_thread.dev = ddev; >> + priv->disp_thread.thread = kthread_run(kthread_worker_fn, >> + &priv->disp_thread.worker, >> + "disp_thread"); >> + if (IS_ERR(priv->disp_thread.thread)) { >> + DRM_DEV_ERROR(dev, "failed to create crtc_commit > kthread\n"); >> + priv->disp_thread.thread = NULL; >> + goto err_msm_uninit; >> + } >> + >> + ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, > ¶m); >> + if (ret) >> + pr_warn("display thread priority update failed: %d\n", > ret); >> + >> /** >> * this priority was found during empiric testing to have > appropriate >> * realtime scheduling to process display updates and interact > with >> @@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> */ >> param.sched_priority = 16; >> for (i = 0; i < priv->num_crtcs; i++) { >> - >> - /* initialize display thread */ >> - priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id; >> - kthread_init_worker(&priv->disp_thread[i].worker); >> - priv->disp_thread[i].dev = ddev; >> - priv->disp_thread[i].thread = >> - kthread_run(kthread_worker_fn, >> - &priv->disp_thread[i].worker, >> - "crtc_commit:%d", > priv->disp_thread[i].crtc_id); >> - if (IS_ERR(priv->disp_thread[i].thread)) { >> - DRM_DEV_ERROR(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, "disp_thread set priority 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); >> diff --git a/drivers/gpu/drm/msm/msm_drv.h > b/drivers/gpu/drm/msm/msm_drv.h >> index 9d11f32..e81b1fa 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.h >> +++ b/drivers/gpu/drm/msm/msm_drv.h >> @@ -197,7 +197,7 @@ struct msm_drm_private { >> unsigned int num_crtcs; >> struct drm_crtc *crtcs[MAX_CRTCS]; >> >> - struct msm_drm_thread disp_thread[MAX_CRTCS]; >> + struct msm_drm_thread disp_thread; >> struct msm_drm_thread event_thread[MAX_CRTCS]; >> >> unsigned int num_encoders; >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > Forum, >> a Linux Foundation Collaborative Project >> >> _______________________________________________ >> Freedreno mailing list >> Freedreno@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/freedreno
On 2018-11-01 12:09, Sean Paul wrote: > On Wed, Oct 31, 2018 at 05:19:04PM -0700, Jeykumar Sankaran wrote: >> DPU was using one thread per display to dispatch async >> commits and vblank requests. Since clean up already happened >> in msm to use the common thread for all the display commits, >> display threads are only used to cater vblank requests. Single >> thread is sufficient to do the job without any performance hits. >> >> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +--- >> drivers/gpu/drm/msm/msm_drv.c | 50 > ++++++++++++----------------- >> drivers/gpu/drm/msm/msm_drv.h | 2 +- >> 3 files changed, 23 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 82c55ef..aff20f5 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct > drm_encoder *drm_enc, >> is_vid_mode = dpu_enc->disp_info.capabilities & >> MSM_DISPLAY_CAP_VID_MODE; >> >> - if (drm_enc->crtc->index >= ARRAY_SIZE(priv->disp_thread)) { >> - DPU_ERROR("invalid crtc index\n"); >> - return -EINVAL; >> - } >> - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; >> + disp_thread = &priv->disp_thread; >> >> /* >> * when idle_pc is not supported, process only KICKOFF, STOP and > MODESET >> diff --git a/drivers/gpu/drm/msm/msm_drv.c > b/drivers/gpu/drm/msm/msm_drv.c >> index 9c9f7ff..1f384b3 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.c >> +++ b/drivers/gpu/drm/msm/msm_drv.c >> @@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct > msm_drm_private *priv, >> list_add_tail(&vbl_ev->node, &vbl_ctrl->event_list); >> spin_unlock_irqrestore(&vbl_ctrl->lock, flags); >> >> - kthread_queue_work(&priv->disp_thread[crtc_id].worker, >> - &vbl_ctrl->work); >> + kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work); >> >> return 0; >> } >> @@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev) >> kfree(vbl_ev); >> } >> >> + kthread_flush_worker(&priv->disp_thread.worker); >> + kthread_stop(priv->disp_thread.thread); > > I realize this is moving existing code, but is there a race here? You > can't have > work enqueued in between the flush and stop? I looked further into this comment. Ideally, we call into msm_unbind only when the device is released and we release the device only on the last close of the drm device. So the userspace doesn't have any device handle to make ioctl calls, which could queue jobs to this queue. Since we are making sure to flush out the last job already on the queue, we can safely call the kthread_stop here. Thanks and Regards, Jeykumar S. > > You might also want to use kthread_destroy_worker to do this work (in a > follow-up patch including the event threads too). > >> + priv->disp_thread.thread = NULL; >> + >> /* clean up display commit/event worker threads */ > > This comment needs updating now > >> for (i = 0; i < priv->num_crtcs; i++) { >> - if (priv->disp_thread[i].thread) { >> - > kthread_flush_worker(&priv->disp_thread[i].worker); >> - kthread_stop(priv->disp_thread[i].thread); >> - priv->disp_thread[i].thread = NULL; >> - } >> - >> if (priv->event_thread[i].thread) { >> > kthread_flush_worker(&priv->event_thread[i].worker); >> kthread_stop(priv->event_thread[i].thread); >> @@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> ddev->mode_config.funcs = &mode_config_funcs; >> ddev->mode_config.helper_private = &mode_config_helper_funcs; >> >> + /* initialize display thread */ >> + kthread_init_worker(&priv->disp_thread.worker); >> + priv->disp_thread.dev = ddev; >> + priv->disp_thread.thread = kthread_run(kthread_worker_fn, >> + &priv->disp_thread.worker, >> + "disp_thread"); >> + if (IS_ERR(priv->disp_thread.thread)) { >> + DRM_DEV_ERROR(dev, "failed to create crtc_commit > kthread\n"); >> + priv->disp_thread.thread = NULL; >> + goto err_msm_uninit; >> + } >> + >> + ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, > ¶m); >> + if (ret) >> + pr_warn("display thread priority update failed: %d\n", > ret); >> + >> /** >> * this priority was found during empiric testing to have > appropriate >> * realtime scheduling to process display updates and interact > with >> @@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, >> struct > drm_driver *drv) >> */ >> param.sched_priority = 16; >> for (i = 0; i < priv->num_crtcs; i++) { >> - >> - /* initialize display thread */ >> - priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id; >> - kthread_init_worker(&priv->disp_thread[i].worker); >> - priv->disp_thread[i].dev = ddev; >> - priv->disp_thread[i].thread = >> - kthread_run(kthread_worker_fn, >> - &priv->disp_thread[i].worker, >> - "crtc_commit:%d", > priv->disp_thread[i].crtc_id); >> - if (IS_ERR(priv->disp_thread[i].thread)) { >> - DRM_DEV_ERROR(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, "disp_thread set priority 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); >> diff --git a/drivers/gpu/drm/msm/msm_drv.h > b/drivers/gpu/drm/msm/msm_drv.h >> index 9d11f32..e81b1fa 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.h >> +++ b/drivers/gpu/drm/msm/msm_drv.h >> @@ -197,7 +197,7 @@ struct msm_drm_private { >> unsigned int num_crtcs; >> struct drm_crtc *crtcs[MAX_CRTCS]; >> >> - struct msm_drm_thread disp_thread[MAX_CRTCS]; >> + struct msm_drm_thread disp_thread; >> struct msm_drm_thread event_thread[MAX_CRTCS]; >> >> unsigned int num_encoders; >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > Forum, >> a Linux Foundation Collaborative Project >> >> _______________________________________________ >> Freedreno mailing list >> Freedreno@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/freedreno
On Thu, Nov 08, 2018 at 02:23:33PM -0800, Jeykumar Sankaran wrote: > On 2018-11-01 12:09, Sean Paul wrote: > > On Wed, Oct 31, 2018 at 05:19:04PM -0700, Jeykumar Sankaran wrote: > > > DPU was using one thread per display to dispatch async > > > commits and vblank requests. Since clean up already happened > > > in msm to use the common thread for all the display commits, > > > display threads are only used to cater vblank requests. Single > > > thread is sufficient to do the job without any performance hits. > > > > > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +--- > > > drivers/gpu/drm/msm/msm_drv.c | 50 > > ++++++++++++----------------- > > > drivers/gpu/drm/msm/msm_drv.h | 2 +- > > > 3 files changed, 23 insertions(+), 35 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > index 82c55ef..aff20f5 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > @@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct > > drm_encoder *drm_enc, > > > is_vid_mode = dpu_enc->disp_info.capabilities & > > > MSM_DISPLAY_CAP_VID_MODE; > > > > > > - if (drm_enc->crtc->index >= ARRAY_SIZE(priv->disp_thread)) { > > > - DPU_ERROR("invalid crtc index\n"); > > > - return -EINVAL; > > > - } > > > - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; > > > + disp_thread = &priv->disp_thread; > > > > > > /* > > > * when idle_pc is not supported, process only KICKOFF, STOP and > > MODESET > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c > > b/drivers/gpu/drm/msm/msm_drv.c > > > index 9c9f7ff..1f384b3 100644 > > > --- a/drivers/gpu/drm/msm/msm_drv.c > > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > > @@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct > > msm_drm_private *priv, > > > list_add_tail(&vbl_ev->node, &vbl_ctrl->event_list); > > > spin_unlock_irqrestore(&vbl_ctrl->lock, flags); > > > > > > - kthread_queue_work(&priv->disp_thread[crtc_id].worker, > > > - &vbl_ctrl->work); > > > + kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work); > > > > > > return 0; > > > } > > > @@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev) > > > kfree(vbl_ev); > > > } > > > > > > + kthread_flush_worker(&priv->disp_thread.worker); > > > + kthread_stop(priv->disp_thread.thread); > > > > I realize this is moving existing code, but is there a race here? You > > can't have > > work enqueued in between the flush and stop? > I looked further into this comment. Ideally, we call into msm_unbind only > when > the device is released and we release the device only on the last close of > the drm device. So the userspace doesn't have any device handle to make > ioctl calls, which could queue jobs to this queue. Since we are making sure > to flush out the last job already on the queue, we can safely call the > kthread_stop here. Cool, thanks for digging into this! Sean > > Thanks and Regards, > Jeykumar S. > > > > > You might also want to use kthread_destroy_worker to do this work (in a > > follow-up patch including the event threads too). > > > > > + priv->disp_thread.thread = NULL; > > > + > > > /* clean up display commit/event worker threads */ > > > > This comment needs updating now > > > > > for (i = 0; i < priv->num_crtcs; i++) { > > > - if (priv->disp_thread[i].thread) { > > > - > > kthread_flush_worker(&priv->disp_thread[i].worker); > > > - kthread_stop(priv->disp_thread[i].thread); > > > - priv->disp_thread[i].thread = NULL; > > > - } > > > - > > > if (priv->event_thread[i].thread) { > > > > > kthread_flush_worker(&priv->event_thread[i].worker); > > > kthread_stop(priv->event_thread[i].thread); > > > @@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, > > > struct > > drm_driver *drv) > > > ddev->mode_config.funcs = &mode_config_funcs; > > > ddev->mode_config.helper_private = &mode_config_helper_funcs; > > > > > > + /* initialize display thread */ > > > + kthread_init_worker(&priv->disp_thread.worker); > > > + priv->disp_thread.dev = ddev; > > > + priv->disp_thread.thread = kthread_run(kthread_worker_fn, > > > + &priv->disp_thread.worker, > > > + "disp_thread"); > > > + if (IS_ERR(priv->disp_thread.thread)) { > > > + DRM_DEV_ERROR(dev, "failed to create crtc_commit > > kthread\n"); > > > + priv->disp_thread.thread = NULL; > > > + goto err_msm_uninit; > > > + } > > > + > > > + ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, > > ¶m); > > > + if (ret) > > > + pr_warn("display thread priority update failed: %d\n", > > ret); > > > + > > > /** > > > * this priority was found during empiric testing to have > > appropriate > > > * realtime scheduling to process display updates and interact > > with > > > @@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, > > > struct > > drm_driver *drv) > > > */ > > > param.sched_priority = 16; > > > for (i = 0; i < priv->num_crtcs; i++) { > > > - > > > - /* initialize display thread */ > > > - priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id; > > > - kthread_init_worker(&priv->disp_thread[i].worker); > > > - priv->disp_thread[i].dev = ddev; > > > - priv->disp_thread[i].thread = > > > - kthread_run(kthread_worker_fn, > > > - &priv->disp_thread[i].worker, > > > - "crtc_commit:%d", > > priv->disp_thread[i].crtc_id); > > > - if (IS_ERR(priv->disp_thread[i].thread)) { > > > - DRM_DEV_ERROR(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, "disp_thread set priority 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); > > > diff --git a/drivers/gpu/drm/msm/msm_drv.h > > b/drivers/gpu/drm/msm/msm_drv.h > > > index 9d11f32..e81b1fa 100644 > > > --- a/drivers/gpu/drm/msm/msm_drv.h > > > +++ b/drivers/gpu/drm/msm/msm_drv.h > > > @@ -197,7 +197,7 @@ struct msm_drm_private { > > > unsigned int num_crtcs; > > > struct drm_crtc *crtcs[MAX_CRTCS]; > > > > > > - struct msm_drm_thread disp_thread[MAX_CRTCS]; > > > + struct msm_drm_thread disp_thread; > > > struct msm_drm_thread event_thread[MAX_CRTCS]; > > > > > > unsigned int num_encoders; > > > -- > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > > Forum, > > > a Linux Foundation Collaborative Project > > > > > > _______________________________________________ > > > Freedreno mailing list > > > Freedreno@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/freedreno > > -- > Jeykumar S
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 82c55ef..aff20f5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc, is_vid_mode = dpu_enc->disp_info.capabilities & MSM_DISPLAY_CAP_VID_MODE; - if (drm_enc->crtc->index >= ARRAY_SIZE(priv->disp_thread)) { - DPU_ERROR("invalid crtc index\n"); - return -EINVAL; - } - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; + disp_thread = &priv->disp_thread; /* * when idle_pc is not supported, process only KICKOFF, STOP and MODESET diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 9c9f7ff..1f384b3 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv, list_add_tail(&vbl_ev->node, &vbl_ctrl->event_list); spin_unlock_irqrestore(&vbl_ctrl->lock, flags); - kthread_queue_work(&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); + kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work); return 0; } @@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev) kfree(vbl_ev); } + kthread_flush_worker(&priv->disp_thread.worker); + kthread_stop(priv->disp_thread.thread); + priv->disp_thread.thread = NULL; + /* clean up display commit/event worker threads */ for (i = 0; i < priv->num_crtcs; i++) { - if (priv->disp_thread[i].thread) { - kthread_flush_worker(&priv->disp_thread[i].worker); - kthread_stop(priv->disp_thread[i].thread); - priv->disp_thread[i].thread = NULL; - } - if (priv->event_thread[i].thread) { kthread_flush_worker(&priv->event_thread[i].worker); kthread_stop(priv->event_thread[i].thread); @@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) ddev->mode_config.funcs = &mode_config_funcs; ddev->mode_config.helper_private = &mode_config_helper_funcs; + /* initialize display thread */ + kthread_init_worker(&priv->disp_thread.worker); + priv->disp_thread.dev = ddev; + priv->disp_thread.thread = kthread_run(kthread_worker_fn, + &priv->disp_thread.worker, + "disp_thread"); + if (IS_ERR(priv->disp_thread.thread)) { + DRM_DEV_ERROR(dev, "failed to create crtc_commit kthread\n"); + priv->disp_thread.thread = NULL; + goto err_msm_uninit; + } + + ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, ¶m); + if (ret) + pr_warn("display thread priority update failed: %d\n", ret); + /** * this priority was found during empiric testing to have appropriate * realtime scheduling to process display updates and interact with @@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) */ param.sched_priority = 16; for (i = 0; i < priv->num_crtcs; i++) { - - /* initialize display thread */ - priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id; - kthread_init_worker(&priv->disp_thread[i].worker); - priv->disp_thread[i].dev = ddev; - priv->disp_thread[i].thread = - kthread_run(kthread_worker_fn, - &priv->disp_thread[i].worker, - "crtc_commit:%d", priv->disp_thread[i].crtc_id); - if (IS_ERR(priv->disp_thread[i].thread)) { - DRM_DEV_ERROR(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, "disp_thread set priority 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); diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 9d11f32..e81b1fa 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -197,7 +197,7 @@ struct msm_drm_private { unsigned int num_crtcs; struct drm_crtc *crtcs[MAX_CRTCS]; - struct msm_drm_thread disp_thread[MAX_CRTCS]; + struct msm_drm_thread disp_thread; struct msm_drm_thread event_thread[MAX_CRTCS]; unsigned int num_encoders;
DPU was using one thread per display to dispatch async commits and vblank requests. Since clean up already happened in msm to use the common thread for all the display commits, display threads are only used to cater vblank requests. Single thread is sufficient to do the job without any performance hits. Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +--- drivers/gpu/drm/msm/msm_drv.c | 50 ++++++++++++----------------- drivers/gpu/drm/msm/msm_drv.h | 2 +- 3 files changed, 23 insertions(+), 35 deletions(-)