diff mbox series

[RFC,2/7] drm/display: dp: implement new access helpers

Message ID 20250117-drm-rework-dpcd-access-v1-2-7fc020e04dbc@linaro.org
State New
Headers show
Series drm/display: dp: add new DPCD access functions | expand

Commit Message

Dmitry Baryshkov Jan. 17, 2025, 8:56 a.m. UTC
Existing DPCD access functions return an error code or the number of
bytes being read / write in case of partial access. However a lot of
drivers either (incorrectly) ignore partial access or mishandle error
codes. In other cases this results in a boilerplate code which compares
returned value with the size.

Implement new set of DPCD access helpers, which ignore partial access,
always return 0 or an error code. Implement existing helpers using the
new functions to ensure backwards compatibility.

Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_dp_helper.c       | 42 +++++++-------
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 27 +++++----
 include/drm/display/drm_dp_helper.h           | 81 ++++++++++++++++++++++++++-
 include/drm/display/drm_dp_mst_helper.h       | 10 ++--
 4 files changed, 119 insertions(+), 41 deletions(-)

Comments

Dmitry Baryshkov Jan. 23, 2025, 11:04 a.m. UTC | #1
On Thu, Jan 23, 2025 at 12:26:25PM +0200, Jani Nikula wrote:
> On Fri, 17 Jan 2025, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > Existing DPCD access functions return an error code or the number of
> > bytes being read / write in case of partial access. However a lot of
> > drivers either (incorrectly) ignore partial access or mishandle error
> > codes. In other cases this results in a boilerplate code which compares
> > returned value with the size.
> >
> > Implement new set of DPCD access helpers, which ignore partial access,
> > always return 0 or an error code. Implement existing helpers using the
> > new functions to ensure backwards compatibility.
> >
> > Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c       | 42 +++++++-------
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 27 +++++----
> >  include/drm/display/drm_dp_helper.h           | 81 ++++++++++++++++++++++++++-
> >  include/drm/display/drm_dp_mst_helper.h       | 10 ++--
> >  4 files changed, 119 insertions(+), 41 deletions(-)
> >
> > +
> > +/**
> > + * drm_dp_dpcd_write() - write a series of bytes from the DPCD
> > + * @aux: DisplayPort AUX channel (SST or MST)
> > + * @offset: address of the (first) register to write
> > + * @buffer: buffer containing the values to write
> > + * @size: number of bytes in @buffer
> > + *
> > + * Deprecated wrapper around drm_dp_dpcd_write().
> > + * Returns the number of bytes transferred on success, or a negative error
> > + * code on failure.
> > + */
> > +static inline ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux,
> > +					unsigned int offset,
> > +					void *buffer, size_t size)
> > +{
> > +	int ret = drm_dp_dpcd_write_data(aux, offset, buffer, size);
> > +
> > +	if (ret < 0)
> > +		return ret;
> 
> I just realized this changes behaviour. This no longer returns the
> number of bytes transferred when it's less than size. It'll always be an
> error.
> 
> Now, if we were to accept this change, I wonder if we could do that as a
> standalone change first, within the current functions? Return either
> size or negative error, nothing between [0..size).
> 
> After that, we could change all the return checks for "!= size" or "<
> size" to "< 0" (because they would be the same thing). When all the
> places have been changed, we could eventually switch from returning size
> to returning 0 on success when nobody depends on it anymore, and keep
> the same function names.
> 
> I think this does have a certain appeal to it. Thoughts?

I thought about it while working on the series. There is an obvious and
huge problem: with the changed function names you actually have to
review usage patterns and verify that the return comparison is correct.
If the name is unchanged, it is easy to miss such usage patterns. For
example, i915 / amd / msm drivers are being developed in their own
trees. Even if we review those drivers at some point, nothing stops them
from adding new code points, checking for size instead of 0. Likewise
patches-in-flight also can't be properly reviewed if we just change the
return value.

Thus, I think, while the idea of keeping function names sounds
appealing, it doesn't help in a longer term and can potentially create
even more confusion.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 809c65dcb58983693fb335b88759a66919410114..5a693f2779284467e2d05b9d8b2c2bee0ad6c112 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -495,13 +495,13 @@  EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
 
 static inline void
 drm_dp_dump_access(const struct drm_dp_aux *aux,
-		   u8 request, uint offset, void *buffer, int ret)
+		   u8 request, uint offset, void *buffer, size_t size, int ret)
 {
 	const char *arrow = request == DP_AUX_NATIVE_READ ? "->" : "<-";
 
-	if (ret > 0)
+	if (ret == 0)
 		drm_dbg_dp(aux->drm_dev, "%s: 0x%05x AUX %s (ret=%3d) %*ph\n",
-			   aux->name, offset, arrow, ret, min(ret, 20), buffer);
+			   aux->name, offset, arrow, ret, min_t(int, size, 20), buffer);
 	else
 		drm_dbg_dp(aux->drm_dev, "%s: 0x%05x AUX %s (ret=%3d)\n",
 			   aux->name, offset, arrow, ret);
