Switch drm_dp_aux_dev.c to use new set of DPCD read / write helpers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_dp_aux_dev.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_aux_dev.c b/drivers/gpu/drm/display/drm_dp_aux_dev.c
index 29555b9f03c8c42681c17c4a01e74a966cf8611f..a31ab3f41efb71fd5f936c24ba5c3b8ebea68a5e 100644
--- a/drivers/gpu/drm/display/drm_dp_aux_dev.c
+++ b/drivers/gpu/drm/display/drm_dp_aux_dev.c
@@ -163,17 +163,16 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			break;
 		}
 
-		res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
-
+		res = drm_dp_dpcd_read_data(aux_dev->aux, pos, buf, todo);
 		if (res <= 0)
 			break;
 
-		if (copy_to_iter(buf, res, to) != res) {
+		if (copy_to_iter(buf, todo, to) != todo) {
 			res = -EFAULT;
 			break;
 		}
 
-		pos += res;
+		pos += todo;
 	}
 
 	if (pos != iocb->ki_pos)
@@ -211,12 +210,11 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			break;
 		}
 
-		res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
-
+		res = drm_dp_dpcd_write_data(aux_dev->aux, pos, buf, todo);
 		if (res <= 0)
 			break;
 
-		pos += res;
+		pos += todo;
 	}
 
 	if (pos != iocb->ki_pos)

-- 
2.39.5

On Fri, 17 Jan 2025, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> Switch drm_dp_aux_dev.c to use new set of DPCD read / write helpers.

This might be one of the few places where the old functions and the old
return value was used in a sensible manner.

BR,
Jani.

>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/display/drm_dp_aux_dev.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_dev.c b/drivers/gpu/drm/display/drm_dp_aux_dev.c
> index 29555b9f03c8c42681c17c4a01e74a966cf8611f..a31ab3f41efb71fd5f936c24ba5c3b8ebea68a5e 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_dev.c
> @@ -163,17 +163,16 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  			break;
>  		}
>  
> -		res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> -
> +		res = drm_dp_dpcd_read_data(aux_dev->aux, pos, buf, todo);
>  		if (res <= 0)
>  			break;
>  
> -		if (copy_to_iter(buf, res, to) != res) {
> +		if (copy_to_iter(buf, todo, to) != todo) {
>  			res = -EFAULT;
>  			break;
>  		}
>  
> -		pos += res;
> +		pos += todo;
>  	}
>  
>  	if (pos != iocb->ki_pos)
> @@ -211,12 +210,11 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  			break;
>  		}
>  
> -		res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> -
> +		res = drm_dp_dpcd_write_data(aux_dev->aux, pos, buf, todo);
>  		if (res <= 0)
>  			break;
>  
> -		pos += res;
> +		pos += todo;
>  	}
>  
>  	if (pos != iocb->ki_pos)

-- 
Jani Nikula, Intel

On Thu, Jan 23, 2025 at 12:05:29PM +0200, Jani Nikula wrote:
> On Fri, 17 Jan 2025, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > Switch drm_dp_aux_dev.c to use new set of DPCD read / write helpers.
> 
> This might be one of the few places where the old functions and the old
> return value was used in a sensible manner.

Well... Yes and no. What does it mean if we return less bytes? Is that
still a protocol error?

> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/display/drm_dp_aux_dev.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_aux_dev.c b/drivers/gpu/drm/display/drm_dp_aux_dev.c
> > index 29555b9f03c8c42681c17c4a01e74a966cf8611f..a31ab3f41efb71fd5f936c24ba5c3b8ebea68a5e 100644
> > --- a/drivers/gpu/drm/display/drm_dp_aux_dev.c
> > +++ b/drivers/gpu/drm/display/drm_dp_aux_dev.c
> > @@ -163,17 +163,16 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> >  			break;
> >  		}
> >  
> > -		res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> > -
> > +		res = drm_dp_dpcd_read_data(aux_dev->aux, pos, buf, todo);
> >  		if (res <= 0)
> >  			break;
> >  
> > -		if (copy_to_iter(buf, res, to) != res) {
> > +		if (copy_to_iter(buf, todo, to) != todo) {
> >  			res = -EFAULT;
> >  			break;
> >  		}
> >  
> > -		pos += res;
> > +		pos += todo;
> >  	}
> >  
> >  	if (pos != iocb->ki_pos)
> > @@ -211,12 +210,11 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
> >  			break;
> >  		}
> >  
> > -		res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> > -
> > +		res = drm_dp_dpcd_write_data(aux_dev->aux, pos, buf, todo);
> >  		if (res <= 0)
> >  			break;
> >  
> > -		pos += res;
> > +		pos += todo;
> >  	}
> >  
> >  	if (pos != iocb->ki_pos)
> 
> -- 
> Jani Nikula, Intel

