[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
Am Mittwoch, 13. Januar 2016, 12:53:34 schrieb John Keeping: > As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies > on cursor ioctls being unsynced. Converting the rockchip driver to > atomic has significantly impacted cursor performance by making every > cursor update wait for vblank. > > By skipping the vblank sync when the framebuffer has not changed (as is > done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the > common case of moving the cursor and only need to delay the cursor ioctl > when the cursor icon changes. > > I originally inserted a check on legacy_cursor_update as well, but that > caused a storm of iommu page faults. I didn't investigate the cause of > those since this change gives enough of a performance improvement for my > use case. > > This is RFC because of that and because the framebuffer_changed() > function is copied from drm_atomic_helper.c as a quick way to test the > result. > > Signed-off-by: John Keeping I've seen the effects now as well after making the atomic parts work on in my devtree - i.e. sluggish cursor movements. This patch fixes that issue, so at least: Tested-by: Heiko Stuebner Right now I still see flickering on animated cursors though (like ones used by KDE), that wasn't present before. Heiko
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On 2016å¹´01æ14æ¥ 16:32, Daniel Vetter wrote:
> On Thu, Jan 14, 2016 at 2:16 AM, Mark yao wrote:
>> On 2016å¹´01æ14æ¥ 01:39, John Keeping wrote:
>>> On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote:
>>>
On Wed, Jan 13, 2016 at 04:40:38PM +, John Keeping wrote:
> On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
>
>> On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
>>> On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
>>>
On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
>
>> On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
>>> As commented in drm_atomic_helper_wait_for_vblanks(), userspace
>>> relies on cursor ioctls being unsynced. Converting the rockchip
>>> driver to atomic has significantly impacted cursor performance by
>>> making every cursor update wait for vblank.
>>>
>>> By skipping the vblank sync when the framebuffer has not changed
>>> (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
>>> this for the common case of moving the cursor and only need to
>>> delay the cursor ioctl when the cursor icon changes.
>>>
>>> I originally inserted a check on legacy_cursor_update as well, but
>>> that caused a storm of iommu page faults. I didn't investigate
>>> the
>>> cause of those since this change gives enough of a performance
>>> improvement for my use case.
>>>
>>> This is RFC because of that and because the framebuffer_changed()
>>> function is copied from drm_atomic_helper.c as a quick way to test
>>> the result.
>>>
>>> Signed-off-by: John Keeping
>>> ---
>>>drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
>>> +-- 1 file changed, 25 insertions(+), 2
>>> deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index
>>> f784488..8fd9821
>>> 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> @@ -177,8 +177,28 @@ static void
>>> rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
>>> crtc_funcs->wait_for_update(crtc); }
>>>+static bool framebuffer_changed(struct drm_device *dev,
>>> + struct drm_atomic_state
>>> *old_state,
>>> + struct drm_crtc *crtc)
>>> +{
>>> + struct drm_plane *plane;
>>> + struct drm_plane_state *old_plane_state;
>>> + int i;
>>> +
>>> + for_each_plane_in_state(old_state, plane, old_plane_state,
>>> i) {
>>> + if (plane->state->crtc != crtc &&
>>> + old_plane_state->crtc != crtc)
>>> + continue;
>>> +
>>> + if (plane->state->fb != old_plane_state->fb)
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>> Please don't hand-roll logic that affects semantics like this.
>> Instead
>> please use drm_atomic_helper_wait_for_vblanks(), which should do
>> this
>> correctly for you.
>>
>> If that's not the case then we need to improve the generic helper,
>> or
>> figure out what's different with rockhip.
> According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> API) it's because rockchip doesn't have a hardware vblank counter.
>
> I'm not entirely clear on why this prevents the use of
> drm_atomic_helper_wait_for_vblanks().
Hm, that commit isn't terribly helpful. If that's really needed then
imo I
think we should extract a
"drm_atomic_helper_plane_needs_vblank_wait()"
helper that's used by both. But since rockchip does vblank_get/put
calls
I'd hope vblanks actually work correctly. And then the helper should
work
too.
>>> I tried switching the call to rockchip_crtc_wait_for_update() to
>>> drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
>>> the buffer associated with a cursor, at which point I get iommu page
>>> faults, presumably because the GEM buffer is unreferenced too early.
>>>
>>> AFAICT the buffer will be released via drm_atomic_state_free()
>>> unconditionally, but I suspect I'm missing something since that would
>>> mean every driver would hit a similar problem.
>> Yeah, with the helper we always skip, which means when the cursor bo
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Thu, Jan 14, 2016 at 04:46:37PM +0800, Mark yao wrote:
> On 2016å¹´01æ14æ¥ 16:32, Daniel Vetter wrote:
> >On Thu, Jan 14, 2016 at 2:16 AM, Mark yao wrote:
> >>On 2016å¹´01æ14æ¥ 01:39, John Keeping wrote:
> >>>On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote:
> >>>
> On Wed, Jan 13, 2016 at 04:40:38PM +, John Keeping wrote:
> >On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
> >
> >>On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
> >>>On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
> >>>
> On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> >On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> >
> >>On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> >>>As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> >>>relies on cursor ioctls being unsynced. Converting the rockchip
> >>>driver to atomic has significantly impacted cursor performance by
> >>>making every cursor update wait for vblank.
> >>>
> >>>By skipping the vblank sync when the framebuffer has not changed
> >>>(as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> >>>this for the common case of moving the cursor and only need to
> >>>delay the cursor ioctl when the cursor icon changes.
> >>>
> >>>I originally inserted a check on legacy_cursor_update as well, but
> >>>that caused a storm of iommu page faults. I didn't investigate
> >>>the
> >>>cause of those since this change gives enough of a performance
> >>>improvement for my use case.
> >>>
> >>>This is RFC because of that and because the framebuffer_changed()
> >>>function is copied from drm_atomic_helper.c as a quick way to test
> >>>the result.
> >>>
> >>>Signed-off-by: John Keeping
> >>>---
> >>> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> >>>+-- 1 file changed, 25 insertions(+), 2
> >>>deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >>>b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index
> >>>f784488..8fd9821
> >>>100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >>>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >>>@@ -177,8 +177,28 @@ static void
> >>>rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> >>>crtc_funcs->wait_for_update(crtc); }
> >>> +static bool framebuffer_changed(struct drm_device *dev,
> >>>+ struct drm_atomic_state
> >>>*old_state,
> >>>+ struct drm_crtc *crtc)
> >>>+{
> >>>+ struct drm_plane *plane;
> >>>+ struct drm_plane_state *old_plane_state;
> >>>+ int i;
> >>>+
> >>>+ for_each_plane_in_state(old_state, plane, old_plane_state,
> >>>i) {
> >>>+ if (plane->state->crtc != crtc &&
> >>>+ old_plane_state->crtc != crtc)
> >>>+ continue;
> >>>+
> >>>+ if (plane->state->fb != old_plane_state->fb)
> >>>+ return true;
> >>>+ }
> >>>+
> >>>+ return false;
> >>>+}
> >>Please don't hand-roll logic that affects semantics like this.
> >>Instead
> >>please use drm_atomic_helper_wait_for_vblanks(), which should do
> >>this
> >>correctly for you.
> >>
> >>If that's not the case then we need to improve the generic helper,
> >>or
> >>figure out what's different with rockhip.
> >According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> >API) it's because rockchip doesn't have a hardware vblank counter.
> >
> >I'm not entirely clear on why this prevents the use of
> >drm_atomic_helper_wait_for_vblanks().
> Hm, that commit isn't terribly helpful. If that's really needed then
> imo I
> think we should extract a
> "drm_atomic_helper_plane_needs_vblank_wait()"
> helper that's used by both. But since rockchip does vblank_get/put
> calls
> I'd hope vblanks actually work correctly. And then the helper should
> work
> too.
> >>>I tried switching the call to rockchip_crtc_wait_for_update() to
> >>>drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
> >>>the buffer associated with a cursor, at which point I get iommu page
> >>>faults, presumably because the GEM buffer is unreferenced too early.
> >>>
> >>>AFAICT the buffer will be released via drm_atomic_state_free()
> >>>unconditionally, but I
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Thu, Jan 14, 2016 at 2:16 AM, Mark yao wrote:
> On 2016å¹´01æ14æ¥ 01:39, John Keeping wrote:
>>
>> On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote:
>>
>>> On Wed, Jan 13, 2016 at 04:40:38PM +, John Keeping wrote:
On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
>
> On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
>>
>> On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
>>
>>>
>>> On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
>
> On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
>>
>> As commented in drm_atomic_helper_wait_for_vblanks(), userspace
>> relies on cursor ioctls being unsynced. Converting the rockchip
>> driver to atomic has significantly impacted cursor performance by
>> making every cursor update wait for vblank.
>>
>> By skipping the vblank sync when the framebuffer has not changed
>> (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
>> this for the common case of moving the cursor and only need to
>> delay the cursor ioctl when the cursor icon changes.
>>
>> I originally inserted a check on legacy_cursor_update as well, but
>> that caused a storm of iommu page faults. I didn't investigate
>> the
>> cause of those since this change gives enough of a performance
>> improvement for my use case.
>>
>> This is RFC because of that and because the framebuffer_changed()
>> function is copied from drm_atomic_helper.c as a quick way to test
>> the result.
>>
>> Signed-off-by: John Keeping
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
>> +-- 1 file changed, 25 insertions(+), 2
>> deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index
>> f784488..8fd9821
>> 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> @@ -177,8 +177,28 @@ static void
>> rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
>> crtc_funcs->wait_for_update(crtc); }
>> +static bool framebuffer_changed(struct drm_device *dev,
>> + struct drm_atomic_state
>> *old_state,
>> + struct drm_crtc *crtc)
>> +{
>> + struct drm_plane *plane;
>> + struct drm_plane_state *old_plane_state;
>> + int i;
>> +
>> + for_each_plane_in_state(old_state, plane, old_plane_state,
>> i) {
>> + if (plane->state->crtc != crtc &&
>> + old_plane_state->crtc != crtc)
>> + continue;
>> +
>> + if (plane->state->fb != old_plane_state->fb)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>
> Please don't hand-roll logic that affects semantics like this.
> Instead
> please use drm_atomic_helper_wait_for_vblanks(), which should do
> this
> correctly for you.
>
> If that's not the case then we need to improve the generic helper,
> or
> figure out what's different with rockhip.
According to commit 63ebb9f (drm/rockchip: Convert to support atomic
API) it's because rockchip doesn't have a hardware vblank counter.
I'm not entirely clear on why this prevents the use of
drm_atomic_helper_wait_for_vblanks().
>>>
>>> Hm, that commit isn't terribly helpful. If that's really needed then
>>> imo I
>>> think we should extract a
>>> "drm_atomic_helper_plane_needs_vblank_wait()"
>>> helper that's used by both. But since rockchip does vblank_get/put
>>> calls
>>> I'd hope vblanks actually work correctly. And then the helper should
>>> work
>>> too.
>>
>> I tried switching the call to rockchip_crtc_wait_for_update() to
>> drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
>> the buffer associated with a cursor, at which point I get iommu page
>> faults, presumably because the GEM buffer is unreferenced too early.
>>
>> AFAICT the buffer will be released via drm_atomic_state_free()
>> unconditionally, but I suspect I'm missing something since that would
>> mean every driver would hit a similar problem.
>
> Yeah, with the helper we always skip, which means when the cursor bo
> changes you indeed unmap too early. So can't
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On 2016å¹´01æ14æ¥ 01:39, John Keeping wrote:
> On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote:
>
>> On Wed, Jan 13, 2016 at 04:40:38PM +, John Keeping wrote:
>>> On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
>>>
On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
> On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
>
>> On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
>>> On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
>>>
On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> relies on cursor ioctls being unsynced. Converting the rockchip
> driver to atomic has significantly impacted cursor performance by
> making every cursor update wait for vblank.
>
> By skipping the vblank sync when the framebuffer has not changed
> (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> this for the common case of moving the cursor and only need to
> delay the cursor ioctl when the cursor icon changes.
>
> I originally inserted a check on legacy_cursor_update as well, but
> that caused a storm of iommu page faults. I didn't investigate the
> cause of those since this change gives enough of a performance
> improvement for my use case.
>
> This is RFC because of that and because the framebuffer_changed()
> function is copied from drm_atomic_helper.c as a quick way to test
> the result.
>
> Signed-off-by: John Keeping
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> +-- 1 file changed, 25 insertions(+), 2
> deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
> 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -177,8 +177,28 @@ static void
> rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> crtc_funcs->wait_for_update(crtc); }
>
> +static bool framebuffer_changed(struct drm_device *dev,
> + struct drm_atomic_state *old_state,
> + struct drm_crtc *crtc)
> +{
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state;
> + int i;
> +
> + for_each_plane_in_state(old_state, plane, old_plane_state,
> i) {
> + if (plane->state->crtc != crtc &&
> + old_plane_state->crtc != crtc)
> + continue;
> +
> + if (plane->state->fb != old_plane_state->fb)
> + return true;
> + }
> +
> + return false;
> +}
Please don't hand-roll logic that affects semantics like this. Instead
please use drm_atomic_helper_wait_for_vblanks(), which should do this
correctly for you.
If that's not the case then we need to improve the generic helper, or
figure out what's different with rockhip.
>>> According to commit 63ebb9f (drm/rockchip: Convert to support atomic
>>> API) it's because rockchip doesn't have a hardware vblank counter.
>>>
>>> I'm not entirely clear on why this prevents the use of
>>> drm_atomic_helper_wait_for_vblanks().
>> Hm, that commit isn't terribly helpful. If that's really needed then imo
>> I
>> think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
>> helper that's used by both. But since rockchip does vblank_get/put calls
>> I'd hope vblanks actually work correctly. And then the helper should work
>> too.
> I tried switching the call to rockchip_crtc_wait_for_update() to
> drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
> the buffer associated with a cursor, at which point I get iommu page
> faults, presumably because the GEM buffer is unreferenced too early.
>
> AFAICT the buffer will be released via drm_atomic_state_free()
> unconditionally, but I suspect I'm missing something since that would
> mean every driver would hit a similar problem.
Yeah, with the helper we always skip, which means when the cursor bo
changes you indeed unmap too early. So can't even share the overall
condition, but we could definitely share the little framebuffer_changed
helper.
>>> That leaves me with the question: why do other atomic drivers work?
>>>
>>> If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the
>>> cursor bo being unmapped too early for rockchip, why is it
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, Jan 13, 2016 at 04:40:38PM +, John Keeping wrote:
> On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
>
> > On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
> > > On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
> > >
> > > > On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> > > > > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> > > > >
> > > > > > On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > > > > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > > > > > > relies on cursor ioctls being unsynced. Converting the rockchip
> > > > > > > driver to atomic has significantly impacted cursor performance by
> > > > > > > making every cursor update wait for vblank.
> > > > > > >
> > > > > > > By skipping the vblank sync when the framebuffer has not changed
> > > > > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> > > > > > > this for the common case of moving the cursor and only need to
> > > > > > > delay the cursor ioctl when the cursor icon changes.
> > > > > > >
> > > > > > > I originally inserted a check on legacy_cursor_update as well, but
> > > > > > > that caused a storm of iommu page faults. I didn't investigate
> > > > > > > the
> > > > > > > cause of those since this change gives enough of a performance
> > > > > > > improvement for my use case.
> > > > > > >
> > > > > > > This is RFC because of that and because the framebuffer_changed()
> > > > > > > function is copied from drm_atomic_helper.c as a quick way to test
> > > > > > > the result.
> > > > > > >
> > > > > > > Signed-off-by: John Keeping
> > > > > > > ---
> > > > > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > > > > > > +-- 1 file changed, 25 insertions(+), 2
> > > > > > > deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index
> > > > > > > f784488..8fd9821
> > > > > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > > @@ -177,8 +177,28 @@ static void
> > > > > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > > > > > > crtc_funcs->wait_for_update(crtc); }
> > > > > > >
> > > > > > > +static bool framebuffer_changed(struct drm_device *dev,
> > > > > > > + struct drm_atomic_state *old_state,
> > > > > > > + struct drm_crtc *crtc)
> > > > > > > +{
> > > > > > > + struct drm_plane *plane;
> > > > > > > + struct drm_plane_state *old_plane_state;
> > > > > > > + int i;
> > > > > > > +
> > > > > > > + for_each_plane_in_state(old_state, plane, old_plane_state,
> > > > > > > i) {
> > > > > > > + if (plane->state->crtc != crtc &&
> > > > > > > + old_plane_state->crtc != crtc)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + if (plane->state->fb != old_plane_state->fb)
> > > > > > > + return true;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return false;
> > > > > > > +}
> > > > > >
> > > > > > Please don't hand-roll logic that affects semantics like this.
> > > > > > Instead
> > > > > > please use drm_atomic_helper_wait_for_vblanks(), which should do
> > > > > > this
> > > > > > correctly for you.
> > > > > >
> > > > > > If that's not the case then we need to improve the generic helper,
> > > > > > or
> > > > > > figure out what's different with rockhip.
> > > > >
> > > > > According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> > > > > API) it's because rockchip doesn't have a hardware vblank counter.
> > > > >
> > > > > I'm not entirely clear on why this prevents the use of
> > > > > drm_atomic_helper_wait_for_vblanks().
> > > >
> > > > Hm, that commit isn't terribly helpful. If that's really needed then
> > > > imo I
> > > > think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
> > > > helper that's used by both. But since rockchip does vblank_get/put calls
> > > > I'd hope vblanks actually work correctly. And then the helper should
> > > > work
> > > > too.
> > >
> > > I tried switching the call to rockchip_crtc_wait_for_update() to
> > > drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
> > > the buffer associated with a cursor, at which point I get iommu page
> > > faults, presumably because the GEM buffer is unreferenced too early.
> > >
> > > AFAICT the buffer will be released via drm_atomic_state_free()
> > > unconditionally, but I suspect I'm missing something since that would
> > > mean every driver would hit a similar problem.
> >
> > Yeah, with the helper we always skip, which means when the cursor bo
> > changes you indeed unmap too early. So can't even share the overall
> > condition, but we could definitely share the little framebuffer_changed
>
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote:
> On Wed, Jan 13, 2016 at 04:40:38PM +, John Keeping wrote:
> > On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
> >
> > > On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
> > > > On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
> > > >
> > > > > On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> > > > > > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> > > > > >
> > > > > > > On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > > > > > >
> > > > > > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > > > > > > > relies on cursor ioctls being unsynced. Converting the rockchip
> > > > > > > > driver to atomic has significantly impacted cursor performance
> > > > > > > > by
> > > > > > > > making every cursor update wait for vblank.
> > > > > > > >
> > > > > > > > By skipping the vblank sync when the framebuffer has not changed
> > > > > > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can
> > > > > > > > avoid
> > > > > > > > this for the common case of moving the cursor and only need to
> > > > > > > > delay the cursor ioctl when the cursor icon changes.
> > > > > > > >
> > > > > > > > I originally inserted a check on legacy_cursor_update as well,
> > > > > > > > but
> > > > > > > > that caused a storm of iommu page faults. I didn't investigate
> > > > > > > > the
> > > > > > > > cause of those since this change gives enough of a performance
> > > > > > > > improvement for my use case.
> > > > > > > >
> > > > > > > > This is RFC because of that and because the
> > > > > > > > framebuffer_changed()
> > > > > > > > function is copied from drm_atomic_helper.c as a quick way to
> > > > > > > > test
> > > > > > > > the result.
> > > > > > > >
> > > > > > > > Signed-off-by: John Keeping
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > > > > > > > +-- 1 file changed, 25 insertions(+), 2
> > > > > > > > deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index
> > > > > > > > f784488..8fd9821
> > > > > > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > > > @@ -177,8 +177,28 @@ static void
> > > > > > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > > > > > > > crtc_funcs->wait_for_update(crtc); }
> > > > > > > >
> > > > > > > > +static bool framebuffer_changed(struct drm_device *dev,
> > > > > > > > + struct drm_atomic_state
> > > > > > > > *old_state,
> > > > > > > > + struct drm_crtc *crtc)
> > > > > > > > +{
> > > > > > > > + struct drm_plane *plane;
> > > > > > > > + struct drm_plane_state *old_plane_state;
> > > > > > > > + int i;
> > > > > > > > +
> > > > > > > > + for_each_plane_in_state(old_state, plane,
> > > > > > > > old_plane_state,
> > > > > > > > i) {
> > > > > > > > + if (plane->state->crtc != crtc &&
> > > > > > > > + old_plane_state->crtc != crtc)
> > > > > > > > + continue;
> > > > > > > > +
> > > > > > > > + if (plane->state->fb != old_plane_state->fb)
> > > > > > > > + return true;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + return false;
> > > > > > > > +}
> > > > > > >
> > > > > > > Please don't hand-roll logic that affects semantics like this.
> > > > > > > Instead
> > > > > > > please use drm_atomic_helper_wait_for_vblanks(), which should do
> > > > > > > this
> > > > > > > correctly for you.
> > > > > > >
> > > > > > > If that's not the case then we need to improve the generic
> > > > > > > helper, or
> > > > > > > figure out what's different with rockhip.
> > > > > >
> > > > > > According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> > > > > > API) it's because rockchip doesn't have a hardware vblank counter.
> > > > > >
> > > > > > I'm not entirely clear on why this prevents the use of
> > > > > > drm_atomic_helper_wait_for_vblanks().
> > > > >
> > > > > Hm, that commit isn't terribly helpful. If that's really needed then
> > > > > imo I
> > > > > think we should extract a
> > > > > "drm_atomic_helper_plane_needs_vblank_wait()"
> > > > > helper that's used by both. But since rockchip does vblank_get/put
> > > > > calls
> > > > > I'd hope vblanks actually work correctly. And then the helper should
> > > > > work
> > > > > too.
> > > >
> > > > I tried switching the call to rockchip_crtc_wait_for_update() to
> > > > drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
> > > > the buffer associated with a cursor, at which point I get iommu page
> > > >
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
> On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
>
> > On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> > > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> > >
> > > > On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > > > > relies on cursor ioctls being unsynced. Converting the rockchip
> > > > > driver to atomic has significantly impacted cursor performance by
> > > > > making every cursor update wait for vblank.
> > > > >
> > > > > By skipping the vblank sync when the framebuffer has not changed
> > > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> > > > > this for the common case of moving the cursor and only need to
> > > > > delay the cursor ioctl when the cursor icon changes.
> > > > >
> > > > > I originally inserted a check on legacy_cursor_update as well, but
> > > > > that caused a storm of iommu page faults. I didn't investigate the
> > > > > cause of those since this change gives enough of a performance
> > > > > improvement for my use case.
> > > > >
> > > > > This is RFC because of that and because the framebuffer_changed()
> > > > > function is copied from drm_atomic_helper.c as a quick way to test
> > > > > the result.
> > > > >
> > > > > Signed-off-by: John Keeping
> > > > > ---
> > > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > > > > +-- 1 file changed, 25 insertions(+), 2
> > > > > deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
> > > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > @@ -177,8 +177,28 @@ static void
> > > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > > > > crtc_funcs->wait_for_update(crtc); }
> > > > >
> > > > > +static bool framebuffer_changed(struct drm_device *dev,
> > > > > + struct drm_atomic_state *old_state,
> > > > > + struct drm_crtc *crtc)
> > > > > +{
> > > > > + struct drm_plane *plane;
> > > > > + struct drm_plane_state *old_plane_state;
> > > > > + int i;
> > > > > +
> > > > > + for_each_plane_in_state(old_state, plane, old_plane_state,
> > > > > i) {
> > > > > + if (plane->state->crtc != crtc &&
> > > > > + old_plane_state->crtc != crtc)
> > > > > + continue;
> > > > > +
> > > > > + if (plane->state->fb != old_plane_state->fb)
> > > > > + return true;
> > > > > + }
> > > > > +
> > > > > + return false;
> > > > > +}
> > > >
> > > > Please don't hand-roll logic that affects semantics like this. Instead
> > > > please use drm_atomic_helper_wait_for_vblanks(), which should do this
> > > > correctly for you.
> > > >
> > > > If that's not the case then we need to improve the generic helper, or
> > > > figure out what's different with rockhip.
> > >
> > > According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> > > API) it's because rockchip doesn't have a hardware vblank counter.
> > >
> > > I'm not entirely clear on why this prevents the use of
> > > drm_atomic_helper_wait_for_vblanks().
> >
> > Hm, that commit isn't terribly helpful. If that's really needed then imo I
> > think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
> > helper that's used by both. But since rockchip does vblank_get/put calls
> > I'd hope vblanks actually work correctly. And then the helper should work
> > too.
>
> I tried switching the call to rockchip_crtc_wait_for_update() to
> drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
> the buffer associated with a cursor, at which point I get iommu page
> faults, presumably because the GEM buffer is unreferenced too early.
>
> AFAICT the buffer will be released via drm_atomic_state_free()
> unconditionally, but I suspect I'm missing something since that would
> mean every driver would hit a similar problem.
Yeah, with the helper we always skip, which means when the cursor bo
changes you indeed unmap too early. So can't even share the overall
condition, but we could definitely share the little framebuffer_changed
helper. Plus rockchip_crtc_wait_for_update should have a big comment
explaining why we have different rules than core helpers!
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote:
> On Wed, Jan 13, 2016 at 03:55:29PM +, John Keeping wrote:
> > On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
> >
> > > On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> > > > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> > > >
> > > > > On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > > > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > > > > > relies on cursor ioctls being unsynced. Converting the rockchip
> > > > > > driver to atomic has significantly impacted cursor performance by
> > > > > > making every cursor update wait for vblank.
> > > > > >
> > > > > > By skipping the vblank sync when the framebuffer has not changed
> > > > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> > > > > > this for the common case of moving the cursor and only need to
> > > > > > delay the cursor ioctl when the cursor icon changes.
> > > > > >
> > > > > > I originally inserted a check on legacy_cursor_update as well, but
> > > > > > that caused a storm of iommu page faults. I didn't investigate the
> > > > > > cause of those since this change gives enough of a performance
> > > > > > improvement for my use case.
> > > > > >
> > > > > > This is RFC because of that and because the framebuffer_changed()
> > > > > > function is copied from drm_atomic_helper.c as a quick way to test
> > > > > > the result.
> > > > > >
> > > > > > Signed-off-by: John Keeping
> > > > > > ---
> > > > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > > > > > +-- 1 file changed, 25 insertions(+), 2
> > > > > > deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
> > > > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > > > @@ -177,8 +177,28 @@ static void
> > > > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > > > > > crtc_funcs->wait_for_update(crtc); }
> > > > > >
> > > > > > +static bool framebuffer_changed(struct drm_device *dev,
> > > > > > + struct drm_atomic_state *old_state,
> > > > > > + struct drm_crtc *crtc)
> > > > > > +{
> > > > > > + struct drm_plane *plane;
> > > > > > + struct drm_plane_state *old_plane_state;
> > > > > > + int i;
> > > > > > +
> > > > > > + for_each_plane_in_state(old_state, plane, old_plane_state,
> > > > > > i) {
> > > > > > + if (plane->state->crtc != crtc &&
> > > > > > + old_plane_state->crtc != crtc)
> > > > > > + continue;
> > > > > > +
> > > > > > + if (plane->state->fb != old_plane_state->fb)
> > > > > > + return true;
> > > > > > + }
> > > > > > +
> > > > > > + return false;
> > > > > > +}
> > > > >
> > > > > Please don't hand-roll logic that affects semantics like this. Instead
> > > > > please use drm_atomic_helper_wait_for_vblanks(), which should do this
> > > > > correctly for you.
> > > > >
> > > > > If that's not the case then we need to improve the generic helper, or
> > > > > figure out what's different with rockhip.
> > > >
> > > > According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> > > > API) it's because rockchip doesn't have a hardware vblank counter.
> > > >
> > > > I'm not entirely clear on why this prevents the use of
> > > > drm_atomic_helper_wait_for_vblanks().
> > >
> > > Hm, that commit isn't terribly helpful. If that's really needed then imo I
> > > think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
> > > helper that's used by both. But since rockchip does vblank_get/put calls
> > > I'd hope vblanks actually work correctly. And then the helper should work
> > > too.
> >
> > I tried switching the call to rockchip_crtc_wait_for_update() to
> > drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
> > the buffer associated with a cursor, at which point I get iommu page
> > faults, presumably because the GEM buffer is unreferenced too early.
> >
> > AFAICT the buffer will be released via drm_atomic_state_free()
> > unconditionally, but I suspect I'm missing something since that would
> > mean every driver would hit a similar problem.
>
> Yeah, with the helper we always skip, which means when the cursor bo
> changes you indeed unmap too early. So can't even share the overall
> condition, but we could definitely share the little framebuffer_changed
> helper.
That leaves me with the question: why do other atomic drivers work?
If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the
cursor bo being unmapped too early for rockchip, why is it not unmapped
too early for all of the other drivers using that helper?
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
>
> > On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > > relies on cursor ioctls being unsynced. Converting the rockchip
> > > driver to atomic has significantly impacted cursor performance by
> > > making every cursor update wait for vblank.
> > >
> > > By skipping the vblank sync when the framebuffer has not changed
> > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> > > this for the common case of moving the cursor and only need to
> > > delay the cursor ioctl when the cursor icon changes.
> > >
> > > I originally inserted a check on legacy_cursor_update as well, but
> > > that caused a storm of iommu page faults. I didn't investigate the
> > > cause of those since this change gives enough of a performance
> > > improvement for my use case.
> > >
> > > This is RFC because of that and because the framebuffer_changed()
> > > function is copied from drm_atomic_helper.c as a quick way to test
> > > the result.
> > >
> > > Signed-off-by: John Keeping
> > > ---
> > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > > +-- 1 file changed, 25 insertions(+), 2
> > > deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
> > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > @@ -177,8 +177,28 @@ static void
> > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > > crtc_funcs->wait_for_update(crtc); }
> > >
> > > +static bool framebuffer_changed(struct drm_device *dev,
> > > + struct drm_atomic_state *old_state,
> > > + struct drm_crtc *crtc)
> > > +{
> > > + struct drm_plane *plane;
> > > + struct drm_plane_state *old_plane_state;
> > > + int i;
> > > +
> > > + for_each_plane_in_state(old_state, plane, old_plane_state,
> > > i) {
> > > + if (plane->state->crtc != crtc &&
> > > + old_plane_state->crtc != crtc)
> > > + continue;
> > > +
> > > + if (plane->state->fb != old_plane_state->fb)
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > +}
> >
> > Please don't hand-roll logic that affects semantics like this. Instead
> > please use drm_atomic_helper_wait_for_vblanks(), which should do this
> > correctly for you.
> >
> > If that's not the case then we need to improve the generic helper, or
> > figure out what's different with rockhip.
>
> According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> API) it's because rockchip doesn't have a hardware vblank counter.
>
> I'm not entirely clear on why this prevents the use of
> drm_atomic_helper_wait_for_vblanks().
Hm, that commit isn't terribly helpful. If that's really needed then imo I
think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
helper that's used by both. But since rockchip does vblank_get/put calls
I'd hope vblanks actually work correctly. And then the helper should work
too.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote:
> On Wed, Jan 13, 2016 at 02:34:25PM +, John Keeping wrote:
> > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> >
> > > On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > > > relies on cursor ioctls being unsynced. Converting the rockchip
> > > > driver to atomic has significantly impacted cursor performance by
> > > > making every cursor update wait for vblank.
> > > >
> > > > By skipping the vblank sync when the framebuffer has not changed
> > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> > > > this for the common case of moving the cursor and only need to
> > > > delay the cursor ioctl when the cursor icon changes.
> > > >
> > > > I originally inserted a check on legacy_cursor_update as well, but
> > > > that caused a storm of iommu page faults. I didn't investigate the
> > > > cause of those since this change gives enough of a performance
> > > > improvement for my use case.
> > > >
> > > > This is RFC because of that and because the framebuffer_changed()
> > > > function is copied from drm_atomic_helper.c as a quick way to test
> > > > the result.
> > > >
> > > > Signed-off-by: John Keeping
> > > > ---
> > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > > > +-- 1 file changed, 25 insertions(+), 2
> > > > deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
> > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > > @@ -177,8 +177,28 @@ static void
> > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > > > crtc_funcs->wait_for_update(crtc); }
> > > >
> > > > +static bool framebuffer_changed(struct drm_device *dev,
> > > > + struct drm_atomic_state *old_state,
> > > > + struct drm_crtc *crtc)
> > > > +{
> > > > + struct drm_plane *plane;
> > > > + struct drm_plane_state *old_plane_state;
> > > > + int i;
> > > > +
> > > > + for_each_plane_in_state(old_state, plane, old_plane_state,
> > > > i) {
> > > > + if (plane->state->crtc != crtc &&
> > > > + old_plane_state->crtc != crtc)
> > > > + continue;
> > > > +
> > > > + if (plane->state->fb != old_plane_state->fb)
> > > > + return true;
> > > > + }
> > > > +
> > > > + return false;
> > > > +}
> > >
> > > Please don't hand-roll logic that affects semantics like this. Instead
> > > please use drm_atomic_helper_wait_for_vblanks(), which should do this
> > > correctly for you.
> > >
> > > If that's not the case then we need to improve the generic helper, or
> > > figure out what's different with rockhip.
> >
> > According to commit 63ebb9f (drm/rockchip: Convert to support atomic
> > API) it's because rockchip doesn't have a hardware vblank counter.
> >
> > I'm not entirely clear on why this prevents the use of
> > drm_atomic_helper_wait_for_vblanks().
>
> Hm, that commit isn't terribly helpful. If that's really needed then imo I
> think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
> helper that's used by both. But since rockchip does vblank_get/put calls
> I'd hope vblanks actually work correctly. And then the helper should work
> too.
I tried switching the call to rockchip_crtc_wait_for_update() to
drm_atomic_helper_wait_for_vblanks() and it works fine until I switch
the buffer associated with a cursor, at which point I get iommu page
faults, presumably because the GEM buffer is unreferenced too early.
AFAICT the buffer will be released via drm_atomic_state_free()
unconditionally, but I suspect I'm missing something since that would
mean every driver would hit a similar problem.
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies
> on cursor ioctls being unsynced. Converting the rockchip driver to
> atomic has significantly impacted cursor performance by making every
> cursor update wait for vblank.
>
> By skipping the vblank sync when the framebuffer has not changed (as is
> done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the
> common case of moving the cursor and only need to delay the cursor ioctl
> when the cursor icon changes.
>
> I originally inserted a check on legacy_cursor_update as well, but that
> caused a storm of iommu page faults. I didn't investigate the cause of
> those since this change gives enough of a performance improvement for my
> use case.
>
> This is RFC because of that and because the framebuffer_changed()
> function is copied from drm_atomic_helper.c as a quick way to test the
> result.
>
> Signed-off-by: John Keeping
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27 +--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index f784488..8fd9821 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -177,8 +177,28 @@ static void rockchip_crtc_wait_for_update(struct
> drm_crtc *crtc)
> crtc_funcs->wait_for_update(crtc);
> }
>
> +static bool framebuffer_changed(struct drm_device *dev,
> + struct drm_atomic_state *old_state,
> + struct drm_crtc *crtc)
> +{
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state;
> + int i;
> +
> + for_each_plane_in_state(old_state, plane, old_plane_state, i) {
> + if (plane->state->crtc != crtc &&
> + old_plane_state->crtc != crtc)
> + continue;
> +
> + if (plane->state->fb != old_plane_state->fb)
> + return true;
> + }
> +
> + return false;
> +}
Please don't hand-roll logic that affects semantics like this. Instead
please use drm_atomic_helper_wait_for_vblanks(), which should do this
correctly for you.
If that's not the case then we need to improve the generic helper, or
figure out what's different with rockhip.
Thanks, Daniel
> +
> static void
> -rockchip_atomic_wait_for_complete(struct drm_atomic_state *old_state)
> +rockchip_atomic_wait_for_complete(struct drm_device *dev, struct
> drm_atomic_state *old_state)
> {
> struct drm_crtc_state *old_crtc_state;
> struct drm_crtc *crtc;
> @@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct drm_atomic_state
> *old_state)
> if (!crtc->state->active)
> continue;
>
> + if (!framebuffer_changed(dev, old_state, crtc))
> + continue;
> +
> ret = drm_crtc_vblank_get(crtc);
> if (ret != 0)
> continue;
> @@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct
> rockchip_atomic_commit *commit)
>
> drm_atomic_helper_commit_planes(dev, state, true);
>
> - rockchip_atomic_wait_for_complete(state);
> + rockchip_atomic_wait_for_complete(dev, state);
>
> drm_atomic_helper_cleanup_planes(dev, state);
>
> --
> 2.7.0.rc3.140.g520a093
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote:
> On Wed, Jan 13, 2016 at 12:53:34PM +, John Keeping wrote:
> > As commented in drm_atomic_helper_wait_for_vblanks(), userspace
> > relies on cursor ioctls being unsynced. Converting the rockchip
> > driver to atomic has significantly impacted cursor performance by
> > making every cursor update wait for vblank.
> >
> > By skipping the vblank sync when the framebuffer has not changed
> > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid
> > this for the common case of moving the cursor and only need to
> > delay the cursor ioctl when the cursor icon changes.
> >
> > I originally inserted a check on legacy_cursor_update as well, but
> > that caused a storm of iommu page faults. I didn't investigate the
> > cause of those since this change gives enough of a performance
> > improvement for my use case.
> >
> > This is RFC because of that and because the framebuffer_changed()
> > function is copied from drm_atomic_helper.c as a quick way to test
> > the result.
> >
> > Signed-off-by: John Keeping
> > ---
> > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27
> > +-- 1 file changed, 25 insertions(+), 2
> > deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
> > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -177,8 +177,28 @@ static void
> > rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
> > crtc_funcs->wait_for_update(crtc); }
> >
> > +static bool framebuffer_changed(struct drm_device *dev,
> > + struct drm_atomic_state *old_state,
> > + struct drm_crtc *crtc)
> > +{
> > + struct drm_plane *plane;
> > + struct drm_plane_state *old_plane_state;
> > + int i;
> > +
> > + for_each_plane_in_state(old_state, plane, old_plane_state,
> > i) {
> > + if (plane->state->crtc != crtc &&
> > + old_plane_state->crtc != crtc)
> > + continue;
> > +
> > + if (plane->state->fb != old_plane_state->fb)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
>
> Please don't hand-roll logic that affects semantics like this. Instead
> please use drm_atomic_helper_wait_for_vblanks(), which should do this
> correctly for you.
>
> If that's not the case then we need to improve the generic helper, or
> figure out what's different with rockhip.
According to commit 63ebb9f (drm/rockchip: Convert to support atomic
API) it's because rockchip doesn't have a hardware vblank counter.
I'm not entirely clear on why this prevents the use of
drm_atomic_helper_wait_for_vblanks().
> > +
> > static void
> > -rockchip_atomic_wait_for_complete(struct drm_atomic_state
> > *old_state) +rockchip_atomic_wait_for_complete(struct drm_device
> > *dev, struct drm_atomic_state *old_state) {
> > struct drm_crtc_state *old_crtc_state;
> > struct drm_crtc *crtc;
> > @@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct
> > drm_atomic_state *old_state) if (!crtc->state->active)
> > continue;
> >
> > + if (!framebuffer_changed(dev, old_state, crtc))
> > + continue;
> > +
> > ret = drm_crtc_vblank_get(crtc);
> > if (ret != 0)
> > continue;
> > @@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct
> > rockchip_atomic_commit *commit)
> > drm_atomic_helper_commit_planes(dev, state, true);
> >
> > - rockchip_atomic_wait_for_complete(state);
> > + rockchip_atomic_wait_for_complete(dev, state);
> >
> > drm_atomic_helper_cleanup_planes(dev, state);
> >
> > --
> > 2.7.0.rc3.140.g520a093
> >
> > ___
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
[RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed
As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies
on cursor ioctls being unsynced. Converting the rockchip driver to
atomic has significantly impacted cursor performance by making every
cursor update wait for vblank.
By skipping the vblank sync when the framebuffer has not changed (as is
done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the
common case of moving the cursor and only need to delay the cursor ioctl
when the cursor icon changes.
I originally inserted a check on legacy_cursor_update as well, but that
caused a storm of iommu page faults. I didn't investigate the cause of
those since this change gives enough of a performance improvement for my
use case.
This is RFC because of that and because the framebuffer_changed()
function is copied from drm_atomic_helper.c as a quick way to test the
result.
Signed-off-by: John Keeping
---
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27 +--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index f784488..8fd9821 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -177,8 +177,28 @@ static void rockchip_crtc_wait_for_update(struct drm_crtc
*crtc)
crtc_funcs->wait_for_update(crtc);
}
+static bool framebuffer_changed(struct drm_device *dev,
+ struct drm_atomic_state *old_state,
+ struct drm_crtc *crtc)
+{
+ struct drm_plane *plane;
+ struct drm_plane_state *old_plane_state;
+ int i;
+
+ for_each_plane_in_state(old_state, plane, old_plane_state, i) {
+ if (plane->state->crtc != crtc &&
+ old_plane_state->crtc != crtc)
+ continue;
+
+ if (plane->state->fb != old_plane_state->fb)
+ return true;
+ }
+
+ return false;
+}
+
static void
-rockchip_atomic_wait_for_complete(struct drm_atomic_state *old_state)
+rockchip_atomic_wait_for_complete(struct drm_device *dev, struct
drm_atomic_state *old_state)
{
struct drm_crtc_state *old_crtc_state;
struct drm_crtc *crtc;
@@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct drm_atomic_state
*old_state)
if (!crtc->state->active)
continue;
+ if (!framebuffer_changed(dev, old_state, crtc))
+ continue;
+
ret = drm_crtc_vblank_get(crtc);
if (ret != 0)
continue;
@@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct
rockchip_atomic_commit *commit)
drm_atomic_helper_commit_planes(dev, state, true);
- rockchip_atomic_wait_for_complete(state);
+ rockchip_atomic_wait_for_complete(dev, state);
drm_atomic_helper_cleanup_planes(dev, state);
--
2.7.0.rc3.140.g520a093
