| Message ID | 20230705213010.390849-19-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | f663fb4967b0e428f80f7850090ed3ba3bff0e3b |
| Headers | show |
| Series | media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand |
On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote: > Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from > the v4l2-async bound op so that an I2C-client will be instatiated for > the VCM. > > Note unfortunately on atomisp the _DSM to get the VCM type sometimes > returns a VCM even though there is none. Since VCMs are typically only > used together with certain sensors, work around this by adding a vcm > field to atomisp_sensor_config and only check for a VCM when that is set. ... > +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev) > +{ > + union acpi_object *obj; > + char *vcm_type; > + > + obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0, > + NULL, ACPI_TYPE_STRING); > + if (!obj) > + return NULL; > + > + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); Where is the counterpart kfree()? > + ACPI_FREE(obj); > + > + if (!vcm_type) > + return NULL; > + > + string_lower(vcm_type, vcm_type); > + return vcm_type; > +}
Hi, On 7/6/23 12:15, Andy Shevchenko wrote: > On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote: >> Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from >> the v4l2-async bound op so that an I2C-client will be instatiated for >> the VCM. >> >> Note unfortunately on atomisp the _DSM to get the VCM type sometimes >> returns a VCM even though there is none. Since VCMs are typically only >> used together with certain sensors, work around this by adding a vcm >> field to atomisp_sensor_config and only check for a VCM when that is set. > > ... > >> +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev) >> +{ >> + union acpi_object *obj; >> + char *vcm_type; >> + >> + obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0, >> + NULL, ACPI_TYPE_STRING); >> + if (!obj) >> + return NULL; >> + >> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); > > Where is the counterpart kfree()? The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge code only creates those once and them leaves them in memory, even on a rmmod. So this is deliberately leaked just like that the ipu_bridge struct which contains all the swnode-s is deliberately leaked by ipu-bridge.c Regards, Hans > >> + ACPI_FREE(obj); >> + >> + if (!vcm_type) >> + return NULL; >> + >> + string_lower(vcm_type, vcm_type); >> + return vcm_type; >> +} >
On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote: > On 7/6/23 12:15, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote: > >> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); > > > > Where is the counterpart kfree()? > > The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge > code only creates those once and them leaves them in memory, even on > a rmmod. So this is deliberately leaked just like that the ipu_bridge > struct which contains all the swnode-s is deliberately leaked by > ipu-bridge.c Should we worry about those leakages?
Hi, On 7/6/23 14:42, Andy Shevchenko wrote: > On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote: >> On 7/6/23 12:15, Andy Shevchenko wrote: >>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote: > >>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); >>> >>> Where is the counterpart kfree()? >> >> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge >> code only creates those once and them leaves them in memory, even on >> a rmmod. So this is deliberately leaked just like that the ipu_bridge >> struct which contains all the swnode-s is deliberately leaked by >> ipu-bridge.c > > Should we worry about those leakages? No this is by design because removing the swnodes while e.g. a sensor driver might still be bound to the i2c-client is trouble-some and the callers of ipu_bridge_init check if it has already run and then skip calling it. So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver ipu_bridge_init() will not get called a second time. Instead the old swnodes (1) which are already set as secondary fwnodes for the sensor and bridge devices are re-used. Regards, Hans 1) + the properties they contain
On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote: > On 7/6/23 14:42, Andy Shevchenko wrote: > > On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote: > >> On 7/6/23 12:15, Andy Shevchenko wrote: > >>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote: > > > >>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); > >>> > >>> Where is the counterpart kfree()? > >> > >> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge > >> code only creates those once and them leaves them in memory, even on > >> a rmmod. So this is deliberately leaked just like that the ipu_bridge > >> struct which contains all the swnode-s is deliberately leaked by > >> ipu-bridge.c > > > > Should we worry about those leakages? > > No this is by design because removing the swnodes while e.g. a sensor > driver might still be bound to the i2c-client is trouble-some and > the callers of ipu_bridge_init check if it has already run and then > skip calling it. > > So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver > ipu_bridge_init() will not get called a second time. Instead > the old swnodes (1) which are already set as secondary fwnodes for > the sensor and bridge devices are re-used. But this will be actual leak if we hot unplug/plug back the device, right? (I think we can do that in some [debug?] cases). Whatever, it's out of scope of this series... > 1) + the properties they contain
Hi, On 7/6/23 14:56, Andy Shevchenko wrote: > On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote: >> On 7/6/23 14:42, Andy Shevchenko wrote: >>> On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote: >>>> On 7/6/23 12:15, Andy Shevchenko wrote: >>>>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote: >>> >>>>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); >>>>> >>>>> Where is the counterpart kfree()? >>>> >>>> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge >>>> code only creates those once and them leaves them in memory, even on >>>> a rmmod. So this is deliberately leaked just like that the ipu_bridge >>>> struct which contains all the swnode-s is deliberately leaked by >>>> ipu-bridge.c >>> >>> Should we worry about those leakages? >> >> No this is by design because removing the swnodes while e.g. a sensor >> driver might still be bound to the i2c-client is trouble-some and >> the callers of ipu_bridge_init check if it has already run and then >> skip calling it. >> >> So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver >> ipu_bridge_init() will not get called a second time. Instead >> the old swnodes (1) which are already set as secondary fwnodes for >> the sensor and bridge devices are re-used. > > But this will be actual leak if we hot unplug/plug back the device, right? > (I think we can do that in some [debug?] cases). No, the kstrdup() is called from the parse_sensor_fwnode() callback passed to ipu_bridge_init(), so it will only happen once even on unbind + re-bind or rmmod + modprobe. Regards, Hans
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c index 8124be486e2e..92693206e4ca 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c @@ -66,15 +66,25 @@ static const guid_t atomisp_dsm_guid = GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d, 0x97, 0xb9, 0x88, 0x2a, 0x68, 0x60, 0xa4, 0xbe); +/* + * 75c9a639-5c8a-4a00-9f48-a9c3b5da789f + * This _DSM GUID returns a string giving the VCM type e.g. "AD5823". + */ +static const guid_t vcm_dsm_guid = + GUID_INIT(0x75c9a639, 0x5c8a, 0x4a00, + 0x9f, 0x48, 0xa9, 0xc3, 0xb5, 0xda, 0x78, 0x9f); + struct atomisp_sensor_config { int lanes; + bool vcm; }; -#define ATOMISP_SENSOR_CONFIG(_HID, _LANES) \ +#define ATOMISP_SENSOR_CONFIG(_HID, _LANES, _VCM) \ { \ .id = _HID, \ .driver_data = (long)&((const struct atomisp_sensor_config) { \ .lanes = _LANES, \ + .vcm = _VCM, \ }) \ } @@ -490,8 +500,28 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) return ret; } +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev) +{ + union acpi_object *obj; + char *vcm_type; + + obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0, + NULL, ACPI_TYPE_STRING); + if (!obj) + return NULL; + + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL); + ACPI_FREE(obj); + + if (!vcm_type) + return NULL; + + string_lower(vcm_type, vcm_type); + return vcm_type; +} + static const struct acpi_device_id atomisp_sensor_configs[] = { - ATOMISP_SENSOR_CONFIG("INT33BE", 2), /* OV5693 */ + ATOMISP_SENSOR_CONFIG("INT33BE", 2, true), /* OV5693 */ {} }; @@ -500,6 +530,7 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev, { const struct acpi_device_id *id; int ret, clock_num; + bool vcm = false; int lanes = 1; id = acpi_match_acpi_device(atomisp_sensor_configs, adev); @@ -508,6 +539,7 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev, (struct atomisp_sensor_config *)id->driver_data; lanes = cfg->lanes; + vcm = cfg->vcm; } /* @@ -545,6 +577,9 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev, sensor->orientation = (sensor->link == 1) ? V4L2_FWNODE_ORIENTATION_BACK : V4L2_FWNODE_ORIENTATION_FRONT; + if (vcm) + sensor->vcm_type = atomisp_csi2_get_vcm_type(adev); + return 0; } @@ -583,6 +618,7 @@ static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier, { struct atomisp_device *isp = notifier_to_atomisp(notifier); struct sensor_async_subdev *s_asd = to_sensor_asd(asd); + int ret; if (s_asd->port >= ATOMISP_CAMERA_NR_PORTS) { dev_err(isp->dev, "port %d not supported\n", s_asd->port); @@ -594,6 +630,10 @@ static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier, return -EBUSY; } + ret = ipu_bridge_instantiate_vcm(sd->dev); + if (ret) + return ret; + isp->sensor_subdevs[s_asd->port] = sd; return 0; }
Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from the v4l2-async bound op so that an I2C-client will be instatiated for the VCM. Note unfortunately on atomisp the _DSM to get the VCM type sometimes returns a VCM even though there is none. Since VCMs are typically only used together with certain sensors, work around this by adding a vcm field to atomisp_sensor_config and only check for a VCM when that is set. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../media/atomisp/pci/atomisp_csi2_bridge.c | 44 ++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-)