| Message ID | 1455875288-4370-3-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
On 23/02/16 12:27, Laurent Pinchart wrote: > 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. Yes, the comment was a bit lacking. Here's an updated comment: /* * 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. */ Tomi
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;
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> --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 ++++++ 1 file changed, 6 insertions(+)