@@ -559,8 +559,10 @@  static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 		if (ret >= 0) {
 			native_reply = msg.reply & DP_AUX_NATIVE_REPLY_MASK;
 			if (native_reply == DP_AUX_NATIVE_REPLY_ACK) {
-				if (ret == size)
+				if (ret == size) {
+					ret = 0;
 					goto unlock;
+				}
 
 				ret = -EPROTO;
 			} else
@@ -602,9 +604,9 @@  int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset)
 	int ret;
 
 	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, &buffer, 1);
-	WARN_ON(ret == 0);
+	WARN_ON(ret == -EPROTO);
 
-	drm_dp_dump_access(aux, DP_AUX_NATIVE_READ, offset, &buffer, ret);
+	drm_dp_dump_access(aux, DP_AUX_NATIVE_READ, offset, &buffer, 1, ret);
 
 	return ret < 0 ? ret : 0;
 }
@@ -634,21 +636,21 @@  void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
 EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
 
 /**
- * drm_dp_dpcd_read() - read a series of bytes from the DPCD
+ * drm_dp_dpcd_read_data() - read a series of bytes from the DPCD
  * @aux: DisplayPort AUX channel (SST or MST)
  * @offset: address of the (first) register to read
  * @buffer: buffer to store the register values
  * @size: number of bytes in @buffer
  *
- * Returns the number of bytes transferred on success, or a negative error
+ * Returns zero (0) on success, or a negative error
  * code on failure. -EIO is returned if the request was NAKed by the sink or
  * if the retry count was exceeded. If not all bytes were transferred, this
  * function returns -EPROTO. Errors from the underlying AUX channel transfer
  * function, with the exception of -EBUSY (which causes the transaction to
  * be retried), are propagated to the caller.
  */
-ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
-			 void *buffer, size_t size)
+int drm_dp_dpcd_read_data(struct drm_dp_aux *aux, unsigned int offset,
+			  void *buffer, size_t size)
 {
 	int ret;
 
@@ -671,45 +673,45 @@  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 	}
 
 	if (aux->is_remote)
-		ret = drm_dp_mst_dpcd_read(aux, offset, buffer, size);
+		ret = drm_dp_mst_dpcd_read_data(aux, offset, buffer, size);
 	else
 		ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset,
 					 buffer, size);
 
-	drm_dp_dump_access(aux, DP_AUX_NATIVE_READ, offset, buffer, ret);
+	drm_dp_dump_access(aux, DP_AUX_NATIVE_READ, offset, buffer, size, ret);
 	return ret;
 }
-EXPORT_SYMBOL(drm_dp_dpcd_read);
+EXPORT_SYMBOL(drm_dp_dpcd_read_data);
 
 /**
- * drm_dp_dpcd_write() - write a series of bytes to the DPCD
+ * drm_dp_dpcd_write_data() - write a series of bytes to the DPCD
  * @aux: DisplayPort AUX channel (SST or MST)
  * @offset: address of the (first) register to write
  * @buffer: buffer containing the values to write
  * @size: number of bytes in @buffer
  *
- * Returns the number of bytes transferred on success, or a negative error
+ * Returns zero (0) on success, or a negative error
  * code on failure. -EIO is returned if the request was NAKed by the sink or
  * if the retry count was exceeded. If not all bytes were transferred, this
  * function returns -EPROTO. Errors from the underlying AUX channel transfer
  * function, with the exception of -EBUSY (which causes the transaction to
  * be retried), are propagated to the caller.
  */
-ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
-			  void *buffer, size_t size)
+int drm_dp_dpcd_write_data(struct drm_dp_aux *aux, unsigned int offset,
+			   void *buffer, size_t size)
 {
 	int ret;
 
 	if (aux->is_remote)
-		ret = drm_dp_mst_dpcd_write(aux, offset, buffer, size);
+		ret = drm_dp_mst_dpcd_write_data(aux, offset, buffer, size);
 	else
 		ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset,
 					 buffer, size);
 
-	drm_dp_dump_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, ret);
+	drm_dp_dump_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, size, ret);
 	return ret;
 }
-EXPORT_SYMBOL(drm_dp_dpcd_write);
+EXPORT_SYMBOL(drm_dp_dpcd_write_data);
 
 /**
  * drm_dp_dpcd_read_link_status() - read DPCD link status (bytes 0x202-0x207)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index f8cd094efa3c0bd6f75b52a0410b0910d8026a76..f8db5be53a33e87e94b864ba48151354e091f5aa 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2128,20 +2128,20 @@  drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
 }
 
 /**
- * drm_dp_mst_dpcd_read() - read a series of bytes from the DPCD via sideband
+ * drm_dp_mst_dpcd_read_data() - read a series of bytes from the DPCD via sideband
  * @aux: Fake sideband AUX CH
  * @offset: address of the (first) register to read
  * @buffer: buffer to store the register values
  * @size: number of bytes in @buffer
  *
  * Performs the same functionality for remote devices via
- * sideband messaging as drm_dp_dpcd_read() does for local
+ * sideband messaging as drm_dp_dpcd_read_data() does for local
  * devices via actual AUX CH.
  *
- * Return: Number of bytes read, or negative error code on failure.
+ * Return: Zero (0) on success, or negative error code on failure.
  */
-ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
-			     unsigned int offset, void *buffer, size_t size)
+int drm_dp_mst_dpcd_read_data(struct drm_dp_aux *aux,
+			      unsigned int offset, void *buffer, size_t size)
 {
 	struct drm_dp_mst_port *port = container_of(aux, struct drm_dp_mst_port,
 						    aux);
@@ -2151,20 +2151,20 @@  ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
 }
 
 /**
- * drm_dp_mst_dpcd_write() - write a series of bytes to the DPCD via sideband
+ * drm_dp_mst_dpcd_write_data() - write a series of bytes to the DPCD via sideband
  * @aux: Fake sideband AUX CH
  * @offset: address of the (first) register to write
  * @buffer: buffer containing the values to write
  * @size: number of bytes in @buffer
  *
  * Performs the same functionality for remote devices via
- * sideband messaging as drm_dp_dpcd_write() does for local
+ * sideband messaging as drm_dp_dpcd_write_data() does for local
  * devices via actual AUX CH.
  *
- * Return: number of bytes written on success, negative error code on failure.
+ * Return: zero (0) on success, negative error code on failure.
  */
-ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
-			      unsigned int offset, void *buffer, size_t size)
+int drm_dp_mst_dpcd_write_data(struct drm_dp_aux *aux,
+			       unsigned int offset, void *buffer, size_t size)
 {
 	struct drm_dp_mst_port *port = container_of(aux, struct drm_dp_mst_port,
 						    aux);
@@ -3490,9 +3490,8 @@  static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
 		goto fail_free;
 	}
 
-	ret = min_t(size_t, txmsg->reply.u.remote_dpcd_read_ack.num_bytes,
-		    size);
-	memcpy(bytes, txmsg->reply.u.remote_dpcd_read_ack.bytes, ret);
+	memcpy(bytes, txmsg->reply.u.remote_dpcd_read_ack.bytes, size);
+	ret = 0;
 
 fail_free:
 	kfree(txmsg);
