Re: [PATCH 02/33] HACK: drm/omap: always use blocking DMM fill

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 02/33] HACK: drm/omap: always use blocking DMM fill
Date: Tue, 23 Feb 2016 12:27:17 +0200	[thread overview]
Message-ID: <1818041.Py0QKdZPaW@avalon> (raw)
In-Reply-To: <1455875288-4370-3-git-send-email-tomi.valkeinen@ti.com>

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:37 Tomi Valkeinen wrote:
> The current driver uses non-blocking DMM fill when releasing memory.
> This gives us a small performance increase as we don't have to wait for
> the fill operation to finish.
> 
> However, the driver does not have any error handling for non-blocking
> fill. In case of an error, the fill operation may silently fail, leading
> to leaking DMM engines, which may eventually lead to deadlock if we run
> out of DMM engines.
> 
> This patch makes the DMM driver always use blocking fills, so that we
> can catch the errors. A more complex option would be to allow
> non-blocking fills, and implement proper error handling, but that is
> left for the future.
> 
> This patch is a HACK, as the proper fix is to either decide to always
> use sync fills and remove all the async related code, or fix the async
> code.

Could you capture this in the comment in the source code below ? I'd also 
replace XXXX with either FIXME or TODO.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index dfebdc4aa0f2..80526dec7b2c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -309,6 +309,12 @@ static int fill(struct tcm_area *area, struct page
> **pages, struct tcm_area slice, area_s;
>  	struct dmm_txn *txn;
> 
> +	/*
> +	 * XXX async wait doesn't work, as it does not handle timeout errors
> +	 * properly
> +	 */
> +	wait = true;
> +
>  	txn = dmm_txn_init(omap_dmm, area->tcm);
>  	if (IS_ERR_OR_NULL(txn))
>  		return -ENOMEM;

-- 
Regards,

Laurent Pinchart

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