Hi Folks,
This is a new revision of
https://lore.kernel.org/linux-acpi/13276375.uLZWGnKmhe@kreacher/
which was reported to have issues and it took time to revisit it.
> The main points from the original cover letter are still valid:
>
> The general idea is the same - CSI-2 resource descriptors, introduced in
> ACPI 6.4 and defined by
>
> https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#camera-serial-i
> nterface-csi-2-connection-resource-descriptor
>
> are found and used for creating a set of software nodes that represent the
> CSI-2 connection graph.
>
> These software nodes need to be available before any scan handlers or ACPI
> drivers are bound to any struct acpi_device objects, so all of that is done
> at the early stage of ACPI device enumeration, but unnecessary ACPI
> namespace walks are avoided.
>
> The CSI-2 software nodes are populated with data extracted from the CSI-2
> resource descriptors themselves and from device properties defined by the
> MIPI DiSco for Imaging specification (see
> https://www.mipi.org/specifications/mipi-disco-imaging).
>
> Patches [4,6/6] come from the original series directly, but the other
> patches have been changes substantially, so I've decided to re-start patch
> series versioning from scratch.
The v2 addresses at least 3 issues found in the v1 by code inspection:
* A port_count field incrementation was missing in acpi_mipi_scan_crs_csi2(),
so its value for all of the devices having CSI2 resources in _CRS was always
1 (and it should be equal to the number of valid CSI2 connection resources).
* Some acpi_mipi_crs_csi2_list members could be freed prematurely, so they were
inaccessible when extract_crs_csi2_conn_info() attempted to access them.
* A check of remote_swnodes() against NULL was missing, which could result in
a crash in a case when the swnodes memory could not be allocated for some
acpi_mipi_crs_csi2_list entries.
Apart from that, it rearranges the code somewhat to make it easier to follow
and to avoid premature freeing of memory in it in general and the new file
added by it is now called mipi-di.c (instead of mipi-disco-imaging.c) for
compactness.
The series is based on current linux-next.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Use the connection information extracted from the _CRS CSI-2 resource
descriptors for all devices that have them to populate port names and the
"reg", "bus-type" and "remote-endpoint" properties in the software nodes
representing the CSI-2 connection graph.
Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#camera-serial-interface-csi-2-connection-resource-descriptor
Co-developed-by: Sakari Ailus <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/mipi-di.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++-
include/acpi/acpi_bus.h | 53 ++++++++++++++++
2 files changed, 205 insertions(+), 1 deletion(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -366,8 +366,61 @@ struct acpi_device_data {
struct acpi_gpio_mapping;
+#define ACPI_DEVICE_CSI2_DATA_LANES 4
+
+#define ACPI_DEVICE_SWNODE_PORT_NAME_LENGTH 8
+
+enum acpi_device_swnode_port_props {
+ ACPI_DEVICE_SWNODE_PORT_REG,
+ ACPI_DEVICE_SWNODE_PORT_NUM_OF,
+ ACPI_DEVICE_SWNODE_PORT_NUM_ENTRIES
+};
+
+enum acpi_device_swnode_ep_props {
+ ACPI_DEVICE_SWNODE_EP_REMOTE_EP,
+ ACPI_DEVICE_SWNODE_EP_BUS_TYPE,
+ ACPI_DEVICE_SWNODE_EP_REG,
+ ACPI_DEVICE_SWNODE_EP_CLOCK_LANES,
+ ACPI_DEVICE_SWNODE_EP_DATA_LANES,
+ ACPI_DEVICE_SWNODE_EP_LANE_POLARITIES,
+ /* TX only */
+ ACPI_DEVICE_SWNODE_EP_LINK_FREQUENCIES,
+ ACPI_DEVICE_SWNODE_EP_NUM_OF,
+ ACPI_DEVICE_SWNODE_EP_NUM_ENTRIES
+};
+
+/*
+ * Each device has a root software node plus two times as many nodes as the
+ * number of CSI-2 ports.
+ */
+#define ACPI_DEVICE_SWNODE_PORT(port) (2 * (port) + 1)
+#define ACPI_DEVICE_SWNODE_EP(endpoint) \
+ (ACPI_DEVICE_SWNODE_PORT(endpoint) + 1)
+
+/**
+ * struct acpi_device_software_node_port - MIPI DisCo for Imaging CSI-2 port
+ * @port_name: Port name.
+ * @data_lanes: "data-lanes" property values.
+ * @lane_polarities: "lane-polarities" property values.
+ * @link_frequencies: "link_frequencies" property values.
+ * @port_nr: Port number.
+ * @crs_crs2_local: _CRS CSI2 record present (i.e. this is a transmitter one).
+ * @port_props: Port properties.
+ * @ep_props: Endpoint properties.
+ * @remote_ep: Reference to the remote endpoint.
+ */
struct acpi_device_software_node_port {
+ char port_name[ACPI_DEVICE_SWNODE_PORT_NAME_LENGTH + 1];
+ u32 data_lanes[ACPI_DEVICE_CSI2_DATA_LANES];
+ u32 lane_polarities[ACPI_DEVICE_CSI2_DATA_LANES + 1 /* clock lane */];
+ u64 link_frequencies[ACPI_DEVICE_CSI2_DATA_LANES];
unsigned int port_nr;
+ bool crs_csi2_local;
+
+ struct property_entry port_props[ACPI_DEVICE_SWNODE_PORT_NUM_ENTRIES];
+ struct property_entry ep_props[ACPI_DEVICE_SWNODE_EP_NUM_ENTRIES];
+
+ struct software_node_ref_args remote_ep[1];
};
/**
Index: linux-pm/drivers/acpi/mipi-di.c
===================================================================
--- linux-pm.orig/drivers/acpi/mipi-di.c
+++ linux-pm/drivers/acpi/mipi-di.c
@@ -23,6 +23,8 @@
#include <linux/slab.h>
#include <linux/string.h>
+#include <media/v4l2-fwnode.h>
+
#include "internal.h"
static LIST_HEAD(acpi_mipi_crs_csi2_list);
@@ -240,6 +242,142 @@ overflow:
port_count);
}
+#define ACPI_CRS_CSI2_PHY_TYPE_C 0
+#define ACPI_CRS_CSI2_PHY_TYPE_D 1
+
+static unsigned int next_csi2_port_index(struct acpi_device_software_nodes *swnodes,
+ unsigned int port_nr)
+{
+ unsigned int i;
+
+ for (i = 0; i < swnodes->num_ports; i++) {
+ struct acpi_device_software_node_port *port = &swnodes->ports[i];
+
+ if (port->port_nr == port_nr)
+ return i;
+
+ if (port->port_nr == NO_CSI2_PORT) {
+ port->port_nr = port_nr;
+ return i;
+ }
+ }
+
+ return NO_CSI2_PORT;
+}
+
+/* Print graph port name into a buffer, return non-zero on failure. */
+#define GRAPH_PORT_NAME(var, num) \
+ (snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) >= \
+ sizeof(var))
+
+static void extract_crs_csi2_conn_info(acpi_handle local_handle,
+ struct acpi_device_software_nodes *local_swnodes,
+ struct crs_csi2_connection *conn)
+{
+ struct crs_csi2 *remote_csi2 = acpi_mipi_get_crs_csi2(conn->remote_handle);
+ struct acpi_device_software_nodes *remote_swnodes;
+ struct acpi_device_software_node_port *local_port, *remote_port;
+ struct software_node *local_node, *remote_node;
+ unsigned int local_index, remote_index;
+ unsigned int bus_type;
+
+ /*
+ * If the previous steps have failed to make room for a _CRS CSI-2
+ * representation for the remote end of the given connection, skip it.
+ */
+ if (!remote_csi2)
+ return;
+
+ remote_swnodes = remote_csi2->swnodes;
+ if (!remote_swnodes)
+ return;
+
+ switch (conn->csi2_data.phy_type) {
+ case ACPI_CRS_CSI2_PHY_TYPE_C:
+ bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_CPHY;
+ break;
+
+ case ACPI_CRS_CSI2_PHY_TYPE_D:
+ bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_DPHY;
+ break;
+
+ default:
+ acpi_handle_info(local_handle, "unknown CSI-2 PHY type %u\n",
+ conn->csi2_data.phy_type);
+ return;
+ }
+
+ local_index = next_csi2_port_index(local_swnodes,
+ conn->csi2_data.local_port_instance);
+ if (WARN_ON_ONCE(local_index >= local_swnodes->num_ports))
+ return;
+
+ remote_index = next_csi2_port_index(remote_swnodes,
+ conn->csi2_data.resource_source.index);
+ if (WARN_ON_ONCE(remote_index >= remote_swnodes->num_ports))
+ return;
+
+ local_port = &local_swnodes->ports[local_index];
+ local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
+ local_port->crs_csi2_local = true;
+
+ remote_port = &remote_swnodes->ports[remote_index];
+ remote_node = &remote_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(remote_index)];
+
+ local_port->remote_ep[0] = SOFTWARE_NODE_REFERENCE(remote_node);
+ remote_port->remote_ep[0] = SOFTWARE_NODE_REFERENCE(local_node);
+
+ local_port->ep_props[ACPI_DEVICE_SWNODE_EP_REMOTE_EP] =
+ PROPERTY_ENTRY_REF_ARRAY("remote-endpoint",
+ local_port->remote_ep);
+
+ local_port->ep_props[ACPI_DEVICE_SWNODE_EP_BUS_TYPE] =
+ PROPERTY_ENTRY_U32("bus-type", bus_type);
+
+ local_port->ep_props[ACPI_DEVICE_SWNODE_EP_REG] =
+ PROPERTY_ENTRY_U32("reg", 0);
+
+ local_port->port_props[ACPI_DEVICE_SWNODE_PORT_REG] =
+ PROPERTY_ENTRY_U32("reg", conn->csi2_data.local_port_instance);
+
+ if (GRAPH_PORT_NAME(local_port->port_name,
+ conn->csi2_data.local_port_instance))
+ acpi_handle_info(local_handle, "local port %u name too long",
+ conn->csi2_data.local_port_instance);
+
+ remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_REMOTE_EP] =
+ PROPERTY_ENTRY_REF_ARRAY("remote-endpoint",
+ remote_port->remote_ep);
+
+ remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_BUS_TYPE] =
+ PROPERTY_ENTRY_U32("bus-type", bus_type);
+
+ remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_REG] =
+ PROPERTY_ENTRY_U32("reg", 0);
+
+ remote_port->port_props[ACPI_DEVICE_SWNODE_PORT_REG] =
+ PROPERTY_ENTRY_U32("reg", conn->csi2_data.resource_source.index);
+
+ if (GRAPH_PORT_NAME(remote_port->port_name,
+ conn->csi2_data.resource_source.index))
+ acpi_handle_info(local_handle, "remote port %u name too long",
+ conn->csi2_data.resource_source.index);
+}
+
+static void prepare_crs_csi2_swnodes(struct crs_csi2 *csi2)
+{
+ struct acpi_device_software_nodes *local_swnodes = csi2->swnodes;
+ acpi_handle local_handle = csi2->handle;
+ struct crs_csi2_connection *conn;
+
+ /* Bail out if the allocation of swnodes has failed. */
+ if (!local_swnodes)
+ return;
+
+ list_for_each_entry(conn, &csi2->connections, entry)
+ extract_crs_csi2_conn_info(local_handle, local_swnodes, conn);
+}
+
/**
* acpi_mipi_scan_crs_csi2 - Create ACPI _CRS CSI-2 software nodes
*
@@ -278,9 +416,22 @@ void acpi_mipi_scan_crs_csi2(void)
}
list_splice(&aux_list, &acpi_mipi_crs_csi2_list);
- /* Allocate softwware nodes for representing the CSI-2 information. */
+ /*
+ * Allocate softwware nodes for representing the CSI-2 information.
+ *
+ * This needs to be done for all of the list entries in one go, because
+ * they may point to each other without restrictions and the next step
+ * relies on the availability of swnodes memory for each list entry.
+ */
list_for_each_entry(csi2, &acpi_mipi_crs_csi2_list, entry)
alloc_crs_csi2_swnodes(csi2);
+
+ /*
+ * Set up software node properties using data from _CRS CSI-2 resource
+ * descriptors.
+ */
+ list_for_each_entry(csi2, &acpi_mipi_crs_csi2_list, entry)
+ prepare_crs_csi2_swnodes(csi2);
}
/**
From: Sakari Ailus <[email protected]>
Find the "rotation" property value for devices with _CRS CSI-2 resource
descriptors and use it to add the "rotation" property to the software
nodes representing the CSI-2 connection graph. That value typically
comes from the _PLD (Physical Location of Device) object if it is
present for the given device.
This way, camera sensor drivers that know the "rotation" property do not
need to care about _PLD on systems using ACPI.
Signed-off-by: Sakari Ailus <[email protected]>
[ rjw: Changelog edits, file rename ]
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/mipi-di.c | 17 +++++++++++++++++
include/acpi/acpi_bus.h | 1 +
2 files changed, 18 insertions(+)
Index: linux-pm/drivers/acpi/mipi-di.c
===================================================================
--- linux-pm.orig/drivers/acpi/mipi-di.c
+++ linux-pm/drivers/acpi/mipi-di.c
@@ -586,6 +586,7 @@ static void init_crs_csi2_swnodes(struct
struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER };
struct acpi_device_software_nodes *swnodes = csi2->swnodes;
acpi_handle handle = csi2->handle;
+ unsigned int prop_index = 0;
struct fwnode_handle *adev_fwnode;
struct acpi_device *adev;
acpi_status status;
@@ -605,6 +606,22 @@ static void init_crs_csi2_swnodes(struct
adev_fwnode = acpi_fwnode_handle(adev);
+ /*
+ * If the "rotation" property is not present, but _PLD is there,
+ * evaluate it to get the "rotation" value.
+ */
+ if (!fwnode_property_present(adev_fwnode, "rotation")) {
+ struct acpi_pld_info *pld;
+
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (ACPI_SUCCESS(status)) {
+ swnodes->dev_props[NEXT_PROPERTY(prop_index, DEV_ROTATION)] =
+ PROPERTY_ENTRY_U32("rotation",
+ pld->rotation * 45U);
+ kfree(pld);
+ }
+ }
+
status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
if (ACPI_FAILURE(status)) {
acpi_handle_info(handle, "Unable to get the path name\n");
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -373,6 +373,7 @@ struct acpi_gpio_mapping;
#define ACPI_DEVICE_SWNODE_PORT_NAME_LENGTH 8
enum acpi_device_swnode_dev_props {
+ ACPI_DEVICE_SWNODE_DEV_ROTATION,
ACPI_DEVICE_SWNODE_DEV_NUM_OF,
ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES
};
Hi Rafael,
On Fri, Oct 20, 2023 at 04:33:56PM +0200, Rafael J. Wysocki wrote:
> Hi Folks,
>
> This is a new revision of
>
> https://lore.kernel.org/linux-acpi/13276375.uLZWGnKmhe@kreacher/
>
> which was reported to have issues and it took time to revisit it.
>
> > The main points from the original cover letter are still valid:
> >
> > The general idea is the same - CSI-2 resource descriptors, introduced in
> > ACPI 6.4 and defined by
> >
> > https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#camera-serial-i
> > nterface-csi-2-connection-resource-descriptor
> >
> > are found and used for creating a set of software nodes that represent the
> > CSI-2 connection graph.
> >
> > These software nodes need to be available before any scan handlers or ACPI
> > drivers are bound to any struct acpi_device objects, so all of that is done
> > at the early stage of ACPI device enumeration, but unnecessary ACPI
> > namespace walks are avoided.
> >
> > The CSI-2 software nodes are populated with data extracted from the CSI-2
> > resource descriptors themselves and from device properties defined by the
> > MIPI DiSco for Imaging specification (see
> > https://www.mipi.org/specifications/mipi-disco-imaging).
> >
> > Patches [4,6/6] come from the original series directly, but the other
> > patches have been changes substantially, so I've decided to re-start patch
> > series versioning from scratch.
>
> The v2 addresses at least 3 issues found in the v1 by code inspection:
>
> * A port_count field incrementation was missing in acpi_mipi_scan_crs_csi2(),
> so its value for all of the devices having CSI2 resources in _CRS was always
> 1 (and it should be equal to the number of valid CSI2 connection resources).
>
> * Some acpi_mipi_crs_csi2_list members could be freed prematurely, so they were
> inaccessible when extract_crs_csi2_conn_info() attempted to access them.
>
> * A check of remote_swnodes() against NULL was missing, which could result in
> a crash in a case when the swnodes memory could not be allocated for some
> acpi_mipi_crs_csi2_list entries.
>
> Apart from that, it rearranges the code somewhat to make it easier to follow
> and to avoid premature freeing of memory in it in general and the new file
> added by it is now called mipi-di.c (instead of mipi-disco-imaging.c) for
> compactness.
>
> The series is based on current linux-next.
Thanks for the update. I've tested this and I can confirm it works, to the
extent implemented in the set. The rest can be implemented on top
(mainly replicating properties).
I'll comment on a few patches in the set.
Do you prefer to make the changes or shall I? I presume them to be fairly
minor.
--
Regards,
Sakari Ailus
Hi Sakari,
On Tue, Oct 31, 2023 at 11:33 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Rafael,
[cut]
> > The v2 addresses at least 3 issues found in the v1 by code inspection:
> >
> > * A port_count field incrementation was missing in acpi_mipi_scan_crs_csi2(),
> > so its value for all of the devices having CSI2 resources in _CRS was always
> > 1 (and it should be equal to the number of valid CSI2 connection resources).
> >
> > * Some acpi_mipi_crs_csi2_list members could be freed prematurely, so they were
> > inaccessible when extract_crs_csi2_conn_info() attempted to access them.
> >
> > * A check of remote_swnodes() against NULL was missing, which could result in
> > a crash in a case when the swnodes memory could not be allocated for some
> > acpi_mipi_crs_csi2_list entries.
> >
> > Apart from that, it rearranges the code somewhat to make it easier to follow
> > and to avoid premature freeing of memory in it in general and the new file
> > added by it is now called mipi-di.c (instead of mipi-disco-imaging.c) for
> > compactness.
> >
> > The series is based on current linux-next.
>
> Thanks for the update. I've tested this and I can confirm it works, to the
> extent implemented in the set. The rest can be implemented on top
> (mainly replicating properties).
Thanks for testing!
> I'll comment on a few patches in the set.
>
> Do you prefer to make the changes or shall I? I presume them to be fairly
> minor.
I can make the changes.