* [RFC] drm: Add utility function to check for edp1.4 @ 2014-10-22 6:15 sonika.jindal 2014-10-29 13:42 ` Thierry Reding 0 siblings, 1 reply; 7+ messages in thread From: sonika.jindal @ 2014-10-22 6:15 UTC (permalink / raw) To: intel-gfx, dri-devel; +Cc: alexdeucher, treding, daniel.vetter From: Sonika Jindal <sonika.jindal@intel.com> v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) v3: Moving the utility function to drm_dp_helper (Daniel) Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ include/drm/drm_dp_helper.h | 2 ++ 2 files changed, 17 insertions(+) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 08e33b8..a54a760 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister); + +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) +{ + uint8_t reg; + + if (dpcd[DP_EDP_CONFIGURATION_CAP] & + DP_DPCD_DISPLAY_CONTROL_CAPABLE) { + + if (drm_dp_dpcd_read(aux, DP_EDP_REV, ®, 1)) + if (reg == 0x03) + return true; + } + return false; +} +EXPORT_SYMBOL(drm_dp_is_edp_v1_4); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 8edeed0..b017e1e 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -102,6 +102,8 @@ #define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) +#define DP_EDP_REV 0x700 /* Multiple stream transport */ #define DP_FAUX_CAP 0x020 /* 1.2 */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC] drm: Add utility function to check for edp1.4 2014-10-22 6:15 [RFC] drm: Add utility function to check for edp1.4 sonika.jindal @ 2014-10-29 13:42 ` Thierry Reding 2014-10-30 4:14 ` sonika 2014-10-31 16:06 ` Daniel Vetter 0 siblings, 2 replies; 7+ messages in thread From: Thierry Reding @ 2014-10-29 13:42 UTC (permalink / raw) To: sonika.jindal; +Cc: alexdeucher, intel-gfx, dri-devel, daniel.vetter [-- Attachment #1.1: Type: text/plain, Size: 2944 bytes --] On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) > > v3: Moving the utility function to drm_dp_helper (Daniel) > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ > include/drm/drm_dp_helper.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 08e33b8..a54a760 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) > i2c_del_adapter(&aux->ddc); > } > EXPORT_SYMBOL(drm_dp_aux_unregister); > + > +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) I'd prefer if this didn't take a dpcd argument but rather directly accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used directly rather than rely on the driver to have read a dpcd block in the appropriate format. > +{ > + uint8_t reg; > + > + if (dpcd[DP_EDP_CONFIGURATION_CAP] & > + DP_DPCD_DISPLAY_CONTROL_CAPABLE) { > + > + if (drm_dp_dpcd_read(aux, DP_EDP_REV, ®, 1)) > + if (reg == 0x03) > + return true; > + } > + return false; > +} > +EXPORT_SYMBOL(drm_dp_is_edp_v1_4); Does it make sense to have a function that checks for a specific version? Why not add one that returns the revision so that it can be compared, something like: u8 value; drm_dp_dpcd_read(aux, DP_EDP_REV, &value, 1); return value; Then we can do something like: #define DP_EDP_REV_1_1 0x00 #define DP_EDP_REV_1_2 0x01 #define DP_EDP_REV_1_3 0x02 #define DP_EDP_REV_1_4 0x03 And code can simply compare against that: drm_dp_get_edp_revision(aux, &rev); if (rev >= DP_EDP_REV_1_4) { ... } The check in your variant will only match v1.4 exactly, but presumably v1.5 will be backwards compatible. Having a direct check on the revision code will allow code to continue to work with future, backwards- compatible revisions. > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 8edeed0..b017e1e 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -102,6 +102,8 @@ > > #define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) This seems to be a field in the DP_EDP_CONFIGURATION_CAP register, so it should be sorted below that register, not DP_TRAINING_AUX_RD_INTERVAL. > +#define DP_EDP_REV 0x700 And this belongs further down, so it properly sorts into the list of registers. Thierry [-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm: Add utility function to check for edp1.4 2014-10-29 13:42 ` Thierry Reding @ 2014-10-30 4:14 ` sonika 2014-10-31 16:06 ` Daniel Vetter 1 sibling, 0 replies; 7+ messages in thread From: sonika @ 2014-10-30 4:14 UTC (permalink / raw) To: Thierry Reding; +Cc: daniel.vetter, intel-gfx, dri-devel, s, alexdeucher Thanks for your comments Thierry. I agree to all your comments. I will write a general function to return version and repost the patch Thanks, Sonika On Wednesday 29 October 2014 07:12 PM, Thierry Reding wrote: > On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: >> From: Sonika Jindal <sonika.jindal@intel.com> >> >> v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) >> >> v3: Moving the utility function to drm_dp_helper (Daniel) >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ >> include/drm/drm_dp_helper.h | 2 ++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c >> index 08e33b8..a54a760 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) >> i2c_del_adapter(&aux->ddc); >> } >> EXPORT_SYMBOL(drm_dp_aux_unregister); >> + >> +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > I'd prefer if this didn't take a dpcd argument but rather directly > accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used > directly rather than rely on the driver to have read a dpcd block in the > appropriate format. > >> +{ >> + uint8_t reg; >> + >> + if (dpcd[DP_EDP_CONFIGURATION_CAP] & >> + DP_DPCD_DISPLAY_CONTROL_CAPABLE) { >> + >> + if (drm_dp_dpcd_read(aux, DP_EDP_REV, ®, 1)) >> + if (reg == 0x03) >> + return true; >> + } >> + return false; >> +} >> +EXPORT_SYMBOL(drm_dp_is_edp_v1_4); > Does it make sense to have a function that checks for a specific > version? Why not add one that returns the revision so that it can be > compared, something like: > > u8 value; > > drm_dp_dpcd_read(aux, DP_EDP_REV, &value, 1); > > return value; > > Then we can do something like: > > #define DP_EDP_REV_1_1 0x00 > #define DP_EDP_REV_1_2 0x01 > #define DP_EDP_REV_1_3 0x02 > #define DP_EDP_REV_1_4 0x03 > > And code can simply compare against that: > > drm_dp_get_edp_revision(aux, &rev); > > if (rev >= DP_EDP_REV_1_4) { > ... > } > > The check in your variant will only match v1.4 exactly, but presumably > v1.5 will be backwards compatible. Having a direct check on the revision > code will allow code to continue to work with future, backwards- > compatible revisions. > >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >> index 8edeed0..b017e1e 100644 >> --- a/include/drm/drm_dp_helper.h >> +++ b/include/drm/drm_dp_helper.h >> @@ -102,6 +102,8 @@ >> >> #define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ >> #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ >> +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) > This seems to be a field in the DP_EDP_CONFIGURATION_CAP register, so it > should be sorted below that register, not DP_TRAINING_AUX_RD_INTERVAL. > >> +#define DP_EDP_REV 0x700 > And this belongs further down, so it properly sorts into the list of > registers. > > Thierry _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm: Add utility function to check for edp1.4 2014-10-29 13:42 ` Thierry Reding 2014-10-30 4:14 ` sonika @ 2014-10-31 16:06 ` Daniel Vetter 2014-11-03 8:25 ` Thierry Reding 1 sibling, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2014-10-31 16:06 UTC (permalink / raw) To: Thierry Reding; +Cc: daniel.vetter, intel-gfx, dri-devel, alexdeucher On Wed, Oct 29, 2014 at 02:42:29PM +0100, Thierry Reding wrote: > On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: > > From: Sonika Jindal <sonika.jindal@intel.com> > > > > v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) > > > > v3: Moving the utility function to drm_dp_helper (Daniel) > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > --- > > drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ > > include/drm/drm_dp_helper.h | 2 ++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 08e33b8..a54a760 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) > > i2c_del_adapter(&aux->ddc); > > } > > EXPORT_SYMBOL(drm_dp_aux_unregister); > > + > > +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > > I'd prefer if this didn't take a dpcd argument but rather directly > accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used > directly rather than rely on the driver to have read a dpcd block in the > appropriate format. The idea is that you'd grab the DPCD field anyway since it's needed all over the place. We have a pile of helpers already that take exactly this block and decode parts of it. So I think this makes sense - dp aux is fast but not entirely free, so caching seems useful. It's a bit strange then that this helper reads more code, but I guess you really only care about the edp_rev field if you want the edp revision, and with your interface suggestion I don't think we'd ever need to read this again. So for me the input fields make sense. I agree that for the output of the function an enum with all the edp revisions is better. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm: Add utility function to check for edp1.4 2014-10-31 16:06 ` Daniel Vetter @ 2014-11-03 8:25 ` Thierry Reding 2014-11-03 8:28 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Thierry Reding @ 2014-11-03 8:25 UTC (permalink / raw) To: Daniel Vetter; +Cc: daniel.vetter, intel-gfx, dri-devel, alexdeucher [-- Attachment #1.1: Type: text/plain, Size: 1876 bytes --] On Fri, Oct 31, 2014 at 05:06:39PM +0100, Daniel Vetter wrote: > On Wed, Oct 29, 2014 at 02:42:29PM +0100, Thierry Reding wrote: > > On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: > > > From: Sonika Jindal <sonika.jindal@intel.com> > > > > > > v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) > > > > > > v3: Moving the utility function to drm_dp_helper (Daniel) > > > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > --- > > > drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ > > > include/drm/drm_dp_helper.h | 2 ++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index 08e33b8..a54a760 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) > > > i2c_del_adapter(&aux->ddc); > > > } > > > EXPORT_SYMBOL(drm_dp_aux_unregister); > > > + > > > +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > > > > I'd prefer if this didn't take a dpcd argument but rather directly > > accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used > > directly rather than rely on the driver to have read a dpcd block in the > > appropriate format. > > The idea is that you'd grab the DPCD field anyway since it's needed all > over the place. We have a pile of helpers already that take exactly this > block and decode parts of it. So I think this makes sense - dp aux is fast > but not entirely free, so caching seems useful. If we want to always cache part of the DPCD wouldn't it be better to add the cache to struct drm_dp_aux instead of having to duplicate this in every driver? Thierry [-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm: Add utility function to check for edp1.4 2014-11-03 8:25 ` Thierry Reding @ 2014-11-03 8:28 ` Daniel Vetter 2014-11-04 5:55 ` sonika 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2014-11-03 8:28 UTC (permalink / raw) To: Thierry Reding; +Cc: intel-gfx, satheeshakrishna.m, dri-devel On Mon, Nov 3, 2014 at 9:25 AM, Thierry Reding <treding@nvidia.com> wrote: >> The idea is that you'd grab the DPCD field anyway since it's needed all >> over the place. We have a pile of helpers already that take exactly this >> block and decode parts of it. So I think this makes sense - dp aux is fast >> but not entirely free, so caching seems useful. > > If we want to always cache part of the DPCD wouldn't it be better to add > the cache to struct drm_dp_aux instead of having to duplicate this in > every driver? Right now we don't (yet) have helper functions to probe SST DP sinks, so there's not really a good place to put it. But yeah longer-term I hope that some shared SST sink probe functions shows up (especially to handle all the various crazy corner-cases with VGA probing, e.g. the apple vga dongle patch Dave sent out many aeons ago), and then putting that cache into the dp aux struct makes sense. But before that happens I don't think it makes a lot of sense really, since then we'd also need to add code to invalidate that cache. Simpler if drivers just take care of that themselves. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm: Add utility function to check for edp1.4 2014-11-03 8:28 ` Daniel Vetter @ 2014-11-04 5:55 ` sonika 0 siblings, 0 replies; 7+ messages in thread From: sonika @ 2014-11-04 5:55 UTC (permalink / raw) To: Daniel Vetter, Thierry Reding; +Cc: Alex Deucher, intel-gfx, dri-devel On Monday 03 November 2014 01:58 PM, Daniel Vetter wrote: > On Mon, Nov 3, 2014 at 9:25 AM, Thierry Reding <treding@nvidia.com> wrote: >>> The idea is that you'd grab the DPCD field anyway since it's needed all >>> over the place. We have a pile of helpers already that take exactly this >>> block and decode parts of it. So I think this makes sense - dp aux is fast >>> but not entirely free, so caching seems useful. >> If we want to always cache part of the DPCD wouldn't it be better to add >> the cache to struct drm_dp_aux instead of having to duplicate this in >> every driver? > Right now we don't (yet) have helper functions to probe SST DP sinks, > so there's not really a good place to put it. But yeah longer-term I > hope that some shared SST sink probe functions shows up (especially to > handle all the various crazy corner-cases with VGA probing, e.g. the > apple vga dongle patch Dave sent out many aeons ago), and then putting > that cache into the dp aux struct makes sense. But before that happens > I don't think it makes a lot of sense really, since then we'd also > need to add code to invalidate that cache. Simpler if drivers just > take care of that themselves. > -Daniel For the edp revision, I am feeling that having a function which simply reads DP_EDP_REV, and returns it doesn't add much value. So, I am thinking to put that part in driver itself. Please let me know if you feel otherwise. -Sonika _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-11-04 5:55 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-22 6:15 [RFC] drm: Add utility function to check for edp1.4 sonika.jindal 2014-10-29 13:42 ` Thierry Reding 2014-10-30 4:14 ` sonika 2014-10-31 16:06 ` Daniel Vetter 2014-11-03 8:25 ` Thierry Reding 2014-11-03 8:28 ` Daniel Vetter 2014-11-04 5:55 ` sonika
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox