[PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Bhaktipriya Shridhar]

alloc_workqueue replaces deprecated create_singlethread_workqueue().

A dedicated workqueue has been used since work items need to be flushed
as a group rather than individually.

Since the flip_queue workqueue is involved in page-flipping and is not
being used on a memory reclaim path, WQ_MEM_RECLAIM has not been set.

Since there are only a fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 drivers/gpu/drm/radeon/radeon_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 6a41b49..bbb29c7 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -711,7 +711,7 @@ static void radeon_crtc_init(struct drm_device *dev, int index)

 	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
 	radeon_crtc->crtc_id = index;
-	radeon_crtc->flip_queue = create_singlethread_workqueue("radeon-crtc");
+	radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc", 0, 0);
 	rdev->mode_info.crtcs[index] = radeon_crtc;

 	if (rdev->family >= CHIP_BONAIRE) {
--
2.1.4

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Tejun Heo]

On Sat, Jul 02, 2016 at 04:33:50PM +0530, Bhaktipriya Shridhar wrote:
> alloc_workqueue replaces deprecated create_singlethread_workqueue().
> 
> A dedicated workqueue has been used since work items need to be flushed
> as a group rather than individually.
> 
> Since the flip_queue workqueue is involved in page-flipping and is not
> being used on a memory reclaim path, WQ_MEM_RECLAIM has not been set.
> 
> Since there are only a fixed number of work items, explicit concurrency
> limit is unnecessary here.

What are the involved work items?  Is it safe to run them
concurrently?

Thanks.

-- 
tejun

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Michel Dänzer]

On 02.07.2016 22:46, Tejun Heo wrote:
> On Sat, Jul 02, 2016 at 04:33:50PM +0530, Bhaktipriya Shridhar wrote:
>> alloc_workqueue replaces deprecated create_singlethread_workqueue().
>>
>> A dedicated workqueue has been used since work items need to be flushed
>> as a group rather than individually.
>>
>> Since the flip_queue workqueue is involved in page-flipping and is not
>> being used on a memory reclaim path, WQ_MEM_RECLAIM has not been set.
>>
>> Since there are only a fixed number of work items, explicit concurrency
>> limit is unnecessary here.
> 
> What are the involved work items?

drivers/gpu/drm/radeon/radeon_display.c:radeon_flip_work_func()


> Is it safe to run them concurrently?

Concurrently with what exactly?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Tejun Heo]

Hello,

On Mon, Jul 04, 2016 at 12:58:32PM +0900, Michel Dänzer wrote:
> On 02.07.2016 22:46, Tejun Heo wrote:
> > On Sat, Jul 02, 2016 at 04:33:50PM +0530, Bhaktipriya Shridhar wrote:
> >> alloc_workqueue replaces deprecated create_singlethread_workqueue().
> >>
> >> A dedicated workqueue has been used since work items need to be flushed
> >> as a group rather than individually.

There seem to be two work items involved, the flip one and unpin one.
Provided that there's no ordering requirement between the two, can't
we just flush them individually?

> >> Since the flip_queue workqueue is involved in page-flipping and is not
> >> being used on a memory reclaim path, WQ_MEM_RECLAIM has not been set.
> >>
> >> Since there are only a fixed number of work items, explicit concurrency
> >> limit is unnecessary here.
> > 
> > What are the involved work items?
> 
> drivers/gpu/drm/radeon/radeon_display.c:radeon_flip_work_func()
>
> > Is it safe to run them concurrently?
> 
> Concurrently with what exactly?

The flip and unpin work items.

Thanks.

-- 
tejun

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Michel Dänzer]

On 06.07.2016 06:06, Tejun Heo wrote:
> On Mon, Jul 04, 2016 at 12:58:32PM +0900, Michel Dänzer wrote:
>> On 02.07.2016 22:46, Tejun Heo wrote:
>>> On Sat, Jul 02, 2016 at 04:33:50PM +0530, Bhaktipriya Shridhar wrote:
>>>> alloc_workqueue replaces deprecated create_singlethread_workqueue().
>>>>
>>>> A dedicated workqueue has been used since work items need to be flushed
>>>> as a group rather than individually.
> 
> There seem to be two work items involved, the flip one and unpin one.
> Provided that there's no ordering requirement between the two, can't
> we just flush them individually?

There is an ordering requirement between the two queues, but it's
enforced by the driver (by only queuing the unpin work once a flip has
completed, which only happens after the corresponding flip work has run).


Not being very familiar with the workqueue APIs, I'll describe how it's
supposed to work from a driver POV, which will hopefully help you guys
decide on the most appropriate alloc_workqueue parameters.

There is one flip work queue for each hardware CRTC. At most one
radeon_flip_work_func item can be queued for any of them at any time.
When a radeon_flip_work_func item is queued, it should be executed ASAP
(so WQ_HIGHPRI might be appropriate?).

In contrast, the radeon_unpin_work_func items aren't particularly
urgent, and it's okay for several of them to be queued up. So I guess it
would actually make sense to use a different workqueue for them, maybe
even the default one?


>>>> Since there are only a fixed number of work items, explicit concurrency
>>>> limit is unnecessary here.
>>>
>>> What are the involved work items?
>>
>> drivers/gpu/drm/radeon/radeon_display.c:radeon_flip_work_func()
>>
>>> Is it safe to run them concurrently?
>>
>> Concurrently with what exactly?
> 
> The flip and unpin work items.

Yes, it's safe to run those concurrently.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Tejun Heo]

Hello, Michel.

On Wed, Jul 06, 2016 at 12:12:52PM +0900, Michel Dänzer wrote:
> There is an ordering requirement between the two queues, but it's
> enforced by the driver (by only queuing the unpin work once a flip has
> completed, which only happens after the corresponding flip work has run).

Oh I see.

> Not being very familiar with the workqueue APIs, I'll describe how it's
> supposed to work from a driver POV, which will hopefully help you guys
> decide on the most appropriate alloc_workqueue parameters.
> 
> There is one flip work queue for each hardware CRTC. At most one
> radeon_flip_work_func item can be queued for any of them at any time.
> When a radeon_flip_work_func item is queued, it should be executed ASAP
> (so WQ_HIGHPRI might be appropriate?).

Hmmm... the only time WQ_HIGHPRI should be used is when it'd otherwise
require a kthread w/ nice value at -20.  Would that be the case here?
What are the consequences of the work item getting delayed?  Also,
what kind of delays matter here?  Is it millisec range or micro?

> In contrast, the radeon_unpin_work_func items aren't particularly
> urgent, and it's okay for several of them to be queued up. So I guess it
> would actually make sense to use a different workqueue for them, maybe
> even the default one?

If the flip work doesn't require high priority kthread, both of them
can be queued to system_wq and flushed individually on shutdown.  The
default workqueue now has high enough concurrency that it isn't
necessary to worry about getting delayed by other work items.

Thanks.

-- 
tejun

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Michel Dänzer]

[-- Attachment #1.1: Type: text/plain, Size: 1173 bytes --]

On 06.07.2016 22:45, Tejun Heo wrote:
> On Wed, Jul 06, 2016 at 12:12:52PM +0900, Michel Dänzer wrote:
> 
>> Not being very familiar with the workqueue APIs, I'll describe how it's
>> supposed to work from a driver POV, which will hopefully help you guys
>> decide on the most appropriate alloc_workqueue parameters.
>>
>> There is one flip work queue for each hardware CRTC. At most one
>> radeon_flip_work_func item can be queued for any of them at any time.
>> When a radeon_flip_work_func item is queued, it should be executed ASAP
>> (so WQ_HIGHPRI might be appropriate?).
> 
> Hmmm... the only time WQ_HIGHPRI should be used is when it'd otherwise
> require a kthread w/ nice value at -20.  Would that be the case here?
> What are the consequences of the work item getting delayed?

A page flip may be delayed to a later display refresh cycle.


> Also, what kind of delays matter here?  Is it millisec range or micro?

It can be the latter in theory, but normally rather the former.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Christian König]

Am 07.07.2016 um 05:32 schrieb Michel Dänzer:
> On 06.07.2016 22:45, Tejun Heo wrote:
>> On Wed, Jul 06, 2016 at 12:12:52PM +0900, Michel Dänzer wrote:
>>
>>> Not being very familiar with the workqueue APIs, I'll describe how it's
>>> supposed to work from a driver POV, which will hopefully help you guys
>>> decide on the most appropriate alloc_workqueue parameters.
>>>
>>> There is one flip work queue for each hardware CRTC. At most one
>>> radeon_flip_work_func item can be queued for any of them at any time.
>>> When a radeon_flip_work_func item is queued, it should be executed ASAP
>>> (so WQ_HIGHPRI might be appropriate?).
>> Hmmm... the only time WQ_HIGHPRI should be used is when it'd otherwise
>> require a kthread w/ nice value at -20.  Would that be the case here?
>> What are the consequences of the work item getting delayed?
> A page flip may be delayed to a later display refresh cycle.
>
>
>> Also, what kind of delays matter here?  Is it millisec range or micro?
> It can be the latter in theory, but normally rather the former.

Well to be precise with a typical 1920x1080@60 resolution you have about 
2.16ms time under ideal conditions for the flip.

So using the high priority queue still sounds like a good idea to me.

Regards,
Christian.

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Michel Dänzer]