-- 
With best wishes
Dmitry

On Thu, Jan 23, 2025 at 01:05:47PM +0200, Dmitry Baryshkov wrote:
> On Thu, Jan 23, 2025 at 12:05:29PM +0200, Jani Nikula wrote:
> > On Fri, 17 Jan 2025, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > > Switch drm_dp_aux_dev.c to use new set of DPCD read / write helpers.
> > 
> > This might be one of the few places where the old functions and the old
> > return value was used in a sensible manner.
> 
> Well... Yes and no. What does it mean if we return less bytes? Is that
> still a protocol error?

AFAIK short AUX replies are perfectly legal accoding to the DP
spec, but we've not really seen them happening in any real
use cases I suppose (although I'm not sure we have sufficient
logging to tell whether something failed completely or only
partially), hence why we've never really handled them
correctly.

For aux_dev it might matter more because the common use
case is to just dump the entire DPCD, and some displays
violate the spec by having black holes inside the DPCD.
What I don't rembmer is whether those black holes actually
result in short replies, or whether the entire AUX transfer
gets rejected when it hits one even partially.

The other concern with not handling short replies correctly
is that writes (and even some reads) can have side effects.
So when a short reply arrives we may have already triggered 
some side effects while still claiming that the access
completely failed.

I suppose if someone was sufficiently motivated they could
try to handle short replies more correctly and keep retrying
the remaining bytes (assuming that is the correct way to
handle them). Although with those black holes I guess
you'd eventually have to give up anyway before having
transferred all the bytes.

-- 
Ville Syrjälä
Intel

On Thu, Feb 13, 2025 at 01:56:12AM +0200, Ville Syrjälä wrote:
> On Thu, Jan 23, 2025 at 01:05:47PM +0200, Dmitry Baryshkov wrote:
> > On Thu, Jan 23, 2025 at 12:05:29PM +0200, Jani Nikula wrote:
> > > On Fri, 17 Jan 2025, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > > > Switch drm_dp_aux_dev.c to use new set of DPCD read / write helpers.
> > > 
> > > This might be one of the few places where the old functions and the old
> > > return value was used in a sensible manner.
> > 
> > Well... Yes and no. What does it mean if we return less bytes? Is that
> > still a protocol error?
> 
> AFAIK short AUX replies are perfectly legal accoding to the DP
> spec, but we've not really seen them happening in any real
> use cases I suppose (although I'm not sure we have sufficient
> logging to tell whether something failed completely or only
> partially), hence why we've never really handled them
> correctly.
> 
> For aux_dev it might matter more because the common use
> case is to just dump the entire DPCD, and some displays
> violate the spec by having black holes inside the DPCD.
> What I don't rembmer is whether those black holes actually
> result in short replies, or whether the entire AUX transfer
> gets rejected when it hits one even partially.

I see. Let's keep the old interface just for the dp-aux-dev and make
everybody else switch to the new interface. This might complicate the
patchset a bit, but it seems that's how it should be done.

> 
> The other concern with not handling short replies correctly
> is that writes (and even some reads) can have side effects.
> So when a short reply arrives we may have already triggered 
> some side effects while still claiming that the access
> completely failed.
> 
> I suppose if someone was sufficiently motivated they could
> try to handle short replies more correctly and keep retrying
> the remaining bytes (assuming that is the correct way to
> handle them). Although with those black holes I guess
> you'd eventually have to give up anyway before having
> transferred all the bytes.

-- 
With best wishes
Dmitry