Re: [PATCH v3 10/17] media: intel/ipu6: add input system driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Andreas,

On Thu, Feb 15, 2024 at 07:43:59AM +0100, Andreas Helbech Kleist wrote:
> Hi Sakari,
> 
> On Mon, 2024-02-12 at 18:31 +0000, Sakari Ailus wrote:
> > Hi Andreas,
> > 
> > On Wed, Feb 07, 2024 at 10:36:15AM +0100, Andreas Helbech Kleist wrote:
> > > Hi Bingbu,
> > > 
> > > Thank you for the patch series, I haven't had a chance to look at v3 in
> > > detail yet, so this is just a small comment from my testing on v2 +
> > > IPU4 hacks, which I can see is also here in v3.
> > > 
> > >  On Thu, 2024-01-11 at 14:55 +0800, bingbu.cao@intel.com wrote:
> > > > From: Bingbu Cao <bingbu.cao@intel.com>
> > > > 
> > > > Input system driver do basic isys hardware setup and irq handling
> > > > and work with fwnode and v4l2 to register the ISYS v4l2 devices.
> > > > 
> > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > ---
> > > >  drivers/media/pci/intel/ipu6/ipu6-isys.c | 1353 ++++++++++++++++++++++
> > > >  drivers/media/pci/intel/ipu6/ipu6-isys.h |  207 ++++
> > > >  2 files changed, 1560 insertions(+)
> > > >  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.c
> > > >  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.h
> > > > 
> > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> > > 
> > > ...
> > > 
> > > > +static void isys_unregister_devices(struct ipu6_isys *isys)
> > > > +{
> > > > +	isys_unregister_video_devices(isys);
> > > > +	isys_csi2_unregister_subdevices(isys);
> > > > +	v4l2_device_unregister(&isys->v4l2_dev);
> > > > +	media_device_unregister(&isys->media_dev);
> > > > +	media_device_cleanup(&isys->media_dev);
> > > > +}
> > > 
> > > ...
> > > 
> > > > +static void isys_remove(struct auxiliary_device *auxdev)
> > > > +{
> > > > +	struct ipu6_bus_device *adev = auxdev_to_adev(auxdev);
> > > > +	struct ipu6_isys *isys = dev_get_drvdata(&auxdev->dev);
> > > > +	struct ipu6_device *isp = adev->isp;
> > > > +	struct isys_fw_msgs *fwmsg, *safe;
> > > > +	unsigned int i;
> > > > +
> > > > +	list_for_each_entry_safe(fwmsg, safe, &isys->framebuflist, head)
> > > > +		dma_free_attrs(&auxdev->dev, sizeof(struct isys_fw_msgs),
> > > > +			       fwmsg, fwmsg->dma_addr, 0);
> > > > +
> > > > +	list_for_each_entry_safe(fwmsg, safe, &isys->framebuflist_fw, head)
> > > > +		dma_free_attrs(&auxdev->dev, sizeof(struct isys_fw_msgs),
> > > > +			       fwmsg, fwmsg->dma_addr, 0);
> > > 
> > > I experienced a crash in ipu6_get_fw_msg_buf when unbinding the PCI
> > > driver while streaming.
> > > 
> > > It happens because the above two lists are still used at this point. I
> > > believe it is safe to free the fw msgs after the
> > > isys_unregister_devices(isys) call below.
> > 
> > Probably yes, indeed.
> > 
> > However there's no support for unbinding a driver from a device while
> > streaming apart from plain V4L2 drivers. This needs to be addressed but we
> > can't address it driver by driver when the framework won't help with that,
> > it requires a comprehensive approach and support for this in MC and V4L2
> > sub-device frameworks.
> 
> Thank you for the information. I'll try to lower my expectations ;)

This has been the case since the very beginning of V4L2 sub-device nodes
and Media controller. At the time the use case involved devices that could
not be physically removed and managing the lifetime of the objects was seen
complex, so instead an assumption was made they'll always stay. And of
course it's now much, much harder to fix.

This set goes some way addressing this but it's really only a start:
<URL:https://lore.kernel.org/linux-media/20231220103713.113386-1-sakari.ailus@linux.intel.com/>

> 
> > > 
> > > > +
> > > > +	isys_unregister_devices(isys);
> > > > +	isys_notifier_cleanup(isys);
> > > > +
> > > > +	cpu_latency_qos_remove_request(&isys->pm_qos);
> > > > +
> > > > +	if (!isp->secure_mode) {
> > > > +		ipu6_cpd_free_pkg_dir(adev);
> > > > +		ipu6_buttress_unmap_fw_image(adev, &adev->fw_sgt);
> > > > +		release_firmware(adev->fw);
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < IPU6_ISYS_MAX_STREAMS; i++)
> > > > +		mutex_destroy(&isys->streams[i].mutex);
> > > > +
> > > > +	isys_iwake_watermark_cleanup(isys);
> > > > +	mutex_destroy(&isys->stream_mutex);
> > > > +	mutex_destroy(&isys->mutex);
> > > > +}
> > > 
> > > /Andreas
> > 
> 

-- 
Regards,

Sakari Ailus