Messages in this thread | | | Date | Fri, 15 Sep 2023 14:44:41 +0200 | Subject | Re: [RESEND PATCH v9 1/2] Added Digiteq Automotive MGB4 driver | From | Martin Tůma <> |
| |
Hi Hans,
On 15. 09. 23 13:50, Hans Verkuil wrote: > Hi Martin, > > On 12/09/2023 14:07, tumic@gpxsee.org wrote: >> From: Martin Tůma <martin.tuma@digiteqautomotive.com> >> >> Digiteq Automotive MGB4 is a modular frame grabber PCIe card for automotive >> video interfaces. As for now, two modules - FPD-Link and GMSL - are >> available and supported by the driver. The card has two inputs and two >> outputs (FPD-Link only). >> >> In addition to the video interfaces it also provides a trigger signal >> interface and a MTD interface for FPGA firmware upload. >> >> Signed-off-by: Martin Tůma <martin.tuma@digiteqautomotive.com> >> --- > > <snip> > >> +static ssize_t video_source_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct video_device *vdev = to_video_device(dev); >> + struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev); >> + u32 config = mgb4_read_reg(&voutdev->mgbdev->video, >> + voutdev->config->regs.config); >> + >> + return sprintf(buf, "%u\n", (config & 0xc) >> 2); >> +} >> + >> +static ssize_t video_source_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct video_device *vdev = to_video_device(dev); >> + struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev); >> + struct mgb4_dev *mgbdev = voutdev->mgbdev; >> + struct mgb4_vin_dev *loopin_new = 0, *loopin_old = 0; >> + unsigned long val; >> + unsigned long flags_in[MGB4_VIN_DEVICES], flags_out[MGB4_VOUT_DEVICES]; >> + ssize_t ret; >> + u32 config; >> + int i; >> + >> + memset(flags_in, 0, sizeof(flags_in)); >> + memset(flags_out, 0, sizeof(flags_out)); >> + >> + ret = kstrtoul(buf, 10, &val); >> + if (ret) >> + return ret; >> + if (val > 3) >> + return -EINVAL; >> + >> + for (i = 0; i < MGB4_VIN_DEVICES; i++) >> + if (mgbdev->vin[i]) >> + spin_lock_irqsave(&mgbdev->vin[i]->vdev.fh_lock, >> + flags_in[i]); >> + for (i = 0; i < MGB4_VOUT_DEVICES; i++) >> + if (mgbdev->vout[i]) >> + spin_lock_irqsave(&mgbdev->vout[i]->vdev.fh_lock, >> + flags_out[i]); > > I missed this in my review: never use fh_lock, that's for internal use only. > > Instead, use the vdev->lock mutex. > >> + >> + ret = -EBUSY; >> + for (i = 0; i < MGB4_VIN_DEVICES; i++) >> + if (mgbdev->vin[i] && !list_empty(&mgbdev->vin[i]->vdev.fh_list)) >> + goto error; > > This is also wrong, the key moment at which such changes are no longer allowed > is when you allocate buffers for the first time. Use vb2_is_busy() to detect that. > > From what I can see in the code most store functions operate on a single video device, > so for those using mutex_lock_interruptible(vdev->lock) and vb2_is_busy() will do the > trick. >
Ok, I will change the check to vb2_is_busy() under the vdev->lock for all the "standard" cases (which are all, except the video_source_store() case).
> For this store function that needs to check all devices it is more complicated. > > I think the best approach would be to create a top-level field 'reconfig_in_progress' > (or something along those lines), protected by a spinlock. > > At the start this function will take the lock, check if reconfig is already in progress > and just return EBUSY here. Otherwise it sets reconfig_in_progress to true and unlocks > the lock. > > After that for each video device you take the vdev->lock, check if vb2_is_busy() and > unlock vdev->lock. If any is busy, then set reconfig_in_progress back to false and > return EBUSY here. > > To prevent another process from creating buffers, you have to check in the queue_setup > functions if a reconfig is in progress and return EBUSY there. > > This scheme avoids having to lock ALL video devices (bad practice), and it should > also avoid this smatch warning. >
Ok, for this "ugly" one configuration I will add a PCIe device-based field and check it in all the v4l2 device's queue_setup() functions. Sounds better than locking all the devices at the same time.
Thanks for your help, I will send you a v10 with this two changes (together with the sparse/smatch fixes) soon.
M.
> Note: instead of using a spinlock I think you can also use test_and_set_bit: that's > atomic and you can avoid adding a spinlock. > > Regards, > > Hans > >> + for (i = 0; i < MGB4_VOUT_DEVICES; i++) >> + if (mgbdev->vout[i] && !list_empty(&mgbdev->vout[i]->vdev.fh_list)) >> + goto error; >> + >> + config = mgb4_read_reg(&mgbdev->video, voutdev->config->regs.config); >> + >> + if (((config & 0xc) >> 2) < MGB4_VIN_DEVICES) >> + loopin_old = mgbdev->vin[(config & 0xc) >> 2]; >> + if (val < MGB4_VIN_DEVICES) >> + loopin_new = mgbdev->vin[val]; >> + if (loopin_old && loopin_cnt(loopin_old) == 1) >> + mgb4_mask_reg(&mgbdev->video, loopin_old->config->regs.config, >> + 0x2, 0x0); >> + if (loopin_new) >> + mgb4_mask_reg(&mgbdev->video, loopin_new->config->regs.config, >> + 0x2, 0x2); >> + >> + if (val == voutdev->config->id + MGB4_VIN_DEVICES) >> + mgb4_write_reg(&mgbdev->video, voutdev->config->regs.config, >> + config & ~(1 << 1)); >> + else >> + mgb4_write_reg(&mgbdev->video, voutdev->config->regs.config, >> + config | (1U << 1)); >> + >> + mgb4_mask_reg(&mgbdev->video, voutdev->config->regs.config, 0xc, >> + val << 2); >> + >> + ret = count; >> + >> +error: >> + for (i = MGB4_VOUT_DEVICES - 1; i >= 0; i--) >> + if (mgbdev->vout[i]) >> + spin_unlock_irqrestore(&mgbdev->vout[i]->vdev.fh_lock, >> + flags_out[i]); >> + for (i = MGB4_VIN_DEVICES - 1; i >= 0; i--) >> + if (mgbdev->vin[i]) >> + spin_unlock_irqrestore(&mgbdev->vin[i]->vdev.fh_lock, >> + flags_in[i]); >> + >> + return ret; >> +} >
| |