[-- Attachment #1.1: Type: text/plain, Size: 1871 bytes --]

On 07.07.2016 16:43, Christian König wrote:
> Am 07.07.2016 um 05:32 schrieb Michel Dänzer:
>> On 06.07.2016 22:45, Tejun Heo wrote:
>>> On Wed, Jul 06, 2016 at 12:12:52PM +0900, Michel Dänzer wrote:
>>>
>>>> Not being very familiar with the workqueue APIs, I'll describe how it's
>>>> supposed to work from a driver POV, which will hopefully help you guys
>>>> decide on the most appropriate alloc_workqueue parameters.
>>>>
>>>> There is one flip work queue for each hardware CRTC. At most one
>>>> radeon_flip_work_func item can be queued for any of them at any time.
>>>> When a radeon_flip_work_func item is queued, it should be executed ASAP
>>>> (so WQ_HIGHPRI might be appropriate?).
>>> Hmmm... the only time WQ_HIGHPRI should be used is when it'd otherwise
>>> require a kthread w/ nice value at -20.  Would that be the case here?
>>> What are the consequences of the work item getting delayed?
>> A page flip may be delayed to a later display refresh cycle.
>>
>>
>>> Also, what kind of delays matter here?  Is it millisec range or micro?
>> It can be the latter in theory, but normally rather the former.
> 
> Well to be precise with a typical 1920x1080@60 resolution you have about
> 2.16ms time under ideal conditions for the flip.
> 
> So using the high priority queue still sounds like a good idea to me.

How did you arrive at 2.16ms?

Userspace can call the ioctl up to one full refresh cycle ahead of time,
which is ~16ms at 60 Hz. On the other hand userspace can also call the
ioctl arbitrarily close to the vertical blank period, in which case even
a delay of just 1ms (or even significantly less) may cause the flip to
be delayed by one refresh cycle.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Tejun Heo]

Hello,

On Fri, Jul 08, 2016 at 02:52:30PM +0900, Michel Dänzer wrote:
> On 07.07.2016 16:43, Christian König wrote:
> >>> Also, what kind of delays matter here?  Is it millisec range or micro?
> >> It can be the latter in theory, but normally rather the former.
> > 
> > Well to be precise with a typical 1920x1080@60 resolution you have about
> > 2.16ms time under ideal conditions for the flip.
> > 
> > So using the high priority queue still sounds like a good idea to me.
> 
> How did you arrive at 2.16ms?
> 
> Userspace can call the ioctl up to one full refresh cycle ahead of time,
> which is ~16ms at 60 Hz. On the other hand userspace can also call the
> ioctl arbitrarily close to the vertical blank period, in which case even
> a delay of just 1ms (or even significantly less) may cause the flip to
> be delayed by one refresh cycle.

If there's too long a delay, the outcome is missing the refresh cycle,
right?  Hmmm... yeah, WQ_HIGHPRI probably is the right answer here.
Bhaktipriya, can you please update the patch to use a dedicated
workqueue with WQ_HIGHPRI?

Thanks.

-- 
tejun

[PATCH v2] drm/radeon: Remove deprecated create_singlethread_workqueue

[Bhaktipriya Shridhar]

alloc_workqueue replaces deprecated create_singlethread_workqueue().

Each hardware CRTC has a single flip work queue.
When a radeon_flip_work_func item is queued, it needs to be executed
ASAP because even a slight delay may cause the flip to be delayed by
one refresh cycle.

Hence, a dedicated workqueue with WQ_HIGHPRI set, has been used here
since a delay can cause the outcome to miss the refresh cycle.

Since there are only a fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 Changes in v2:
	-Used a dedicated work queue with WQ_HIGHPRI

 drivers/gpu/drm/radeon/radeon_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 6a41b49..64b246e 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -711,7 +711,7 @@ static void radeon_crtc_init(struct drm_device *dev, int index)

 	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
 	radeon_crtc->crtc_id = index;
-	radeon_crtc->flip_queue = create_singlethread_workqueue("radeon-crtc");
+	radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc", WQ_HIGHPRI, 0);
 	rdev->mode_info.crtcs[index] = radeon_crtc;

 	if (rdev->family >= CHIP_BONAIRE) {
--
2.1.4

Re: [PATCH v2] drm/radeon: Remove deprecated create_singlethread_workqueue

[Christian König]

Am 16.07.2016 um 13:30 schrieb Bhaktipriya Shridhar:
> alloc_workqueue replaces deprecated create_singlethread_workqueue().
>
> Each hardware CRTC has a single flip work queue.
> When a radeon_flip_work_func item is queued, it needs to be executed
> ASAP because even a slight delay may cause the flip to be delayed by
> one refresh cycle.
>
> Hence, a dedicated workqueue with WQ_HIGHPRI set, has been used here
> since a delay can cause the outcome to miss the refresh cycle.
>
> Since there are only a fixed number of work items, explicit concurrency
> limit is unnecessary here.
>
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   Changes in v2:
> 	-Used a dedicated work queue with WQ_HIGHPRI
>
>   drivers/gpu/drm/radeon/radeon_display.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 6a41b49..64b246e 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -711,7 +711,7 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
>
>   	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
>   	radeon_crtc->crtc_id = index;
> -	radeon_crtc->flip_queue = create_singlethread_workqueue("radeon-crtc");
> +	radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc", WQ_HIGHPRI, 0);
>   	rdev->mode_info.crtcs[index] = radeon_crtc;
>
>   	if (rdev->family >= CHIP_BONAIRE) {
> --
> 2.1.4
>

Re: [PATCH v2] drm/radeon: Remove deprecated create_singlethread_workqueue

[Tejun Heo]

On Sat, Jul 16, 2016 at 05:00:44PM +0530, Bhaktipriya Shridhar wrote:
> 
> alloc_workqueue replaces deprecated create_singlethread_workqueue().
> 
> Each hardware CRTC has a single flip work queue.
> When a radeon_flip_work_func item is queued, it needs to be executed
> ASAP because even a slight delay may cause the flip to be delayed by
> one refresh cycle.
> 
> Hence, a dedicated workqueue with WQ_HIGHPRI set, has been used here
> since a delay can cause the outcome to miss the refresh cycle.
> 
> Since there are only a fixed number of work items, explicit concurrency
> limit is unnecessary here.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH v2] drm/radeon: Remove deprecated create_singlethread_workqueue

[Alex Deucher]

On Mon, Jul 18, 2016 at 8:48 PM, Tejun Heo <tj@kernel.org> wrote:
> On Sat, Jul 16, 2016 at 05:00:44PM +0530, Bhaktipriya Shridhar wrote:
>>
>> alloc_workqueue replaces deprecated create_singlethread_workqueue().
>>
>> Each hardware CRTC has a single flip work queue.
>> When a radeon_flip_work_func item is queued, it needs to be executed
>> ASAP because even a slight delay may cause the flip to be delayed by
>> one refresh cycle.
>>
>> Hence, a dedicated workqueue with WQ_HIGHPRI set, has been used here
>> since a delay can cause the outcome to miss the refresh cycle.
>>
>> Since there are only a fixed number of work items, explicit concurrency
>> limit is unnecessary here.
>>
>> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
>
> Acked-by: Tejun Heo <tj@kernel.org>
>

Applied.  thanks!

Alex

> Thanks.
>
> --
> tejun
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Bhaktipriya Shridhar]

alloc_workqueue replaces deprecated create_singlethread_workqueue().

A dedicated workqueue has been used since work items need to be flushed
as a group rather than individually.

Since the flip_queue workqueue is involved in page-flipping and is not
being used on a memory reclaim path, WQ_MEM_RECLAIM has not been set.

Since there are only a fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 drivers/gpu/drm/radeon/radeon_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 6a41b49..bbb29c7 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -711,7 +711,7 @@ static void radeon_crtc_init(struct drm_device *dev, int index)

 	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
 	radeon_crtc->crtc_id = index;
-	radeon_crtc->flip_queue = create_singlethread_workqueue("radeon-crtc");
+	radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc", 0, 0);
 	rdev->mode_info.crtcs[index] = radeon_crtc;

 	if (rdev->family >= CHIP_BONAIRE) {
--
2.1.4

Re: [PATCH] drm/radeon: Remove deprecated create_singlethread_workqueue

[Bhaktipriya Shridhar]

Please ignore this mail.
Thanks,
Bhaktipriya


On Tue, Jun 28, 2016 at 10:56 PM, Bhaktipriya Shridhar
<bhaktipriya96@gmail.com> wrote:
> alloc_workqueue replaces deprecated create_singlethread_workqueue().
>
> A dedicated workqueue has been used since work items need to be flushed
> as a group rather than individually.
>
> Since the flip_queue workqueue is involved in page-flipping and is not
> being used on a memory reclaim path, WQ_MEM_RECLAIM has not been set.
>
> Since there are only a fixed number of work items, explicit concurrency
> limit is unnecessary here.
>
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
> ---
>  drivers/gpu/drm/radeon/radeon_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 6a41b49..bbb29c7 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -711,7 +711,7 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
>
>         drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
>         radeon_crtc->crtc_id = index;
> -       radeon_crtc->flip_queue = create_singlethread_workqueue("radeon-crtc");
> +       radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc", 0, 0);
>         rdev->mode_info.crtcs[index] = radeon_crtc;
>
>         if (rdev->family >= CHIP_BONAIRE) {
> --
> 2.1.4
>