[v3,18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

Message ID 20230705213010.390849-19-hdegoede@redhat.com (mailing list archive)
State Accepted
Delegated to: Sakari Ailus
Headers
Series media: ipu-bridge: Shared with atomisp, rework VCM instantiation |

Commit Message

Hans de Goede July 5, 2023, 9:30 p.m. UTC
  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(-)
  

Comments

Andy Shevchenko July 6, 2023, 10:15 a.m. UTC | #1
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;
> +}
  
Hans de Goede July 6, 2023, 12:31 p.m. UTC | #2
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;
>> +}
>
  
Andy Shevchenko July 6, 2023, 12:42 p.m. UTC | #3
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?
  
Hans de Goede July 6, 2023, 12:47 p.m. UTC | #4
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
  
Andy Shevchenko July 6, 2023, 12:56 p.m. UTC | #5
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
  
Hans de Goede July 6, 2023, 12:58 p.m. UTC | #6
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
  

Patch

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;
 }