Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context
Hi, On Mon, 2018-11-12 at 18:05 -0300, Ezequiel Garcia wrote: > On Mon, 12 Nov 2018 at 13:52, Paul Kocialkowski > wrote: > > Hi, > > > > On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote: > > > On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote: > > > > This series goal is to avoid drivers from having ad-hoc code > > > > to call .device_run in non-atomic context. Currently, .device_run > > > > can be called via v4l2_m2m_job_finish(), not only running > > > > in interrupt context, but also creating a nasty re-entrant > > > > path into mem2mem drivers. > > > > > > > > The proposed solution is to add a per-device worker that is scheduled > > > > by v4l2_m2m_job_finish, which replaces drivers having a threaded > > > > interrupt > > > > or similar. > > > > > > > > This change allows v4l2_m2m_job_finish() to be called in interrupt > > > > context, separating .device_run and v4l2_m2m_job_finish() contexts. > > > > > > > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need > > > > to flush or cancel the new worker, because the job_spinlock > > > > synchronizes both and also because the core prevents simultaneous > > > > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the > > > > worker will be unable to run a new job. > > > > > > > > Patches apply on top of Request API and the Cedrus VPU > > > > driver. > > > > > > > > Tested with cedrus driver using v4l2-request-test and > > > > vicodec driver using gstreamer. > > > > > > > > Ezequiel Garcia (4): > > > > mem2mem: Require capture and output mutexes to match > > > > v4l2-ioctl.c: Simplify locking for m2m devices > > > > v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish > > > > media: cedrus: Get rid of interrupt bottom-half > > > > > > > > Sakari Ailus (1): > > > > v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule > > > > > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 + > > > > drivers/media/v4l2-core/v4l2-mem2mem.c| 66 --- > > > > .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++-- > > > > 3 files changed, 51 insertions(+), 88 deletions(-) > > > > > > > > > > Hans, Maxime: > > > > > > Any feedback for this? > > > > I just tested the whole series with the cedrus driver and everything > > looks good! > > > > Good! So this means we can add a Tested-by for the entire series? Definitely, I just sent the tested-by line on the cover letter! > > Removing the interrupt bottom-half in favor of a workqueue in the core > > seems like a good way to simplify m2m driver development by avoiding > > per-driver workqueues or threaded irqs. > > > > Thanks for the test! Cheers, Paul -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context
Hi, On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote: > This series goal is to avoid drivers from having ad-hoc code > to call .device_run in non-atomic context. Currently, .device_run > can be called via v4l2_m2m_job_finish(), not only running > in interrupt context, but also creating a nasty re-entrant > path into mem2mem drivers. > > The proposed solution is to add a per-device worker that is scheduled > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt > or similar. > > This change allows v4l2_m2m_job_finish() to be called in interrupt > context, separating .device_run and v4l2_m2m_job_finish() contexts. > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need > to flush or cancel the new worker, because the job_spinlock > synchronizes both and also because the core prevents simultaneous > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the > worker will be unable to run a new job. > > Patches apply on top of Request API and the Cedrus VPU > driver. > > Tested with cedrus driver using v4l2-request-test and > vicodec driver using gstreamer. For the entire series, tested with the cedrus driver: Tested-by: Paul Kocialkowski Cheers, Paul > Ezequiel Garcia (4): > mem2mem: Require capture and output mutexes to match > v4l2-ioctl.c: Simplify locking for m2m devices > v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish > media: cedrus: Get rid of interrupt bottom-half > > Sakari Ailus (1): > v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 + > drivers/media/v4l2-core/v4l2-mem2mem.c| 66 --- > .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++-- > 3 files changed, 51 insertions(+), 88 deletions(-) > -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context
On Mon, 12 Nov 2018 at 13:52, Paul Kocialkowski wrote: > > Hi, > > On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote: > > On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote: > > > This series goal is to avoid drivers from having ad-hoc code > > > to call .device_run in non-atomic context. Currently, .device_run > > > can be called via v4l2_m2m_job_finish(), not only running > > > in interrupt context, but also creating a nasty re-entrant > > > path into mem2mem drivers. > > > > > > The proposed solution is to add a per-device worker that is scheduled > > > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt > > > or similar. > > > > > > This change allows v4l2_m2m_job_finish() to be called in interrupt > > > context, separating .device_run and v4l2_m2m_job_finish() contexts. > > > > > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need > > > to flush or cancel the new worker, because the job_spinlock > > > synchronizes both and also because the core prevents simultaneous > > > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the > > > worker will be unable to run a new job. > > > > > > Patches apply on top of Request API and the Cedrus VPU > > > driver. > > > > > > Tested with cedrus driver using v4l2-request-test and > > > vicodec driver using gstreamer. > > > > > > Ezequiel Garcia (4): > > > mem2mem: Require capture and output mutexes to match > > > v4l2-ioctl.c: Simplify locking for m2m devices > > > v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish > > > media: cedrus: Get rid of interrupt bottom-half > > > > > > Sakari Ailus (1): > > > v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule > > > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 + > > > drivers/media/v4l2-core/v4l2-mem2mem.c| 66 --- > > > .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++-- > > > 3 files changed, 51 insertions(+), 88 deletions(-) > > > > > > > Hans, Maxime: > > > > Any feedback for this? > > I just tested the whole series with the cedrus driver and everything > looks good! > Good! So this means we can add a Tested-by for the entire series? > Removing the interrupt bottom-half in favor of a workqueue in the core > seems like a good way to simplify m2m driver development by avoiding > per-driver workqueues or threaded irqs. > Thanks for the test! -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context
Hi, On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote: > On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote: > > This series goal is to avoid drivers from having ad-hoc code > > to call .device_run in non-atomic context. Currently, .device_run > > can be called via v4l2_m2m_job_finish(), not only running > > in interrupt context, but also creating a nasty re-entrant > > path into mem2mem drivers. > > > > The proposed solution is to add a per-device worker that is scheduled > > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt > > or similar. > > > > This change allows v4l2_m2m_job_finish() to be called in interrupt > > context, separating .device_run and v4l2_m2m_job_finish() contexts. > > > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need > > to flush or cancel the new worker, because the job_spinlock > > synchronizes both and also because the core prevents simultaneous > > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the > > worker will be unable to run a new job. > > > > Patches apply on top of Request API and the Cedrus VPU > > driver. > > > > Tested with cedrus driver using v4l2-request-test and > > vicodec driver using gstreamer. > > > > Ezequiel Garcia (4): > > mem2mem: Require capture and output mutexes to match > > v4l2-ioctl.c: Simplify locking for m2m devices > > v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish > > media: cedrus: Get rid of interrupt bottom-half > > > > Sakari Ailus (1): > > v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 + > > drivers/media/v4l2-core/v4l2-mem2mem.c| 66 --- > > .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++-- > > 3 files changed, 51 insertions(+), 88 deletions(-) > > > > Hans, Maxime: > > Any feedback for this? I just tested the whole series with the cedrus driver and everything looks good! Removing the interrupt bottom-half in favor of a workqueue in the core seems like a good way to simplify m2m driver development by avoiding per-driver workqueues or threaded irqs. Cheers, Paul -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context
On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote: > This series goal is to avoid drivers from having ad-hoc code > to call .device_run in non-atomic context. Currently, .device_run > can be called via v4l2_m2m_job_finish(), not only running > in interrupt context, but also creating a nasty re-entrant > path into mem2mem drivers. > > The proposed solution is to add a per-device worker that is scheduled > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt > or similar. > > This change allows v4l2_m2m_job_finish() to be called in interrupt > context, separating .device_run and v4l2_m2m_job_finish() contexts. > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need > to flush or cancel the new worker, because the job_spinlock > synchronizes both and also because the core prevents simultaneous > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the > worker will be unable to run a new job. > > Patches apply on top of Request API and the Cedrus VPU > driver. > > Tested with cedrus driver using v4l2-request-test and > vicodec driver using gstreamer. > > Ezequiel Garcia (4): > mem2mem: Require capture and output mutexes to match > v4l2-ioctl.c: Simplify locking for m2m devices > v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish > media: cedrus: Get rid of interrupt bottom-half > > Sakari Ailus (1): > v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 + > drivers/media/v4l2-core/v4l2-mem2mem.c| 66 --- > .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++-- > 3 files changed, 51 insertions(+), 88 deletions(-) > Hans, Maxime: Any feedback for this? Thanks, Eze
[PATCH v5 0/5] Make sure .device_run is always called in non-atomic context
This series goal is to avoid drivers from having ad-hoc code to call .device_run in non-atomic context. Currently, .device_run can be called via v4l2_m2m_job_finish(), not only running in interrupt context, but also creating a nasty re-entrant path into mem2mem drivers. The proposed solution is to add a per-device worker that is scheduled by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt or similar. This change allows v4l2_m2m_job_finish() to be called in interrupt context, separating .device_run and v4l2_m2m_job_finish() contexts. It's worth mentioning that v4l2_m2m_cancel_job() doesn't need to flush or cancel the new worker, because the job_spinlock synchronizes both and also because the core prevents simultaneous jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the worker will be unable to run a new job. Patches apply on top of Request API and the Cedrus VPU driver. Tested with cedrus driver using v4l2-request-test and vicodec driver using gstreamer. Ezequiel Garcia (4): mem2mem: Require capture and output mutexes to match v4l2-ioctl.c: Simplify locking for m2m devices v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish media: cedrus: Get rid of interrupt bottom-half Sakari Ailus (1): v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule drivers/media/v4l2-core/v4l2-ioctl.c | 47 + drivers/media/v4l2-core/v4l2-mem2mem.c| 66 --- .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++-- 3 files changed, 51 insertions(+), 88 deletions(-) -- 2.19.1
