Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation

[Daniel Scally <djrscally@gmail.com>]

Hi Fabian, Jacopo

On 07/04/2021 14:19, Fabian Wüthrich wrote:
> That's a good point but I'm also not experienced enough to answer this question.
>
> It looks like that these SSDB buffers are device specific and every vendor has its
> own memory layout. So parsing these in the core could be difficult.
>
> Maybe Dan knows more...


I think the cio2-bridge is the right place to handle it; as far as we
know that buffer only appears on Intel skl/kbl devices that have an IPU3
for the cameras to connect to. To say it's all a bit outside the normal
ACPI specifications would be an understatement, so this isn't a solution
for ACPI generally but rather just for a few specific product ranges.

> On 07.04.21 14:41, Jacopo Mondi wrote:
>> Hi Fabian,
>>
>> On Wed, Apr 07, 2021 at 01:52:31PM +0200, Fabian Wüthrich wrote:
>>>
>>> On 07.04.21 10:22, Jacopo Mondi wrote:
>>>> Hi Fabian,
>>>>
>>>> On Sun, Mar 21, 2021 at 08:11:56PM +0100, Fabian Wüthrich wrote:
>>>>> The sensor orientation is read from the _PLC ACPI buffer and converted
>>>>> to a v4l2 format.
>>>>>
>>>>> See https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
>>>>> page 351 for a definition of the Panel property.
>>>>>
>>>>> The sensor rotation is read from the SSDB ACPI buffer and converted into
>>>>> degrees.
>>>>>
>>>> The framework has v4l2_fwnode_device_parse() in v4l2-fwnode.c which
>>>> works for DT based systems. Could support for retreiving those
>>>> properties from the SSDB block be added there ?
>>>>
>>>> Thanks
>>>>   j
>>>>
>>> This is exactly the purpose of this patch. I've should have been more precise
>>> in the commit message but basically this patch converts the properties from
>>> the SSDB block into fwnodes which are then picked up by v4l2_fwnode_device_parse().
>> Ah, right! The cio2_bridge_create_fwnode_properties() should have
>> hinted me... My question was more on the lines of: "Instead of parsing
>> the SSDB block in the CIO2 driver to translate into v4l2-fwnode
>> consumable properties, can the parsing be done directly into the
>> core" due to my poor understanding of the swnode infrastructure.
>>
>> Thanks for clarifying
>>
>>> P.S. I have a quick question below
>>>
>>>>> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
>>>>> ---
>>>>>  drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
>>>>>  drivers/media/pci/intel/ipu3/cio2-bridge.h | 16 ++++++
>>>>>  2 files changed, 72 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>>>>> index c2199042d3db..503809907b92 100644
>>>>> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
>>>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>>>>> @@ -29,6 +29,7 @@ static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>>  static const struct cio2_property_names prop_names = {
>>>>>  	.clock_frequency = "clock-frequency",
>>>>>  	.rotation = "rotation",
>>>>> +	.orientation = "orientation",
>>>>>  	.bus_type = "bus-type",
>>>>>  	.data_lanes = "data-lanes",
>>>>>  	.remote_endpoint = "remote-endpoint",
>>>>> @@ -72,11 +73,51 @@ static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
>>>>>  	return ret;
>>>>>  }
>>>>>
>>>>> +static u32 cio2_bridge_parse_rotation(struct cio2_sensor *sensor)
>>>>> +{
>>>>> +	switch (sensor->ssdb.degree) {
>>>>> +	case CIO2_SENSOR_ROTATION_NORMAL:
>>>>> +		return 0;
>>>>> +	case CIO2_SENSOR_ROTATION_INVERTED:
>>>>> +		return 180;
>>>>> +	default:
>>>>> +		dev_warn(&sensor->adev->dev,
>>>>> +			 "Unknown rotation %d. Assume 0 degree rotation\n",
>>>>> +			 sensor->ssdb.degree);
>>>>> +		return 0;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static enum v4l2_fwnode_orientation cio2_bridge_parse_orientation(struct cio2_sensor *sensor)
>>>>> +{
>>>>> +	switch (sensor->pld->panel) {
>>>>> +	case CIO2_PLD_PANEL_FRONT:
>>>>> +		return V4L2_FWNODE_ORIENTATION_FRONT;
>>>>> +	case CIO2_PLD_PANEL_BACK:
>>>>> +		return V4L2_FWNODE_ORIENTATION_BACK;
>>>>> +	case CIO2_PLD_PANEL_TOP:
>>>>> +	case CIO2_PLD_PANEL_LEFT:
>>>>> +	case CIO2_PLD_PANEL_RIGHT:
>>>>> +	case CIO2_PLD_PANEL_UNKNOWN:
>>>>> +		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
>>>>> +	default:
>>>>> +		dev_warn(&sensor->adev->dev, "Unknown _PLD panel value %d\n",
>>>>> +			 sensor->pld->panel);
>>>>> +		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
>>>>> +	}
>>>>> +}
>>> These constants are ACPI related and shouldn't be here. Should I move them to e.g.
>>> include/acpi/acbuffer.h or do you know a better place?
>>>
>> I know very few things about ACPI, don't think I'm the right person to
>> answer that question :)
>>
>> Although as I read the definitions of ACPI_PLD_GET_ROTATION() and
>> ACPI_PLD_GET_PANEL() and see they match the bit offset as reported by
>> the ACPI specs you linked here above, I would say your suggestion
>> makes sense, but please confirm with someone that knows better :)
>>
>> Thanks
>>   j
>>
>>>>> +
>>>>>  static void cio2_bridge_create_fwnode_properties(
>>>>>  	struct cio2_sensor *sensor,
>>>>>  	struct cio2_bridge *bridge,
>>>>>  	const struct cio2_sensor_config *cfg)
>>>>>  {
>>>>> +	u32 rotation;
>>>>> +	enum v4l2_fwnode_orientation orientation;
>>>>> +
>>>>> +	rotation = cio2_bridge_parse_rotation(sensor);
>>>>> +	orientation = cio2_bridge_parse_orientation(sensor);
>>>>> +
>>>>>  	sensor->prop_names = prop_names;
>>>>>
>>>>>  	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
>>>>> @@ -85,9 +126,12 @@ static void cio2_bridge_create_fwnode_properties(
>>>>>  	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
>>>>>  					sensor->prop_names.clock_frequency,
>>>>>  					sensor->ssdb.mclkspeed);
>>>>> -	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(
>>>>> +	sensor->dev_properties[1] = PROPERTY_ENTRY_U32(
>>>>>  					sensor->prop_names.rotation,
>>>>> -					sensor->ssdb.degree);
>>>>> +					rotation);
>>>>> +	sensor->dev_properties[2] = PROPERTY_ENTRY_U32(
>>>>> +					sensor->prop_names.orientation,
>>>>> +					orientation);
>>>>>
>>>>>  	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
>>>>>  					sensor->prop_names.bus_type,
>>>>> @@ -159,6 +203,7 @@ static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
>>>>>  	for (i = 0; i < bridge->n_sensors; i++) {
>>>>>  		sensor = &bridge->sensors[i];
>>>>>  		software_node_unregister_nodes(sensor->swnodes);
>>>>> +		ACPI_FREE(sensor->pld);
>>>>>  		acpi_dev_put(sensor->adev);
>>>>>  	}
>>>>>  }
>>>>> @@ -170,6 +215,7 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>>>>>  	struct fwnode_handle *fwnode;
>>>>>  	struct cio2_sensor *sensor;
>>>>>  	struct acpi_device *adev;
>>>>> +	acpi_status status;
>>>>>  	int ret;
>>>>>
>>>>>  	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
>>>>> @@ -193,11 +239,15 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>>>>>  		if (ret)
>>>>>  			goto err_put_adev;
>>>>>
>>>>> +		status = acpi_get_physical_device_location(adev->handle, &sensor->pld);
>>>>> +		if (ACPI_FAILURE(status))
>>>>> +			goto err_put_adev;
>>>>> +
>>>>>  		if (sensor->ssdb.lanes > CIO2_MAX_LANES) {
>>>>>  			dev_err(&adev->dev,
>>>>>  				"Number of lanes in SSDB is invalid\n");
>>>>>  			ret = -EINVAL;
>>>>> -			goto err_put_adev;
>>>>> +			goto err_free_pld;
>>>>>  		}
>>>>>
>>>>>  		cio2_bridge_create_fwnode_properties(sensor, bridge, cfg);
>>>>> @@ -205,7 +255,7 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>>>>>
>>>>>  		ret = software_node_register_nodes(sensor->swnodes);
>>>>>  		if (ret)
>>>>> -			goto err_put_adev;
>>>>> +			goto err_free_pld;
>>>>>
>>>>>  		fwnode = software_node_fwnode(&sensor->swnodes[
>>>>>  						      SWNODE_SENSOR_HID]);
>>>>> @@ -226,6 +276,8 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>>>>>
>>>>>  err_free_swnodes:
>>>>>  	software_node_unregister_nodes(sensor->swnodes);
>>>>> +err_free_pld:
>>>>> +	ACPI_FREE(sensor->pld);
>>>>>  err_put_adev:
>>>>>  	acpi_dev_put(sensor->adev);
>>>>>  err_out:
>>>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>>>> index dd0ffcafa489..e1e388cc9f45 100644
>>>>> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>>>> @@ -12,6 +12,19 @@
>>>>>  #define CIO2_MAX_LANES				4
>>>>>  #define MAX_NUM_LINK_FREQS			3
>>>>>
>>>>> +/* Values are estimated guesses as we don't have a spec */
>>>>> +#define CIO2_SENSOR_ROTATION_NORMAL		0
>>>>> +#define CIO2_SENSOR_ROTATION_INVERTED		1
>>>>> +
>>>>> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
>>>>> +#define CIO2_PLD_PANEL_TOP			0
>>>>> +#define CIO2_PLD_PANEL_BOTTOM			1
>>>>> +#define CIO2_PLD_PANEL_LEFT			2
>>>>> +#define CIO2_PLD_PANEL_RIGHT			3
>>>>> +#define CIO2_PLD_PANEL_FRONT			4
>>>>> +#define CIO2_PLD_PANEL_BACK			5
>>>>> +#define CIO2_PLD_PANEL_UNKNOWN			6
>>>>> +
>>>>>  #define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
>>>>>  	(const struct cio2_sensor_config) {	\
>>>>>  		.hid = _HID,			\
>>>>> @@ -80,6 +93,7 @@ struct cio2_sensor_ssdb {
>>>>>  struct cio2_property_names {
>>>>>  	char clock_frequency[16];
>>>>>  	char rotation[9];
>>>>> +	char orientation[12];
>>>>>  	char bus_type[9];
>>>>>  	char data_lanes[11];
>>>>>  	char remote_endpoint[16];
>>>>> @@ -106,6 +120,8 @@ struct cio2_sensor {
>>>>>  	struct cio2_node_names node_names;
>>>>>
>>>>>  	struct cio2_sensor_ssdb ssdb;
>>>>> +	struct acpi_pld_info *pld;
>>>>> +
>>>>>  	struct cio2_property_names prop_names;
>>>>>  	struct property_entry ep_properties[5];
>>>>>  	struct property_entry dev_properties[3];
>>>>> --
>>>>> 2.31.0
>>>>>