And of course, we'll also need to read the sink count from other drivers
as well if we're checking whether or not it's supported. So, let's
extract the code for this into another helper.
v2:
* Fix drm_dp_dpcd_readb() ret check
* Add back comment and move back sink_count assignment in intel_dp_get_dpcd()
Signed-off-by: Lyude Paul <[email protected]>
Reviewed-by: Sean Paul <[email protected]>
---
drivers/gpu/drm/drm_dp_helper.c | 22 ++++++++++++++++++++++
drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++------
include/drm/drm_dp_helper.h | 1 +
3 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 863e0babc1903..67ad05eb05b7e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -736,6 +736,28 @@ bool drm_dp_has_sink_count(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_dp_has_sink_count);
+/**
+ * drm_dp_get_sink_count() - Retrieve the sink count for a given sink
+ * @aux: The DP AUX channel to use
+ *
+ * Returns: The current sink count reported by @aux, or a negative error code
+ * otherwise.
+ */
+int drm_dp_get_sink_count(struct drm_dp_aux *aux)
+{
+ u8 count;
+ int ret;
+
+ ret = drm_dp_dpcd_readb(aux, DP_SINK_COUNT, &count);
+ if (ret < 0)
+ return ret;
+ if (ret != 1)
+ return -EIO;
+
+ return DP_GET_SINK_COUNT(count);
+}
+EXPORT_SYMBOL(drm_dp_get_sink_count);
+
/*
* I2C-over-AUX implementation
*/
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 35a4779a442e2..4337321a3be4f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4648,6 +4648,8 @@ intel_dp_has_sink_count(struct intel_dp *intel_dp)
static bool
intel_dp_get_dpcd(struct intel_dp *intel_dp)
{
+ int ret;
+
if (!intel_dp_read_dpcd(intel_dp))
return false;
@@ -4664,11 +4666,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
}
if (intel_dp_has_sink_count(intel_dp)) {
- u8 count;
- ssize_t r;
-
- r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count);
- if (r < 1)
+ ret = drm_dp_get_sink_count(&intel_dp->aux);
+ if (ret < 0)
return false;
/*
@@ -4676,7 +4675,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
* a member variable in intel_dp will track any changes
* between short pulse interrupts.
*/
- intel_dp->sink_count = DP_GET_SINK_COUNT(count);
+ intel_dp->sink_count = ret;
/*
* SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index a1413a531eaf4..0c141fc81aaa8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1635,6 +1635,7 @@ struct drm_dp_desc;
bool drm_dp_has_sink_count(struct drm_connector *connector,
const u8 dpcd[DP_RECEIVER_CAP_SIZE],
const struct drm_dp_desc *desc);
+int drm_dp_get_sink_count(struct drm_dp_aux *aux);
void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
void drm_dp_aux_init(struct drm_dp_aux *aux);
--
2.26.2
On Tue, 25 Aug 2020, Lyude Paul <[email protected]> wrote:
> And of course, we'll also need to read the sink count from other drivers
> as well if we're checking whether or not it's supported. So, let's
> extract the code for this into another helper.
>
> v2:
> * Fix drm_dp_dpcd_readb() ret check
> * Add back comment and move back sink_count assignment in intel_dp_get_dpcd()
>
> Signed-off-by: Lyude Paul <[email protected]>
> Reviewed-by: Sean Paul <[email protected]>
> ---
> drivers/gpu/drm/drm_dp_helper.c | 22 ++++++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++------
> include/drm/drm_dp_helper.h | 1 +
> 3 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 863e0babc1903..67ad05eb05b7e 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -736,6 +736,28 @@ bool drm_dp_has_sink_count(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_dp_has_sink_count);
>
> +/**
> + * drm_dp_get_sink_count() - Retrieve the sink count for a given sink
From the department of bikeshedding...
Should we have a naming scheme where it's obvious whether a function
will do DPCD access, or just shuffle existing data?
For example, drm_dp_read_foo() for anything with DPCD access
vs. drm_dp_get_foo() or even simpler for anything that only processes
pre-read data?
> + * @aux: The DP AUX channel to use
> + *
> + * Returns: The current sink count reported by @aux, or a negative error code
> + * otherwise.
> + */
> +int drm_dp_get_sink_count(struct drm_dp_aux *aux)
> +{
> + u8 count;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_SINK_COUNT, &count);
> + if (ret < 0)
> + return ret;
> + if (ret != 1)
> + return -EIO;
Makes me wonder if that shouldn't be at drm_dp_dpcd_read() level, for
reads returning 0..len-1 bytes. Not necessarily part of this series, but
seems silly to set a precedent to start handling that return value all
over the place.
BR,
Jani.
> +
> + return DP_GET_SINK_COUNT(count);
> +}
> +EXPORT_SYMBOL(drm_dp_get_sink_count);
> +
> /*
> * I2C-over-AUX implementation
> */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 35a4779a442e2..4337321a3be4f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4648,6 +4648,8 @@ intel_dp_has_sink_count(struct intel_dp *intel_dp)
> static bool
> intel_dp_get_dpcd(struct intel_dp *intel_dp)
> {
> + int ret;
> +
> if (!intel_dp_read_dpcd(intel_dp))
> return false;
>
> @@ -4664,11 +4666,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> }
>
> if (intel_dp_has_sink_count(intel_dp)) {
> - u8 count;
> - ssize_t r;
> -
> - r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count);
> - if (r < 1)
> + ret = drm_dp_get_sink_count(&intel_dp->aux);
> + if (ret < 0)
> return false;
>
> /*
> @@ -4676,7 +4675,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> * a member variable in intel_dp will track any changes
> * between short pulse interrupts.
> */
> - intel_dp->sink_count = DP_GET_SINK_COUNT(count);
> + intel_dp->sink_count = ret;
>
> /*
> * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index a1413a531eaf4..0c141fc81aaa8 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1635,6 +1635,7 @@ struct drm_dp_desc;
> bool drm_dp_has_sink_count(struct drm_connector *connector,
> const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> const struct drm_dp_desc *desc);
> +int drm_dp_get_sink_count(struct drm_dp_aux *aux);
>
> void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
> void drm_dp_aux_init(struct drm_dp_aux *aux);
--
Jani Nikula, Intel Open Source Graphics Center
On Wed, 2020-08-26 at 10:05 +0300, Jani Nikula wrote:
> On Tue, 25 Aug 2020, Lyude Paul <[email protected]> wrote:
> > And of course, we'll also need to read the sink count from other drivers
> > as well if we're checking whether or not it's supported. So, let's
> > extract the code for this into another helper.
> >
> > v2:
> > * Fix drm_dp_dpcd_readb() ret check
> > * Add back comment and move back sink_count assignment in
> > intel_dp_get_dpcd()
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Reviewed-by: Sean Paul <[email protected]>
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 22 ++++++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++------
> > include/drm/drm_dp_helper.h | 1 +
> > 3 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 863e0babc1903..67ad05eb05b7e 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -736,6 +736,28 @@ bool drm_dp_has_sink_count(struct drm_connector
> > *connector,
> > }
> > EXPORT_SYMBOL(drm_dp_has_sink_count);
> >
> > +/**
> > + * drm_dp_get_sink_count() - Retrieve the sink count for a given sink
>
> From the department of bikeshedding...
>
> Should we have a naming scheme where it's obvious whether a function
> will do DPCD access, or just shuffle existing data?
>
> For example, drm_dp_read_foo() for anything with DPCD access
> vs. drm_dp_get_foo() or even simpler for anything that only processes
> pre-read data?
>
> > + * @aux: The DP AUX channel to use
> > + *
> > + * Returns: The current sink count reported by @aux, or a negative error
> > code
> > + * otherwise.
> > + */
> > +int drm_dp_get_sink_count(struct drm_dp_aux *aux)
> > +{
> > + u8 count;
> > + int ret;
> > +
> > + ret = drm_dp_dpcd_readb(aux, DP_SINK_COUNT, &count);
> > + if (ret < 0)
> > + return ret;
> > + if (ret != 1)
> > + return -EIO;
>
> Makes me wonder if that shouldn't be at drm_dp_dpcd_read() level, for
> reads returning 0..len-1 bytes. Not necessarily part of this series, but
> seems silly to set a precedent to start handling that return value all
> over the place.
>
Yeah definitely - I'm probably going to keep this code here for now, but I would
like to convert drm_dp_dpcd_readb/writeb() to just return 0 on success (all
bytes written, I've never once seen a situation where we got less bytes than we
read - it's always all or nothing) and negative error code on failure. I'll get
to that soon
> BR,
> Jani.
>
> > +
> > + return DP_GET_SINK_COUNT(count);
> > +}
> > +EXPORT_SYMBOL(drm_dp_get_sink_count);
> > +
> > /*
> > * I2C-over-AUX implementation
> > */
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 35a4779a442e2..4337321a3be4f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4648,6 +4648,8 @@ intel_dp_has_sink_count(struct intel_dp *intel_dp)
> > static bool
> > intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > {
> > + int ret;
> > +
> > if (!intel_dp_read_dpcd(intel_dp))
> > return false;
> >
> > @@ -4664,11 +4666,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > }
> >
> > if (intel_dp_has_sink_count(intel_dp)) {
> > - u8 count;
> > - ssize_t r;
> > -
> > - r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count);
> > - if (r < 1)
> > + ret = drm_dp_get_sink_count(&intel_dp->aux);
> > + if (ret < 0)
> > return false;
> >
> > /*
> > @@ -4676,7 +4675,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > * a member variable in intel_dp will track any changes
> > * between short pulse interrupts.
> > */
> > - intel_dp->sink_count = DP_GET_SINK_COUNT(count);
> > + intel_dp->sink_count = ret;
> >
> > /*
> > * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index a1413a531eaf4..0c141fc81aaa8 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1635,6 +1635,7 @@ struct drm_dp_desc;
> > bool drm_dp_has_sink_count(struct drm_connector *connector,
> > const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > const struct drm_dp_desc *desc);
> > +int drm_dp_get_sink_count(struct drm_dp_aux *aux);
> >
> > void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
> > void drm_dp_aux_init(struct drm_dp_aux *aux);
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
On Wed, 2020-08-26 at 10:05 +0300, Jani Nikula wrote:
> On Tue, 25 Aug 2020, Lyude Paul <[email protected]> wrote:
> > And of course, we'll also need to read the sink count from other drivers
> > as well if we're checking whether or not it's supported. So, let's
> > extract the code for this into another helper.
> >
> > v2:
> > * Fix drm_dp_dpcd_readb() ret check
> > * Add back comment and move back sink_count assignment in
> > intel_dp_get_dpcd()
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Reviewed-by: Sean Paul <[email protected]>
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 22 ++++++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++------
> > include/drm/drm_dp_helper.h | 1 +
> > 3 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 863e0babc1903..67ad05eb05b7e 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -736,6 +736,28 @@ bool drm_dp_has_sink_count(struct drm_connector
> > *connector,
> > }
> > EXPORT_SYMBOL(drm_dp_has_sink_count);
> >
> > +/**
> > + * drm_dp_get_sink_count() - Retrieve the sink count for a given sink
>
> From the department of bikeshedding...
>
> Should we have a naming scheme where it's obvious whether a function
> will do DPCD access, or just shuffle existing data?
>
> For example, drm_dp_read_foo() for anything with DPCD access
> vs. drm_dp_get_foo() or even simpler for anything that only processes
> pre-read data?
Forgot to address this comment - yeah, I think that would be a good idea. I'll
go through my previous patches and make sure that they match this naming scheme
as well.
>
> > + * @aux: The DP AUX channel to use
> > + *
> > + * Returns: The current sink count reported by @aux, or a negative error
> > code
> > + * otherwise.
> > + */
> > +int drm_dp_get_sink_count(struct drm_dp_aux *aux)
> > +{
> > + u8 count;
> > + int ret;
> > +
> > + ret = drm_dp_dpcd_readb(aux, DP_SINK_COUNT, &count);
> > + if (ret < 0)
> > + return ret;
> > + if (ret != 1)
> > + return -EIO;
>
> Makes me wonder if that shouldn't be at drm_dp_dpcd_read() level, for
> reads returning 0..len-1 bytes. Not necessarily part of this series, but
> seems silly to set a precedent to start handling that return value all
> over the place.
>
> BR,
> Jani.
>
> > +
> > + return DP_GET_SINK_COUNT(count);
> > +}
> > +EXPORT_SYMBOL(drm_dp_get_sink_count);
> > +
> > /*
> > * I2C-over-AUX implementation
> > */
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 35a4779a442e2..4337321a3be4f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4648,6 +4648,8 @@ intel_dp_has_sink_count(struct intel_dp *intel_dp)
> > static bool
> > intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > {
> > + int ret;
> > +
> > if (!intel_dp_read_dpcd(intel_dp))
> > return false;
> >
> > @@ -4664,11 +4666,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > }
> >
> > if (intel_dp_has_sink_count(intel_dp)) {
> > - u8 count;
> > - ssize_t r;
> > -
> > - r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count);
> > - if (r < 1)
> > + ret = drm_dp_get_sink_count(&intel_dp->aux);
> > + if (ret < 0)
> > return false;
> >
> > /*
> > @@ -4676,7 +4675,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > * a member variable in intel_dp will track any changes
> > * between short pulse interrupts.
> > */
> > - intel_dp->sink_count = DP_GET_SINK_COUNT(count);
> > + intel_dp->sink_count = ret;
> >
> > /*
> > * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index a1413a531eaf4..0c141fc81aaa8 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1635,6 +1635,7 @@ struct drm_dp_desc;
> > bool drm_dp_has_sink_count(struct drm_connector *connector,
> > const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > const struct drm_dp_desc *desc);
> > +int drm_dp_get_sink_count(struct drm_dp_aux *aux);
> >
> > void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
> > void drm_dp_aux_init(struct drm_dp_aux *aux);
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat