* Re: [RFC RESEND PATCH] drm/rockchip: update cursors asynchronously through atomic.
@ 2018-07-23 14:36 ` Sean Paul
0 siblings, 0 replies; 7+ messages in thread
From: Sean Paul @ 2018-07-23 14:36 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Sandy Huang, Heiko Stübner, David Airlie, linux-arm-kernel,
linux-kernel, dri-devel, linux-rockchip, Gustavo Padovan,
Tomasz Figa, Sean Paul, kernel, Stéphane Marchesin
On Wed, Jun 27, 2018 at 11:14:47PM +0200, Enric Balletbo i Serra wrote:
> Add support to async updates of cursors by using the new atomic
> interface for that.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
LGTM. Given rockchip hasn't weighed in on the patch, and that you've tested it
on real hardware, let's land it.
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> I am sending this as RFC because I still don't have a deep knowledge of
> the hw and I am not sure if the vop_plane_update function can be reused
> in both cases, atomic_updates and atomic_async_updates. I think that
> someone with more knowledge should take a look. The patch was tested on
> a Samsung Chromebook Plus in two ways.
>
> 1. Running all igt kms_cursor_legacy and kms_atomic@plane_cursor_legacy
> tests and see that there is no regression after the patch.
>
> 2. Running weston using the atomic API.
>
> Best regards,
> Enric
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 80 ++++++++++++++++-----
> 1 file changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 53d4afe15278..1eb6bda924af 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -688,8 +688,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
> spin_unlock(&vop->reg_lock);
> }
>
> -static void vop_plane_atomic_update(struct drm_plane *plane,
> - struct drm_plane_state *old_state)
> +static void vop_plane_update(struct drm_plane *plane)
> {
> struct drm_plane_state *state = plane->state;
> struct drm_crtc *crtc = state->crtc;
> @@ -710,20 +709,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
> bool rb_swap;
> int format;
>
> - /*
> - * can't update plane when vop is disabled.
> - */
> - if (WARN_ON(!crtc))
> - return;
> -
> - if (WARN_ON(!vop->is_enabled))
> - return;
> -
> - if (!state->visible) {
> - vop_plane_atomic_disable(plane, old_state);
> - return;
> - }
> -
> obj = rockchip_fb_get_gem_obj(fb, 0);
> rk_obj = to_rockchip_obj(obj);
>
> @@ -794,10 +779,73 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
> spin_unlock(&vop->reg_lock);
> }
>
> +static void vop_plane_atomic_update(struct drm_plane *plane,
> + struct drm_plane_state *old_state)
> +{
> + struct drm_plane_state *state = plane->state;
> + struct vop *vop = to_vop(state->crtc);
> +
> + /*
> + * can't update plane when vop is disabled.
> + */
> + if (WARN_ON(!state->crtc))
> + return;
> +
> + if (WARN_ON(!vop->is_enabled))
> + return;
> +
> + if (!state->visible) {
> + vop_plane_atomic_disable(plane, old_state);
> + return;
> + }
> +
> + vop_plane_update(plane);
> +}
> +
> +static int vop_plane_atomic_async_check(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct drm_crtc_state *crtc_state;
> +
> + crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> + state->crtc);
> + if (WARN_ON(!crtc_state))
> + return -EINVAL;
> +
> + if (!crtc_state->active)
> + return -EINVAL;
> +
> + if (plane->state->crtc != state->crtc ||
> + plane->state->src_w != state->src_w ||
> + plane->state->src_h != state->src_h ||
> + plane->state->crtc_w != state->crtc_w ||
> + plane->state->crtc_h != state->crtc_h ||
> + !plane->state->fb ||
> + plane->state->fb != state->fb)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static void vop_plane_atomic_async_update(struct drm_plane *plane,
> + struct drm_plane_state *new_state)
> +{
> + plane->state->src_x = new_state->src_x;
> + plane->state->src_y = new_state->src_y;
> + plane->state->crtc_x = new_state->crtc_x;
> + plane->state->crtc_y = new_state->crtc_y;
> + plane->state->fb = new_state->fb;
> + *plane->state = *new_state;
> +
> + vop_plane_update(plane);
> +}
> +
> static const struct drm_plane_helper_funcs plane_helper_funcs = {
> .atomic_check = vop_plane_atomic_check,
> .atomic_update = vop_plane_atomic_update,
> .atomic_disable = vop_plane_atomic_disable,
> + .atomic_async_check = vop_plane_atomic_async_check,
> + .atomic_async_update = vop_plane_atomic_async_update,
> };
>
> static const struct drm_plane_funcs vop_plane_funcs = {
> --
> 2.18.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC RESEND PATCH] drm/rockchip: update cursors asynchronously through atomic.
@ 2018-07-23 14:36 ` Sean Paul
0 siblings, 0 replies; 7+ messages in thread
From: Sean Paul @ 2018-07-23 14:36 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 27, 2018 at 11:14:47PM +0200, Enric Balletbo i Serra wrote:
> Add support to async updates of cursors by using the new atomic
> interface for that.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
LGTM. Given rockchip hasn't weighed in on the patch, and that you've tested it
on real hardware, let's land it.
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> I am sending this as RFC because I still don't have a deep knowledge of
> the hw and I am not sure if the vop_plane_update function can be reused
> in both cases, atomic_updates and atomic_async_updates. I think that
> someone with more knowledge should take a look. The patch was tested on
> a Samsung Chromebook Plus in two ways.
>
> 1. Running all igt kms_cursor_legacy and kms_atomic at plane_cursor_legacy
> tests and see that there is no regression after the patch.
>
> 2. Running weston using the atomic API.
>
> Best regards,
> Enric
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 80 ++++++++++++++++-----
> 1 file changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 53d4afe15278..1eb6bda924af 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -688,8 +688,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
> spin_unlock(&vop->reg_lock);
> }
>
> -static void vop_plane_atomic_update(struct drm_plane *plane,
> - struct drm_plane_state *old_state)
> +static void vop_plane_update(struct drm_plane *plane)
> {
> struct drm_plane_state *state = plane->state;
> struct drm_crtc *crtc = state->crtc;
> @@ -710,20 +709,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
> bool rb_swap;
> int format;
>
> - /*
> - * can't update plane when vop is disabled.
> - */
> - if (WARN_ON(!crtc))
> - return;
> -
> - if (WARN_ON(!vop->is_enabled))
> - return;
> -
> - if (!state->visible) {
> - vop_plane_atomic_disable(plane, old_state);
> - return;
> - }
> -
> obj = rockchip_fb_get_gem_obj(fb, 0);
> rk_obj = to_rockchip_obj(obj);
>
> @@ -794,10 +779,73 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
> spin_unlock(&vop->reg_lock);
> }
>
> +static void vop_plane_atomic_update(struct drm_plane *plane,
> + struct drm_plane_state *old_state)
> +{
> + struct drm_plane_state *state = plane->state;
> + struct vop *vop = to_vop(state->crtc);
> +
> + /*
> + * can't update plane when vop is disabled.
> + */
> + if (WARN_ON(!state->crtc))
> + return;
> +
> + if (WARN_ON(!vop->is_enabled))
> + return;
> +
> + if (!state->visible) {
> + vop_plane_atomic_disable(plane, old_state);
> + return;
> + }
> +
> + vop_plane_update(plane);
> +}
> +
> +static int vop_plane_atomic_async_check(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct drm_crtc_state *crtc_state;
> +
> + crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> + state->crtc);
> + if (WARN_ON(!crtc_state))
> + return -EINVAL;
> +
> + if (!crtc_state->active)
> + return -EINVAL;
> +
> + if (plane->state->crtc != state->crtc ||
> + plane->state->src_w != state->src_w ||
> + plane->state->src_h != state->src_h ||
> + plane->state->crtc_w != state->crtc_w ||
> + plane->state->crtc_h != state->crtc_h ||
> + !plane->state->fb ||
> + plane->state->fb != state->fb)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static void vop_plane_atomic_async_update(struct drm_plane *plane,
> + struct drm_plane_state *new_state)
> +{
> + plane->state->src_x = new_state->src_x;
> + plane->state->src_y = new_state->src_y;
> + plane->state->crtc_x = new_state->crtc_x;
> + plane->state->crtc_y = new_state->crtc_y;
> + plane->state->fb = new_state->fb;
> + *plane->state = *new_state;
> +
> + vop_plane_update(plane);
> +}
> +
> static const struct drm_plane_helper_funcs plane_helper_funcs = {
> .atomic_check = vop_plane_atomic_check,
> .atomic_update = vop_plane_atomic_update,
> .atomic_disable = vop_plane_atomic_disable,
> + .atomic_async_check = vop_plane_atomic_async_check,
> + .atomic_async_update = vop_plane_atomic_async_update,
> };
>
> static const struct drm_plane_funcs vop_plane_funcs = {
> --
> 2.18.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC RESEND PATCH] drm/rockchip: update cursors asynchronously through atomic.
2018-07-23 14:36 ` Sean Paul
@ 2018-07-23 18:39 ` Gustavo Padovan
-1 siblings, 0 replies; 7+ messages in thread
From: Gustavo Padovan @ 2018-07-23 18:39 UTC (permalink / raw)
To: Sean Paul, Enric Balletbo i Serra
Cc: Sandy Huang, Heiko Stübner, David Airlie, linux-arm-kernel,
linux-kernel, dri-devel, linux-rockchip, Tomasz Figa, Sean Paul,
kernel, Stéphane Marchesin
Hi Enric,
On 07/23/2018 11:36 AM, Sean Paul wrote:
> On Wed, Jun 27, 2018 at 11:14:47PM +0200, Enric Balletbo i Serra wrote:
>> Add support to async updates of cursors by using the new atomic
>> interface for that.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>
> LGTM. Given rockchip hasn't weighed in on the patch, and that you've tested it
> on real hardware, let's land it.
>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
Your patch don't apply cleanly anymore. Can you rebase it and add the
r-b to the patch while at it? Thanks!
Gustavo
>
>
>> ---
>> I am sending this as RFC because I still don't have a deep knowledge of
>> the hw and I am not sure if the vop_plane_update function can be reused
>> in both cases, atomic_updates and atomic_async_updates. I think that
>> someone with more knowledge should take a look. The patch was tested on
>> a Samsung Chromebook Plus in two ways.
>>
>> 1. Running all igt kms_cursor_legacy and kms_atomic@plane_cursor_legacy
>> tests and see that there is no regression after the patch.
>>
>> 2. Running weston using the atomic API.
>>
>> Best regards,
>> Enric
>>
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 80 ++++++++++++++++-----
>> 1 file changed, 64 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 53d4afe15278..1eb6bda924af 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -688,8 +688,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
>> spin_unlock(&vop->reg_lock);
>> }
>>
>> -static void vop_plane_atomic_update(struct drm_plane *plane,
>> - struct drm_plane_state *old_state)
>> +static void vop_plane_update(struct drm_plane *plane)
>> {
>> struct drm_plane_state *state = plane->state;
>> struct drm_crtc *crtc = state->crtc;
>> @@ -710,20 +709,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>> bool rb_swap;
>> int format;
>>
>> - /*
>> - * can't update plane when vop is disabled.
>> - */
>> - if (WARN_ON(!crtc))
>> - return;
>> -
>> - if (WARN_ON(!vop->is_enabled))
>> - return;
>> -
>> - if (!state->visible) {
>> - vop_plane_atomic_disable(plane, old_state);
>> - return;
>> - }
>> -
>> obj = rockchip_fb_get_gem_obj(fb, 0);
>> rk_obj = to_rockchip_obj(obj);
>>
>> @@ -794,10 +779,73 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>> spin_unlock(&vop->reg_lock);
>> }
>>
>> +static void vop_plane_atomic_update(struct drm_plane *plane,
>> + struct drm_plane_state *old_state)
>> +{
>> + struct drm_plane_state *state = plane->state;
>> + struct vop *vop = to_vop(state->crtc);
>> +
>> + /*
>> + * can't update plane when vop is disabled.
>> + */
>> + if (WARN_ON(!state->crtc))
>> + return;
>> +
>> + if (WARN_ON(!vop->is_enabled))
>> + return;
>> +
>> + if (!state->visible) {
>> + vop_plane_atomic_disable(plane, old_state);
>> + return;
>> + }
>> +
>> + vop_plane_update(plane);
>> +}
>> +
>> +static int vop_plane_atomic_async_check(struct drm_plane *plane,
>> + struct drm_plane_state *state)
>> +{
>> + struct drm_crtc_state *crtc_state;
>> +
>> + crtc_state = drm_atomic_get_existing_crtc_state(state->state,
>> + state->crtc);
>> + if (WARN_ON(!crtc_state))
>> + return -EINVAL;
>> +
>> + if (!crtc_state->active)
>> + return -EINVAL;
>> +
>> + if (plane->state->crtc != state->crtc ||
>> + plane->state->src_w != state->src_w ||
>> + plane->state->src_h != state->src_h ||
>> + plane->state->crtc_w != state->crtc_w ||
>> + plane->state->crtc_h != state->crtc_h ||
>> + !plane->state->fb ||
>> + plane->state->fb != state->fb)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static void vop_plane_atomic_async_update(struct drm_plane *plane,
>> + struct drm_plane_state *new_state)
>> +{
>> + plane->state->src_x = new_state->src_x;
>> + plane->state->src_y = new_state->src_y;
>> + plane->state->crtc_x = new_state->crtc_x;
>> + plane->state->crtc_y = new_state->crtc_y;
>> + plane->state->fb = new_state->fb;
>> + *plane->state = *new_state;
>> +
>> + vop_plane_update(plane);
>> +}
>> +
>> static const struct drm_plane_helper_funcs plane_helper_funcs = {
>> .atomic_check = vop_plane_atomic_check,
>> .atomic_update = vop_plane_atomic_update,
>> .atomic_disable = vop_plane_atomic_disable,
>> + .atomic_async_check = vop_plane_atomic_async_check,
>> + .atomic_async_update = vop_plane_atomic_async_update,
>> };
>>
>> static const struct drm_plane_funcs vop_plane_funcs = {
>> --
>> 2.18.0
>>
>
--
Gustavo Padovan
Collabora Ltd
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC RESEND PATCH] drm/rockchip: update cursors asynchronously through atomic.
@ 2018-07-23 18:39 ` Gustavo Padovan
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo Padovan @ 2018-07-23 18:39 UTC (permalink / raw)
To: linux-arm-kernel
Hi Enric,
On 07/23/2018 11:36 AM, Sean Paul wrote:
> On Wed, Jun 27, 2018 at 11:14:47PM +0200, Enric Balletbo i Serra wrote:
>> Add support to async updates of cursors by using the new atomic
>> interface for that.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>
> LGTM. Given rockchip hasn't weighed in on the patch, and that you've tested it
> on real hardware, let's land it.
>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
Your patch don't apply cleanly anymore. Can you rebase it and add the
r-b to the patch while at it? Thanks!
Gustavo
>
>
>> ---
>> I am sending this as RFC because I still don't have a deep knowledge of
>> the hw and I am not sure if the vop_plane_update function can be reused
>> in both cases, atomic_updates and atomic_async_updates. I think that
>> someone with more knowledge should take a look. The patch was tested on
>> a Samsung Chromebook Plus in two ways.
>>
>> 1. Running all igt kms_cursor_legacy and kms_atomic at plane_cursor_legacy
>> tests and see that there is no regression after the patch.
>>
>> 2. Running weston using the atomic API.
>>
>> Best regards,
>> Enric
>>
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 80 ++++++++++++++++-----
>> 1 file changed, 64 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 53d4afe15278..1eb6bda924af 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -688,8 +688,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
>> spin_unlock(&vop->reg_lock);
>> }
>>
>> -static void vop_plane_atomic_update(struct drm_plane *plane,
>> - struct drm_plane_state *old_state)
>> +static void vop_plane_update(struct drm_plane *plane)
>> {
>> struct drm_plane_state *state = plane->state;
>> struct drm_crtc *crtc = state->crtc;
>> @@ -710,20 +709,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>> bool rb_swap;
>> int format;
>>
>> - /*
>> - * can't update plane when vop is disabled.
>> - */
>> - if (WARN_ON(!crtc))
>> - return;
>> -
>> - if (WARN_ON(!vop->is_enabled))
>> - return;
>> -
>> - if (!state->visible) {
>> - vop_plane_atomic_disable(plane, old_state);
>> - return;
>> - }
>> -
>> obj = rockchip_fb_get_gem_obj(fb, 0);
>> rk_obj = to_rockchip_obj(obj);
>>
>> @@ -794,10 +779,73 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>> spin_unlock(&vop->reg_lock);
>> }
>>
>> +static void vop_plane_atomic_update(struct drm_plane *plane,
>> + struct drm_plane_state *old_state)
>> +{
>> + struct drm_plane_state *state = plane->state;
>> + struct vop *vop = to_vop(state->crtc);
>> +
>> + /*
>> + * can't update plane when vop is disabled.
>> + */
>> + if (WARN_ON(!state->crtc))
>> + return;
>> +
>> + if (WARN_ON(!vop->is_enabled))
>> + return;
>> +
>> + if (!state->visible) {
>> + vop_plane_atomic_disable(plane, old_state);
>> + return;
>> + }
>> +
>> + vop_plane_update(plane);
>> +}
>> +
>> +static int vop_plane_atomic_async_check(struct drm_plane *plane,
>> + struct drm_plane_state *state)
>> +{
>> + struct drm_crtc_state *crtc_state;
>> +
>> + crtc_state = drm_atomic_get_existing_crtc_state(state->state,
>> + state->crtc);
>> + if (WARN_ON(!crtc_state))
>> + return -EINVAL;
>> +
>> + if (!crtc_state->active)
>> + return -EINVAL;
>> +
>> + if (plane->state->crtc != state->crtc ||
>> + plane->state->src_w != state->src_w ||
>> + plane->state->src_h != state->src_h ||
>> + plane->state->crtc_w != state->crtc_w ||
>> + plane->state->crtc_h != state->crtc_h ||
>> + !plane->state->fb ||
>> + plane->state->fb != state->fb)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static void vop_plane_atomic_async_update(struct drm_plane *plane,
>> + struct drm_plane_state *new_state)
>> +{
>> + plane->state->src_x = new_state->src_x;
>> + plane->state->src_y = new_state->src_y;
>> + plane->state->crtc_x = new_state->crtc_x;
>> + plane->state->crtc_y = new_state->crtc_y;
>> + plane->state->fb = new_state->fb;
>> + *plane->state = *new_state;
>> +
>> + vop_plane_update(plane);
>> +}
>> +
>> static const struct drm_plane_helper_funcs plane_helper_funcs = {
>> .atomic_check = vop_plane_atomic_check,
>> .atomic_update = vop_plane_atomic_update,
>> .atomic_disable = vop_plane_atomic_disable,
>> + .atomic_async_check = vop_plane_atomic_async_check,
>> + .atomic_async_update = vop_plane_atomic_async_update,
>> };
>>
>> static const struct drm_plane_funcs vop_plane_funcs = {
>> --
>> 2.18.0
>>
>
--
Gustavo Padovan
Collabora Ltd
^ permalink raw reply [flat|nested] 7+ messages in thread