Re: [PATCH 1/2] drm/msm: use common display thread for dispatching vblank events

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

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,
> &param);
>> +	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, &param);
>> -		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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno