[PATCH v2 1/3] drm/dp_helper: Rework the drm_dp_aux documentation

[PATCH v2 1/3] drm/dp_helper: Rework the drm_dp_aux documentation

[Maxime Ripard]

Split the existing documentation to move the comments on particular
fields next to them.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

Changes from v1:
  - New patch
---
 include/drm/drm_dp_helper.h | 84 +++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 27 deletions(-)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1e85c2021f2f..1c5ae07ff0c7 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1837,29 +1837,6 @@ struct drm_dp_aux_cec {
 
 /**
  * struct drm_dp_aux - DisplayPort AUX channel
- * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
- * @ddc: I2C adapter that can be used for I2C-over-AUX communication
- * @dev: pointer to struct device that is the parent for this AUX channel
- * @crtc: backpointer to the crtc that is currently using this AUX channel
- * @hw_mutex: internal mutex used for locking transfers
- * @crc_work: worker that captures CRCs for each frame
- * @crc_count: counter of captured frame CRCs
- * @transfer: transfers a message representing a single AUX transaction
- *
- * The @dev field should be set to a pointer to the device that implements the
- * AUX channel.
- *
- * The @name field may be used to specify the name of the I2C adapter. If set to
- * %NULL, dev_name() of @dev will be used.
- *
- * Drivers provide a hardware-specific implementation of how transactions are
- * executed via the @transfer() function. A pointer to a &drm_dp_aux_msg
- * structure describing the transaction is passed into this function. Upon
- * success, the implementation should return the number of payload bytes that
- * were transferred, or a negative error-code on failure. Helpers propagate
- * errors from the @transfer() function, with the exception of the %-EBUSY
- * error, which causes a transaction to be retried. On a short, helpers will
- * return %-EPROTO to make it simpler to check for failure.
  *
  * An AUX channel can also be used to transport I2C messages to a sink. A
  * typical application of that is to access an EDID that's present in the sink
@@ -1870,21 +1847,74 @@ struct drm_dp_aux_cec {
  * transfers by default; if a partial response is received, the adapter will
  * drop down to the size given by the partial response for this transaction
  * only.
- *
- * Note that the aux helper code assumes that the @transfer() function only
- * modifies the reply field of the &drm_dp_aux_msg structure. The retry logic
- * and i2c helpers assume this is the case.
  */
 struct drm_dp_aux {
+	/**
+	 * @name: user-visible name of this AUX channel and the
+	 * I2C-over-AUX adapter.
+	 *
+	 * It's also used to specify the name of the I2C adapter. If set
+	 * to %NULL, dev_name() of @dev will be used.
+	 */
 	const char *name;
+
+	/**
+	 * @ddc: I2C adapter that can be used for I2C-over-AUX
+	 * communication
+	 */
 	struct i2c_adapter ddc;
+
+	/**
+	 * @dev: pointer to struct device that is the parent for this
+	 * AUX channel.
+	 */
 	struct device *dev;
+
+	/**
+	 * @crtc: backpointer to the crtc that is currently using this
+	 * AUX channel
+	 */
 	struct drm_crtc *crtc;
+
+	/**
+	 * @hw_mutex: internal mutex used for locking transfers.
+	 */
 	struct mutex hw_mutex;
+
+	/**
+	 * @crc_work: worker that captures CRCs for each frame
+	 */
 	struct work_struct crc_work;
+
+	/**
+	 * @crc_count: counter of captured frame CRCs
+	 */
 	u8 crc_count;
+
+	/**
+	 * @transfer: transfers a message representing a single AUX
+	 * transaction.
+	 *
+	 * This is a hardware-specific implementation of how
+	 * transactions are executed that the drivers must provide.
+	 *
+	 * A pointer to a &drm_dp_aux_msg structure describing the
+	 * transaction is passed into this function. Upon success, the
+	 * implementation should return the number of payload bytes that
+	 * were transferred, or a negative error-code on failure.
+	 *
+	 * Helpers will propagate these errors, with the exception of
+	 * the %-EBUSY error, which causes a transaction to be retried.
+	 * On a short, helpers will return %-EPROTO to make it simpler
+	 * to check for failure.
+	 *
+	 * The @transfer() function must only modify the reply field of
+	 * the &drm_dp_aux_msg structure. The retry logic and i2c
+	 * helpers assume this is the case.
+	 */
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
+
 	/**
 	 * @i2c_nack_count: Counts I2C NACKs, used for DP validation.
 	 */
-- 
2.31.1

[PATCH v2 2/3] drm/dp_helper: Mention the concurrency requirement hw_mutex

[Maxime Ripard]

Drivers that allow concurrent access over multiple DP channels need to
provide additional locking, even though the hw_mutex field might
indicate otherwise. Clarify it in the documentation.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

Changes from v1:
  - New patch
---
 include/drm/drm_dp_helper.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1c5ae07ff0c7..0cc6062ff3ad 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1878,6 +1878,10 @@ struct drm_dp_aux {
 
 	/**
 	 * @hw_mutex: internal mutex used for locking transfers.
+	 *
+	 * Note that if the underlying hardware is shared among multiple
+	 * channels, the driver needs to do additional locking to
+	 * prevent concurrent access.
 	 */
 	struct mutex hw_mutex;
 
-- 
2.31.1

[PATCH v2 3/3] drm: Mention the power state requirement on side-channel operations

[Maxime Ripard]

The drm_connector detect, drm_dp_aux transfer and mipi_dsi_host
operations typically require to access their underlying device to
perform what is expected of them.

However, there's no guarantee on the fact that the device has been
enabled through atomic_enable or similar that will usually power the
device. The access to an unpowered device is then an undefined behaviour
ranging from the access being ignored to a hard CPU hang.

Let's document that expectation to avoid as much as possible those
consequences.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

Changes from v1:
  - moved to the field documentation instead of the structure's
---
 include/drm/drm_connector.h | 5 +++++
 include/drm/drm_dp_helper.h | 5 +++++
 include/drm/drm_mipi_dsi.h  | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..fae03a43d04f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -848,6 +848,11 @@ struct drm_connector_funcs {
 	 * locks to avoid races with concurrent modeset changes need to use
 	 * &drm_connector_helper_funcs.detect_ctx instead.
 	 *
+	 * Also note that this callback can be called no matter the
+	 * state the connector is in. Drivers that need the underlying
+	 * device to be powered to perform the detection will first need
+	 * to make sure it's been properly enabled.
+	 *
 	 * RETURNS:
 	 *
 	 * drm_connector_status indicating the connector's status.
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 0cc6062ff3ad..cb673bc5b871 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1915,6 +1915,11 @@ struct drm_dp_aux {
 	 * The @transfer() function must only modify the reply field of
 	 * the &drm_dp_aux_msg structure. The retry logic and i2c
 	 * helpers assume this is the case.
+	 *
+	 * Also note that this callback can be called no matter the
+	 * state @dev is in. Drivers that need that device to be powered
+	 * to perform this operation will first need to make sure it's
+	 * been properly enabled.
 	 */
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 360e6377e84b..849d3029e303 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -80,6 +80,11 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
  * Note that typically DSI packet transmission is atomic, so the .transfer()
  * function will seldomly return anything other than the number of bytes
  * contained in the transmit buffer on success.
+ *
+ * Also note that those callbacks can be called no matter the state the
+ * host is in. Drivers that need the underlying device to be powered to
+ * perform these operations will first need to make sure it's been
+ * properly enabled.
  */
 struct mipi_dsi_host_ops {
 	int (*attach)(struct mipi_dsi_host *host,
-- 
2.31.1

Re: [PATCH v2 1/3] drm/dp_helper: Rework the drm_dp_aux documentation

[Daniel Vetter]

On Wed, Jun 16, 2021 at 04:15:27PM +0200, Maxime Ripard wrote:
> Split the existing documentation to move the comments on particular
> fields next to them.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---
> 
> Changes from v1:
>   - New patch

On the series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks for doing this polish&doc improvement.
-Daniel
> ---
>  include/drm/drm_dp_helper.h | 84 +++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 27 deletions(-)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 1e85c2021f2f..1c5ae07ff0c7 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1837,29 +1837,6 @@ struct drm_dp_aux_cec {
>  
>  /**
>   * struct drm_dp_aux - DisplayPort AUX channel
> - * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
> - * @ddc: I2C adapter that can be used for I2C-over-AUX communication
> - * @dev: pointer to struct device that is the parent for this AUX channel
> - * @crtc: backpointer to the crtc that is currently using this AUX channel
> - * @hw_mutex: internal mutex used for locking transfers
> - * @crc_work: worker that captures CRCs for each frame
> - * @crc_count: counter of captured frame CRCs
> - * @transfer: transfers a message representing a single AUX transaction
> - *
> - * The @dev field should be set to a pointer to the device that implements the
> - * AUX channel.
> - *
> - * The @name field may be used to specify the name of the I2C adapter. If set to
> - * %NULL, dev_name() of @dev will be used.
> - *
> - * Drivers provide a hardware-specific implementation of how transactions are
> - * executed via the @transfer() function. A pointer to a &drm_dp_aux_msg
> - * structure describing the transaction is passed into this function. Upon
> - * success, the implementation should return the number of payload bytes that
> - * were transferred, or a negative error-code on failure. Helpers propagate
> - * errors from the @transfer() function, with the exception of the %-EBUSY
> - * error, which causes a transaction to be retried. On a short, helpers will
> - * return %-EPROTO to make it simpler to check for failure.
>   *
>   * An AUX channel can also be used to transport I2C messages to a sink. A
>   * typical application of that is to access an EDID that's present in the sink
> @@ -1870,21 +1847,74 @@ struct drm_dp_aux_cec {
>   * transfers by default; if a partial response is received, the adapter will
>   * drop down to the size given by the partial response for this transaction
>   * only.
> - *
> - * Note that the aux helper code assumes that the @transfer() function only
> - * modifies the reply field of the &drm_dp_aux_msg structure. The retry logic
> - * and i2c helpers assume this is the case.
>   */
>  struct drm_dp_aux {
> +	/**
> +	 * @name: user-visible name of this AUX channel and the
> +	 * I2C-over-AUX adapter.
> +	 *
> +	 * It's also used to specify the name of the I2C adapter. If set
> +	 * to %NULL, dev_name() of @dev will be used.
> +	 */
>  	const char *name;
> +
> +	/**
> +	 * @ddc: I2C adapter that can be used for I2C-over-AUX
> +	 * communication
> +	 */
>  	struct i2c_adapter ddc;
> +
> +	/**
> +	 * @dev: pointer to struct device that is the parent for this
> +	 * AUX channel.
> +	 */
>  	struct device *dev;
> +
> +	/**
> +	 * @crtc: backpointer to the crtc that is currently using this
> +	 * AUX channel
> +	 */
>  	struct drm_crtc *crtc;
> +
> +	/**
> +	 * @hw_mutex: internal mutex used for locking transfers.
> +	 */
>  	struct mutex hw_mutex;
> +
> +	/**
> +	 * @crc_work: worker that captures CRCs for each frame
> +	 */
>  	struct work_struct crc_work;
> +
> +	/**
> +	 * @crc_count: counter of captured frame CRCs
> +	 */
>  	u8 crc_count;
> +
> +	/**
> +	 * @transfer: transfers a message representing a single AUX
> +	 * transaction.
> +	 *
> +	 * This is a hardware-specific implementation of how
> +	 * transactions are executed that the drivers must provide.
> +	 *
> +	 * A pointer to a &drm_dp_aux_msg structure describing the
> +	 * transaction is passed into this function. Upon success, the
> +	 * implementation should return the number of payload bytes that
> +	 * were transferred, or a negative error-code on failure.
> +	 *
> +	 * Helpers will propagate these errors, with the exception of
> +	 * the %-EBUSY error, which causes a transaction to be retried.
> +	 * On a short, helpers will return %-EPROTO to make it simpler
> +	 * to check for failure.
> +	 *
> +	 * The @transfer() function must only modify the reply field of
> +	 * the &drm_dp_aux_msg structure. The retry logic and i2c
> +	 * helpers assume this is the case.
> +	 */
>  	ssize_t (*transfer)(struct drm_dp_aux *aux,
>  			    struct drm_dp_aux_msg *msg);
> +
>  	/**
>  	 * @i2c_nack_count: Counts I2C NACKs, used for DP validation.
>  	 */
> -- 
> 2.31.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/3] drm/dp_helper: Rework the drm_dp_aux documentation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On Thu, Jun 17, 2021 at 08:58:31PM +0200, Daniel Vetter wrote:
> On Wed, Jun 16, 2021 at 04:15:27PM +0200, Maxime Ripard wrote:
> > Split the existing documentation to move the comments on particular
> > fields next to them.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > ---
> > 
> > Changes from v1:
> >   - New patch
> 
> On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks for doing this polish&doc improvement.

Applied all three, thanks
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]