Re: [PATCHv2 02/31] HACK: drm/omap: always use blocking DMM fill

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Friday 26 February 2016 11:35:50 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.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index dfebdc4aa0f2..67edf839dce3
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -309,6 +309,21 @@ static int fill(struct tcm_area *area, struct page
> **pages, struct tcm_area slice, area_s;
>  	struct dmm_txn *txn;
> 
> +	/*
> +	 * FIXME
> +	 *
> +	 * Asynchronous fill does not work reliably, as the driver does not
> +	 * handle errors in the async code paths. The fill operation may
> +	 * silently fail, leading to leaking DMM engines, which may eventually
> +	 * lead to deadlock if we run out of DMM engines.
> +	 *
> +	 * For now, always set 'wait' so that we only use sync fills. Async
> +	 * fills should be fixed, or alternatively we could decide to only
> +	 * support sync fills and so the whole async code path could be removed.
> +	 */
> +
> +	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