Skip to content

ipu3: Image rotation support #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fabwu merged 3 commits into from
Jan 25, 2021
Merged

ipu3: Image rotation support #79

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c644c4
cio2-bridge: Parse sensor orientation and rotation
fabwu Jan 22, 2021
0eec1db
ov5693: Add orientation and rotation controls
fabwu Jan 22, 2021
7f3095d
cio2-bridge: Use macros and add warnings
fabwu Jan 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions drivers/media/i2c/ov5693.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <linux/i2c.h>
#include <linux/moduleparam.h>
#include <media/v4l2-device.h>
#include <media/v4l2-fwnode.h>
#include <linux/io.h>
#include <linux/acpi.h>
#include <linux/regulator/consumer.h>
Expand Down Expand Up @@ -1608,6 +1609,7 @@ static int ov5693_init_controls(struct ov5693_device *ov5693)
{
struct i2c_client *client = v4l2_get_subdevdata(&ov5693->sd);
const struct v4l2_ctrl_ops *ops = &ov5693_ctrl_ops;
struct v4l2_fwnode_device_properties props;
struct v4l2_ctrl *ctrl;
unsigned int i;
int ret;
Expand Down Expand Up @@ -1663,6 +1665,15 @@ static int ov5693_init_controls(struct ov5693_device *ov5693)
if (ov5693->hblank)
ov5693->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;

/* set properties from fwnode (e.g. rotation, orientation) */
ret = v4l2_fwnode_device_parse(&client->dev, &props);
if (ret)
return ret;

ret = v4l2_ctrl_new_fwnode_properties(&ov5693->ctrl_handler, ops, &props);
if (ret)
return ret;

/* Use same lock for controls as for everything else. */
ov5693->ctrl_handler.lock = &ov5693->input_lock;
ov5693->sd.ctrl_handler = &ov5693->ctrl_handler;
Expand Down
45 changes: 41 additions & 4 deletions drivers/media/pci/intel/ipu3/cio2-bridge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -72,11 +73,36 @@ static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
return ret;
}

static u32 cio2_bridge_parse_rotation(u8 rotation)
{
if (rotation == 1)
return 180;
return 0;
}

static enum v4l2_fwnode_orientation cio2_bridge_parse_orientation(u8 panel)
{
switch (panel) {
case 4:
return V4L2_FWNODE_ORIENTATION_FRONT;
case 5:
return V4L2_FWNODE_ORIENTATION_BACK;
default:
return V4L2_FWNODE_ORIENTATION_EXTERNAL;
}
}

@qzed qzed Jan 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any spec for this, right? Maybe print a warning or debug for any unknown value?

@fabwu fabwu Jan 22, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rotation is reverse engineered so 1 == 180 is just a guess but a comment is indeed necessary.

For the orientation we have this spec on page 351 so I'll add a comment for that as well.

What do you think about the switch-statement? Is there a better way of doing the conversion?

@djrscally djrscally Jan 22, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd probably find / need to add macros for the 4/5 values for the _PLD panel field when this goes upstream. The switch itself is fine imo.

I'm surprised to find 1==180, as it sort of precludes a sensor being mounted at 90 degrees or something...I was expecting something like 1 = 90 degrees, 2 = 180. Anyway, I think that probably ought to return the input value in the case that it's not 1, otherwise if we do find some other value in there it'll be masked here.

@qzed qzed Jan 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the orientation we have this spec on page 351 so I'll add a comment for that as well.

Thanks for the link! Maybe mention that this value comes from the ACPI PLD buffer, then people can find the spec for that themselves. Also since anything except front and back seem to be directly translatable from ACPI, I think you should go with a warning here (as opposed to a debug print).

I think you'd probably find / need to add macros for the 4/5 values for the _PLD panel field when this goes upstream. The switch itself is fine imo.

Yeah, I think those should be constants if they're defined in ACPI. Maybe also for the rotation.

What do you think about the switch-statement? Is there a better way of doing the conversion?

I agree with @djrscally. I think switch is probably the best option here. A lookup-table would be an alternative, but doesn't make much sense here, I think.

I'm surprised to find 1==180, as it sort of precludes a sensor being mounted at 90 degrees or something...I was expecting something like 1 = 90 degrees, 2 = 180.

I assume we don't know any device that has a 90 degree sensor so we could check this?

Anyway, I think that probably ought to return the input value in the case that it's not 1, otherwise if we do find some other value in there it'll be masked here.

I'm not entirely sure if returning the input value is a good thing, but there should be something to help us figure out if there's an unknown value (that's why I proposed warning or debug messages).

@djrscally djrscally Jan 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also for the rotation.

Yeah, they could just got in cio2-bridge.h

I assume we don't know any device that has a 90 degree sensor so we could check this?

Not to my knowledge, I don't recall anything except a 180 rotation, I was just going off what Windows apparently expects from the rotation field in _PLD: https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/camera-device-orientation.

Then again, given the field is called "degree" you'd really expect it to be in degrees if anything!

EDIT: In fact, honestly, the fact that 1 = 180 degree rotation means this has to be a binary field. You could just make that function something along the lines of:

#define CIO2_SENSOR_ROTATION_INVERTED        1

...


static u32 cio2_bridge_parse_rotation(u8 rotation)
{
	return rotation == CIO2_SENSOR_ROTATION_INVERTED ? 180 : 0;
}

I'm not entirely sure if returning the input value is a good thing, but there should be something to help us figure out if there's an unknown value (that's why I proposed warning or debug messages).

Fair enough; a debug print is good for me too then

@qzed qzed Jan 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then again, given the field is called "degree" you'd really expect it to be in degrees if anything!

Yeah... let's just hope the 0/1 thing is consistent for now and we don't end up with other devices actually having a rotation in degrees there. Might be a bit messy handling that.

@fabwu fabwu Jan 23, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also strange that degree is only 8-bit so it cannot encode e.g. 270 degree directly.

I'll try to go through the SSDB buffers and find all values for degree then we have a better picture.

@fabwu fabwu Jan 24, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grepped the ACPI info from here and most of the sensors have 0 or 1 in the degree field (SSDB) and 0 in the roation field (PLD).

There are some quirks though:

  • Switch_SA5-271 has the degree field set to 180 (yey!)
  • Surface_Book_3 and Surface_Pro_7 report 2 (i.e. 90 degree) in the PLD roation field for the IR-Sensor

I think it's save to treat degree as a "binary" variable for now and treat the three devices separately.

@qzed qzed Jan 24, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some quirks though:

  • Switch_SA5-271 has the degree field set to 180 (yey!)

Whelp, of course there are... would be too easy otherwise, would it. I think we can assume though that it's either 0, 1, 2, (maybe 3?) and 90, 180. So still possible to somewhat sanely handle those.

  • Surface_Book_3 and Surface_Pro_7 report 2 (i.e. 90 degree) in the PLD roation field for the IR-Sensor

Hmm, too bad those are IPU4 devices. But at least we now know that we can maybe check these values in the future.

@djrscally djrscally Jan 24, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some quirks though:

Switch_SA5-271 has the degree field set to 180 (yey!)

Oh good grief!

I think it's save to treat degree as a "binary" variable for now and treat the three devices separately.

Yeah, sounds sensible to me.

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->ssdb.degree);
orientation = cio2_bridge_parse_orientation(sensor->pld->panel);

sensor->prop_names = prop_names;

sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
Expand All @@ -85,9 +111,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,
Expand Down Expand Up @@ -159,6 +188,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);
}
}
Expand All @@ -170,6 +200,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) {
Expand All @@ -193,19 +224,23 @@ 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);
cio2_bridge_create_connection_swnodes(bridge, sensor);

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]);
if (!fwnode) {
Expand All @@ -225,6 +260,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:
Expand Down
3 changes: 3 additions & 0 deletions drivers/media/pci/intel/ipu3/cio2-bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,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];
Expand All @@ -106,6 +107,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];
Expand Down