Re: [PATCH 2/2] drm/mediatek: Bypass atomic helpers for cursor updates

[CK Hu]

Hi, Bibby:

On Fri, 2019-08-30 at 15:38 +0800, Bibby Hsieh wrote:
> Moving the driver to atomic helpers regressed cursor responsiveness,
> because cursor updates need their own atomic commits, which have to be
> serialized with other commits, that might include fence waits. To avoid
> this, in certain conditions, we can bypass the atomic helpers for legacy
> cursor update IOCTLs. Currently the conditions are:
>  - no asynchronous mode setting commit pending,
>  - no asynchronous commit that updates the cursor plane is pending.
> With the above two conditions met, we know that the manual cursor state
> update will not conflict with any scheduled update.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> 
> Signed-off-by: Bibby Hsieh <bibby.hsieh@xxxxxxxxxxxx>
> Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 41 ++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.h  |  2 +
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   | 34 ++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h   |  3 +
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 73 +++++++++++++++++++++++-
>  5 files changed, 148 insertions(+), 5 deletions(-)
> 

[snip]

> +
> +static int mtk_plane_update(struct drm_plane *plane,
> +			    struct drm_crtc *crtc,
> +			    struct drm_framebuffer *fb,
> +			    int crtc_x, int crtc_y,
> +			    unsigned int crtc_w, unsigned int crtc_h,
> +			    uint32_t src_x, uint32_t src_y,
> +			    uint32_t src_w, uint32_t src_h,
> +			    struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct mtk_drm_private *private = plane->dev->dev_private;
> +	uint32_t crtc_mask = (1 << drm_crtc_index(crtc));
> +
> +	if (crtc && plane == crtc->cursor &&
> +	    plane->state->crtc == crtc &&
> +	    !(private->commit.flush_for_cursor & crtc_mask))
> +		return mtk_plane_cursor_update(plane, crtc, fb,
> +				crtc_x, crtc_y, crtc_w, crtc_h,
> +				src_x, src_y, src_w, src_h);
> +
> +	return drm_atomic_helper_update_plane(plane, crtc, fb,
> +					      crtc_x, crtc_y, crtc_w, crtc_h,
> +					      src_x, src_y, src_w, src_h, ctx);
> +}
> +
>  static const struct drm_plane_funcs mtk_plane_funcs = {
> -	.update_plane = drm_atomic_helper_update_plane,
> +	.update_plane = mtk_plane_update,

I think drm core has already process cursor async problem. In [1], you
could search 'legacy_cursor_update' and it need driver to implement
atomic_async_check() and atomic_async_update() callback function. You
could refer to [2] for the implementation. 


[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_atomic_helper.c?h=v5.3-rc6
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/rockchip_drm_vop.c?h=v5.3-rc6#n955

Regards,
CK

>  	.disable_plane = drm_atomic_helper_disable_plane,
>  	.destroy = drm_plane_cleanup,
>  	.reset = mtk_plane_reset,
> @@ -90,7 +154,12 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>  	if (!state->crtc)
>  		return 0;
>  
> -	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> +	if (state->state)
> +		crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> +								state->crtc);
> +	else /* Special case for asynchronous cursor updates. */
> +		crtc_state = state->crtc->state;
> +
>  	if (IS_ERR(crtc_state))
>  		return PTR_ERR(crtc_state);
>