@@ -3530,7 +3529,7 @@  static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
 		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
 			ret = -EIO;
 		else
-			ret = size;
+			ret = 0;
 	}
 
 	kfree(txmsg);
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 8f4054a560396a43750570a8c2e95624039ab8ad..548237a81ef0359dab1ed7df6ef0fd1e0c76e0c5 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -522,10 +522,85 @@  struct drm_dp_aux {
 
 int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
 void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
-ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
-			 void *buffer, size_t size);
-ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
+
+int drm_dp_dpcd_read_data(struct drm_dp_aux *aux, unsigned int offset,
 			  void *buffer, size_t size);
+int drm_dp_dpcd_write_data(struct drm_dp_aux *aux, unsigned int offset,
+			   void *buffer, size_t size);
+
+/**
+ * drm_dp_dpcd_read() - read a series of bytes from the DPCD
+ * @aux: DisplayPort AUX channel (SST or MST)
+ * @offset: address of the (first) register to read
+ * @buffer: buffer to store the register values
+ * @size: number of bytes in @buffer
+ *
+ * Deprecated wrapper around drm_dp_dpcd_read().
+ * Returns the number of bytes transferred on success, or a negative error
+ * code on failure.
+ */
+static inline ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux,
+				       unsigned int offset,
+				       void *buffer, size_t size)
+{
+	int ret = drm_dp_dpcd_read_data(aux, offset, buffer, size);
+
+	if (ret < 0)
+		return ret;
+
+	return size;
+}
+
+/**
+ * drm_dp_dpcd_read_byte() - read a single byte from the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the register to read
+ * @valuep: location where the value of the register will be stored
+ *
+ * Returns zero (0) on success, or a negative error code on failure.
+ */
+static inline int drm_dp_dpcd_read_byte(struct drm_dp_aux *aux,
+					unsigned int offset, u8 *valuep)
+{
+	return drm_dp_dpcd_read_data(aux, offset, valuep, 1);
+}
+
+/**
+ * drm_dp_dpcd_write_byte() - write a single byte to the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the register to write
+ * @value: value to write to the register
+ *
+ * Returns zero (0) on success, or a negative error code on failure.
+ */
+static inline int drm_dp_dpcd_write_byte(struct drm_dp_aux *aux,
+					 unsigned int offset, u8 value)
+{
+	return drm_dp_dpcd_write_data(aux, offset, &value, 1);
+}
+
+/**
+ * drm_dp_dpcd_write() - write a series of bytes from the DPCD
+ * @aux: DisplayPort AUX channel (SST or MST)
+ * @offset: address of the (first) register to write
+ * @buffer: buffer containing the values to write
+ * @size: number of bytes in @buffer
+ *
+ * Deprecated wrapper around drm_dp_dpcd_write().
+ * Returns the number of bytes transferred on success, or a negative error
+ * code on failure.
+ */
+static inline ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux,
+					unsigned int offset,
+					void *buffer, size_t size)
+{
+	int ret = drm_dp_dpcd_write_data(aux, offset, buffer, size);
+
+	if (ret < 0)
+		return ret;
+
+	return size;
+}
 
 /**
  * drm_dp_dpcd_readb() - read a single byte from the DPCD
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index a80ba457a858f36ac2110a6fdd91d8a1570b58e1..d527b323a7a8c92b93280fcc8cd3025e21cdcf02 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -899,10 +899,12 @@  int __must_check
 drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
 			       bool sync);
 
-ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
-			     unsigned int offset, void *buffer, size_t size);
-ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
-			      unsigned int offset, void *buffer, size_t size);
+int drm_dp_mst_dpcd_read_data(struct drm_dp_aux *aux,
+			      unsigned int offset,
+			      void *buffer, size_t size);
+int drm_dp_mst_dpcd_write_data(struct drm_dp_aux *aux,
+			       unsigned int offset,
+			       void *buffer, size_t size);
 
 int drm_dp_mst_connector_late_register(struct drm_connector *connector,
 				       struct drm_dp_mst_port *port);