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@xxxxxxxxx wrote:
> > > From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > ---
> > > 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 ;)
> >
> > > +
> > > + 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
>
