| Message ID | 20200825195027.74681-17-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dp, i915, nouveau: Cleanup nouveau HPD and add DP features from i915 | expand |
On Tue, 25 Aug 2020, Lyude Paul <lyude@redhat.com> 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 <lyude@redhat.com> > Reviewed-by: Sean Paul <sean@poorly.run> > --- > 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);
On Wed, 2020-08-26 at 10:05 +0300, Jani Nikula wrote: > On Tue, 25 Aug 2020, Lyude Paul <lyude@redhat.com> 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 <lyude@redhat.com> > > Reviewed-by: Sean Paul <sean@poorly.run> > > --- > > 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);
On Wed, 2020-08-26 at 10:05 +0300, Jani Nikula wrote: > On Tue, 25 Aug 2020, Lyude Paul <lyude@redhat.com> 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 <lyude@redhat.com> > > Reviewed-by: Sean Paul <sean@poorly.run> > > --- > > 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);
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);