[PATCH 1/2] drm/dp: add eDP 1.2 display control DPCD register definitions

[PATCH 1/2] drm/dp: add eDP 1.2 display control DPCD register definitions

[Jani Nikula]

Preparing for the future eDP panels.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/drm/drm_dp_helper.h |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index a92c375..e2dbde6 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -100,6 +100,9 @@
 # define DP_I2C_SPEED_1M		    0x20
 
 #define DP_EDP_CONFIGURATION_CAP            0x00d   /* XXX 1.2? */
+# define DP_ALTERNATE_SCRAMBLER_RESET_CAP   (1 << 0)
+# define DP_FRAMING_CHANGE_CAP              (1 << 1)
+# define DP_DPCD_DISPLAY_CONTROL_CAP        (1 << 3) /* eDP 1.2 */
 #define DP_TRAINING_AUX_RD_INTERVAL         0x00e   /* XXX 1.2? */
 
 /* Multiple stream transport */
@@ -291,6 +294,32 @@
 # define DP_SET_POWER_D0                    0x1
 # define DP_SET_POWER_D3                    0x2
 
+#define DP_EDP_REV                          0x700 /* eDP 1.2 */
+
+#define DP_EDP_GENERAL_CAPABILITY_REGISTER_1               0x701
+#define DP_EDP_BACKLIGHT_ADJUSTMENT_CAPABILITY_REGISTER    0x702
+#define DP_EDP_GENERAL_CAPABILITY_REGISTER_2               0x703
+
+#define DP_EDP_DISPLAY_CONTROL_REGISTER     0x720
+#define DP_EDP_BACKLIGHT_MODE_SET_REGISTER  0x721
+#define DP_EDP_BACKLIGHT_BRIGHTNESS_MSB     0x722
+#define DP_EDP_BACKLIGHT_BRIGHTNESS_LSB     0x723
+#define DP_EDP_PWMGEN_BIT_COUNT             0x724
+#define DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN     0x725
+#define DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX     0x726
+#define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
+#define DP_EDP_BACKLIGHT_FREQ_SET           0x728
+
+#define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
+#define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
+#define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_LSB   0x72c
+#define DP_EDP_BACKLIGHT_FREQ_CAP_MAX_MSB   0x72d
+#define DP_EDP_BACKLIGHT_FREQ_CAP_MAX_MID   0x72e
+#define DP_EDP_BACKLIGHT_FREQ_CAP_MAX_LSB   0x72f
+
+#define DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET   0x732
+#define DP_EDP_DBC_MAXIMUM_BRIGHTNESS_SET   0x733
+
 #define DP_PSR_ERROR_STATUS                 0x2006  /* XXX 1.2? */
 # define DP_PSR_LINK_CRC_ERROR              (1 << 0)
 # define DP_PSR_RFB_STORAGE_ERROR           (1 << 1)
-- 
1.7.9.5

[PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Jani Nikula]

Debug print the capabilities, and flag an error if the panel does not
support adjusting backlight through the BL_PWM_DIM pin, requiring
backlight control through DPCD.

I haven't seen such panels yet, but it's a matter of time. Give
ourselves a reminder when we need to fix this for real.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cbf33be..ea4f3d1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 			dev_priv->psr.sink_support = true;
 			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
 		}
+
+		if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &
+		    DP_DPCD_DISPLAY_CONTROL_CAP) {
+			u8 ctrl[4] = { 0 };
+
+			intel_dp_aux_native_read(intel_dp, DP_EDP_REV,
+						 ctrl, sizeof(ctrl));
+			DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x\n",
+				      ctrl[0], ctrl[1], ctrl[2], ctrl[3]);
+
+			/* We don't support DPCD backlight control yet. */
+			if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1))
+				DRM_ERROR("eDP AUX backlight control only\n");
+		}
 	}
 
 	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
-- 
1.7.9.5

Re: [PATCH 1/2] drm/dp: add eDP 1.2 display control DPCD register definitions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 325 bytes --]

On Fri, Nov 15, 2013 at 03:01:50PM +0200, Jani Nikula wrote:
> Preparing for the future eDP panels.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  include/drm/drm_dp_helper.h |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2842 bytes --]

On Fri, Nov 15, 2013 at 03:01:51PM +0200, Jani Nikula wrote:
> Debug print the capabilities, and flag an error if the panel does not
> support adjusting backlight through the BL_PWM_DIM pin, requiring
> backlight control through DPCD.
> 
> I haven't seen such panels yet, but it's a matter of time. Give
> ourselves a reminder when we need to fix this for real.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)

I have a few general comments below, but this patch itself look fine,
so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cbf33be..ea4f3d1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  			dev_priv->psr.sink_support = true;
>  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>  		}
> +
> +		if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &
> +		    DP_DPCD_DISPLAY_CONTROL_CAP) {
> +			u8 ctrl[4] = { 0 };
> +
> +			intel_dp_aux_native_read(intel_dp, DP_EDP_REV,
> +						 ctrl, sizeof(ctrl));
> +			DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x\n",
> +				      ctrl[0], ctrl[1], ctrl[2], ctrl[3]);
> +
> +			/* We don't support DPCD backlight control yet. */
> +			if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1))
> +				DRM_ERROR("eDP AUX backlight control only\n");
> +		}
>  	}
>  
>  	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &

I think a lot of eDP utility code could be made reusable across drivers.
We could probably do that by having each driver expose a drm_edp object
of some sort.

Actually, the same would be true of DP in general. Accessing the DPCD is
something that's driver specific, but once you know how to do that a lot
of code can be made generic. I think a struct drm_dp could look like
this:

	struct drm_dp;

	struct drm_dpcd_ops {
		ssize_t (*read)(struct drm_dp *dp, unsigned int offset,
				void *buffer, size_t size);
		ssize_t (*write)(struct drm_dp *dp, unsigned int offset,
				 const void *buffer, size_t size);
	};

	struct drm_dp {
		const struct drm_dpcd_ops *dpcd;
	};

Perhaps that could even be extended with functionality to implement link
training in a generic way. There are already quite a few helpers to help
with that in drivers/gpu/drm/drm_dp_helper.c, but they assume that the
DPCD will be handed to them as a large buffer and therefore cannot write
DPCD registers.

I suppose one could argue that it would be introducing a mid-layer, but
that layer would be really thin in my opinion. And it would allow a lot
of the algorithms to be written only once instead of multiple times.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 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

Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Alex Deucher]

On Mon, Nov 18, 2013 at 9:27 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Nov 15, 2013 at 03:01:51PM +0200, Jani Nikula wrote:
>> Debug print the capabilities, and flag an error if the panel does not
>> support adjusting backlight through the BL_PWM_DIM pin, requiring
>> backlight control through DPCD.
>>
>> I haven't seen such panels yet, but it's a matter of time. Give
>> ourselves a reminder when we need to fix this for real.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c |   14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>
> I have a few general comments below, but this patch itself look fine,
> so:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index cbf33be..ea4f3d1 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>                       dev_priv->psr.sink_support = true;
>>                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>               }
>> +
>> +             if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &
>> +                 DP_DPCD_DISPLAY_CONTROL_CAP) {
>> +                     u8 ctrl[4] = { 0 };
>> +
>> +                     intel_dp_aux_native_read(intel_dp, DP_EDP_REV,
>> +                                              ctrl, sizeof(ctrl));
>> +                     DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x\n",
>> +                                   ctrl[0], ctrl[1], ctrl[2], ctrl[3]);
>> +
>> +                     /* We don't support DPCD backlight control yet. */
>> +                     if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1))
>> +                             DRM_ERROR("eDP AUX backlight control only\n");
>> +             }
>>       }
>>
>>       if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>
> I think a lot of eDP utility code could be made reusable across drivers.
> We could probably do that by having each driver expose a drm_edp object
> of some sort.
>
> Actually, the same would be true of DP in general. Accessing the DPCD is
> something that's driver specific, but once you know how to do that a lot
> of code can be made generic. I think a struct drm_dp could look like
> this:
>
>         struct drm_dp;
>
>         struct drm_dpcd_ops {
>                 ssize_t (*read)(struct drm_dp *dp, unsigned int offset,
>                                 void *buffer, size_t size);
>                 ssize_t (*write)(struct drm_dp *dp, unsigned int offset,
>                                  const void *buffer, size_t size);
>         };
>
>         struct drm_dp {
>                 const struct drm_dpcd_ops *dpcd;
>         };
>
> Perhaps that could even be extended with functionality to implement link
> training in a generic way. There are already quite a few helpers to help
> with that in drivers/gpu/drm/drm_dp_helper.c, but they assume that the
> DPCD will be handed to them as a large buffer and therefore cannot write
> DPCD registers.
>
> I suppose one could argue that it would be introducing a mid-layer, but
> that layer would be really thin in my opinion. And it would allow a lot
> of the algorithms to be written only once instead of multiple times.

I think it could probably be made to work.  The tricky part would be
hw specific ordering in the training sequence.  At the very minimum,
you need driver callbacks to set up the source side:

set_training_pattern()
set_vs_emph()

And probably some flags to indicate whether the the hw supports
specific features like training pattern 3.

Alex

Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4411 bytes --]

On Mon, Nov 18, 2013 at 10:09:56AM -0500, Alex Deucher wrote:
> On Mon, Nov 18, 2013 at 9:27 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Nov 15, 2013 at 03:01:51PM +0200, Jani Nikula wrote:
> >> Debug print the capabilities, and flag an error if the panel does not
> >> support adjusting backlight through the BL_PWM_DIM pin, requiring
> >> backlight control through DPCD.
> >>
> >> I haven't seen such panels yet, but it's a matter of time. Give
> >> ourselves a reminder when we need to fix this for real.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c |   14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >
> > I have a few general comments below, but this patch itself look fine,
> > so:
> >
> > Reviewed-by: Thierry Reding <treding@nvidia.com>
> >
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index cbf33be..ea4f3d1 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >>                       dev_priv->psr.sink_support = true;
> >>                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >>               }
> >> +
> >> +             if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &
> >> +                 DP_DPCD_DISPLAY_CONTROL_CAP) {
> >> +                     u8 ctrl[4] = { 0 };
> >> +
> >> +                     intel_dp_aux_native_read(intel_dp, DP_EDP_REV,
> >> +                                              ctrl, sizeof(ctrl));
> >> +                     DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x\n",
> >> +                                   ctrl[0], ctrl[1], ctrl[2], ctrl[3]);
> >> +
> >> +                     /* We don't support DPCD backlight control yet. */
> >> +                     if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1))
> >> +                             DRM_ERROR("eDP AUX backlight control only\n");
> >> +             }
> >>       }
> >>
> >>       if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >
> > I think a lot of eDP utility code could be made reusable across drivers.
> > We could probably do that by having each driver expose a drm_edp object
> > of some sort.
> >
> > Actually, the same would be true of DP in general. Accessing the DPCD is
> > something that's driver specific, but once you know how to do that a lot
> > of code can be made generic. I think a struct drm_dp could look like
> > this:
> >
> >         struct drm_dp;
> >
> >         struct drm_dpcd_ops {
> >                 ssize_t (*read)(struct drm_dp *dp, unsigned int offset,
> >                                 void *buffer, size_t size);
> >                 ssize_t (*write)(struct drm_dp *dp, unsigned int offset,
> >                                  const void *buffer, size_t size);
> >         };
> >
> >         struct drm_dp {
> >                 const struct drm_dpcd_ops *dpcd;
> >         };
> >
> > Perhaps that could even be extended with functionality to implement link
> > training in a generic way. There are already quite a few helpers to help
> > with that in drivers/gpu/drm/drm_dp_helper.c, but they assume that the
> > DPCD will be handed to them as a large buffer and therefore cannot write
> > DPCD registers.
> >
> > I suppose one could argue that it would be introducing a mid-layer, but
> > that layer would be really thin in my opinion. And it would allow a lot
> > of the algorithms to be written only once instead of multiple times.
> 
> I think it could probably be made to work.  The tricky part would be
> hw specific ordering in the training sequence.  At the very minimum,
> you need driver callbacks to set up the source side:
> 
> set_training_pattern()
> set_vs_emph()
> 
> And probably some flags to indicate whether the the hw supports
> specific features like training pattern 3.

Yes, something along those lines was what I had in mind as well. I know
that many people are unhappy about introducing this kind of mid-layer,
but quite frankly, doing this in generic code must have been one of the
primary reasons why VESA specified it that way.

The alternative will be to repeat more or less the same code in all the
drivers. I don't think that's a very nice alternative.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 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

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Daniel Vetter]

On Mon, Nov 18, 2013 at 04:26:17PM +0100, Thierry Reding wrote:
> On Mon, Nov 18, 2013 at 10:09:56AM -0500, Alex Deucher wrote:
> > On Mon, Nov 18, 2013 at 9:27 AM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > > On Fri, Nov 15, 2013 at 03:01:51PM +0200, Jani Nikula wrote:
> > >> Debug print the capabilities, and flag an error if the panel does not
> > >> support adjusting backlight through the BL_PWM_DIM pin, requiring
> > >> backlight control through DPCD.
> > >>
> > >> I haven't seen such panels yet, but it's a matter of time. Give
> > >> ourselves a reminder when we need to fix this for real.
> > >>
> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_dp.c |   14 ++++++++++++++
> > >>  1 file changed, 14 insertions(+)
> > >
> > > I have a few general comments below, but this patch itself look fine,
> > > so:
> > >
> > > Reviewed-by: Thierry Reding <treding@nvidia.com>
> > >
> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > >> index cbf33be..ea4f3d1 100644
> > >> --- a/drivers/gpu/drm/i915/intel_dp.c
> > >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> > >> @@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >>                       dev_priv->psr.sink_support = true;
> > >>                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > >>               }
> > >> +
> > >> +             if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &
> > >> +                 DP_DPCD_DISPLAY_CONTROL_CAP) {
> > >> +                     u8 ctrl[4] = { 0 };
> > >> +
> > >> +                     intel_dp_aux_native_read(intel_dp, DP_EDP_REV,
> > >> +                                              ctrl, sizeof(ctrl));
> > >> +                     DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x\n",
> > >> +                                   ctrl[0], ctrl[1], ctrl[2], ctrl[3]);
> > >> +
> > >> +                     /* We don't support DPCD backlight control yet. */
> > >> +                     if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1))
> > >> +                             DRM_ERROR("eDP AUX backlight control only\n");
> > >> +             }
> > >>       }
> > >>
> > >>       if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > >
> > > I think a lot of eDP utility code could be made reusable across drivers.
> > > We could probably do that by having each driver expose a drm_edp object
> > > of some sort.
> > >
> > > Actually, the same would be true of DP in general. Accessing the DPCD is
> > > something that's driver specific, but once you know how to do that a lot
> > > of code can be made generic. I think a struct drm_dp could look like
> > > this:
> > >
> > >         struct drm_dp;
> > >
> > >         struct drm_dpcd_ops {
> > >                 ssize_t (*read)(struct drm_dp *dp, unsigned int offset,
> > >                                 void *buffer, size_t size);
> > >                 ssize_t (*write)(struct drm_dp *dp, unsigned int offset,
> > >                                  const void *buffer, size_t size);
> > >         };
> > >
> > >         struct drm_dp {
> > >                 const struct drm_dpcd_ops *dpcd;
> > >         };
> > >
> > > Perhaps that could even be extended with functionality to implement link
> > > training in a generic way. There are already quite a few helpers to help
> > > with that in drivers/gpu/drm/drm_dp_helper.c, but they assume that the
> > > DPCD will be handed to them as a large buffer and therefore cannot write
> > > DPCD registers.
> > >
> > > I suppose one could argue that it would be introducing a mid-layer, but
> > > that layer would be really thin in my opinion. And it would allow a lot
> > > of the algorithms to be written only once instead of multiple times.
> > 
> > I think it could probably be made to work.  The tricky part would be
> > hw specific ordering in the training sequence.  At the very minimum,
> > you need driver callbacks to set up the source side:
> > 
> > set_training_pattern()
> > set_vs_emph()
> > 
> > And probably some flags to indicate whether the the hw supports
> > specific features like training pattern 3.
> 
> Yes, something along those lines was what I had in mind as well. I know
> that many people are unhappy about introducing this kind of mid-layer,
> but quite frankly, doing this in generic code must have been one of the
> primary reasons why VESA specified it that way.
> 
> The alternative will be to repeat more or less the same code in all the
> drivers. I don't think that's a very nice alternative.

My plan (which is still somewhere on the todo but hasn't otherwise
materilized) was to extract the dp aux handling code first. There's a lot
of common code we could extract for i2c-over-dp-aux, handling branch
devices and other stuff. Once we have that we can spill tons of little
helper functions all over the place to decode interesting sink properties.

Then hopefully we could tackle more hairy stuff like the probing. As Alex
said we seem to need quite some flexibility in that area (e.g. not all hw
supports per-lane training values), hence why I'd aim for lower-hanging
fruit first.

Note that there's already a bit of abstraction for i2c over dp aux, but
imo that's at the wrong level. At least reading through i915, gma500 and
radeon code there's a lot more we could share with just a dp aux helper
library (which then implements useful stuff on top of it).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6933 bytes --]

On Mon, Nov 18, 2013 at 05:20:54PM +0100, Daniel Vetter wrote:
> On Mon, Nov 18, 2013 at 04:26:17PM +0100, Thierry Reding wrote:
> > On Mon, Nov 18, 2013 at 10:09:56AM -0500, Alex Deucher wrote:
> > > On Mon, Nov 18, 2013 at 9:27 AM, Thierry Reding
> > > <thierry.reding@gmail.com> wrote:
> > > > On Fri, Nov 15, 2013 at 03:01:51PM +0200, Jani Nikula wrote:
> > > >> Debug print the capabilities, and flag an error if the panel does not
> > > >> support adjusting backlight through the BL_PWM_DIM pin, requiring
> > > >> backlight control through DPCD.
> > > >>
> > > >> I haven't seen such panels yet, but it's a matter of time. Give
> > > >> ourselves a reminder when we need to fix this for real.
> > > >>
> > > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > >> ---
> > > >>  drivers/gpu/drm/i915/intel_dp.c |   14 ++++++++++++++
> > > >>  1 file changed, 14 insertions(+)
> > > >
> > > > I have a few general comments below, but this patch itself look fine,
> > > > so:
> > > >
> > > > Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > >
> > > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > >> index cbf33be..ea4f3d1 100644
> > > >> --- a/drivers/gpu/drm/i915/intel_dp.c
> > > >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > >> @@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > > >>                       dev_priv->psr.sink_support = true;
> > > >>                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > > >>               }
> > > >> +
> > > >> +             if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &
> > > >> +                 DP_DPCD_DISPLAY_CONTROL_CAP) {
> > > >> +                     u8 ctrl[4] = { 0 };
> > > >> +
> > > >> +                     intel_dp_aux_native_read(intel_dp, DP_EDP_REV,
> > > >> +                                              ctrl, sizeof(ctrl));
> > > >> +                     DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x\n",
> > > >> +                                   ctrl[0], ctrl[1], ctrl[2], ctrl[3]);
> > > >> +
> > > >> +                     /* We don't support DPCD backlight control yet. */
> > > >> +                     if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1))
> > > >> +                             DRM_ERROR("eDP AUX backlight control only\n");
> > > >> +             }
> > > >>       }
> > > >>
> > > >>       if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > > >
> > > > I think a lot of eDP utility code could be made reusable across drivers.
> > > > We could probably do that by having each driver expose a drm_edp object
> > > > of some sort.
> > > >
> > > > Actually, the same would be true of DP in general. Accessing the DPCD is
> > > > something that's driver specific, but once you know how to do that a lot
> > > > of code can be made generic. I think a struct drm_dp could look like
> > > > this:
> > > >
> > > >         struct drm_dp;
> > > >
> > > >         struct drm_dpcd_ops {
> > > >                 ssize_t (*read)(struct drm_dp *dp, unsigned int offset,
> > > >                                 void *buffer, size_t size);
> > > >                 ssize_t (*write)(struct drm_dp *dp, unsigned int offset,
> > > >                                  const void *buffer, size_t size);
> > > >         };
> > > >
> > > >         struct drm_dp {
> > > >                 const struct drm_dpcd_ops *dpcd;
> > > >         };
> > > >
> > > > Perhaps that could even be extended with functionality to implement link
> > > > training in a generic way. There are already quite a few helpers to help
> > > > with that in drivers/gpu/drm/drm_dp_helper.c, but they assume that the
> > > > DPCD will be handed to them as a large buffer and therefore cannot write
> > > > DPCD registers.
> > > >
> > > > I suppose one could argue that it would be introducing a mid-layer, but
> > > > that layer would be really thin in my opinion. And it would allow a lot
> > > > of the algorithms to be written only once instead of multiple times.
> > > 
> > > I think it could probably be made to work.  The tricky part would be
> > > hw specific ordering in the training sequence.  At the very minimum,
> > > you need driver callbacks to set up the source side:
> > > 
> > > set_training_pattern()
> > > set_vs_emph()
> > > 
> > > And probably some flags to indicate whether the the hw supports
> > > specific features like training pattern 3.
> > 
> > Yes, something along those lines was what I had in mind as well. I know
> > that many people are unhappy about introducing this kind of mid-layer,
> > but quite frankly, doing this in generic code must have been one of the
> > primary reasons why VESA specified it that way.
> > 
> > The alternative will be to repeat more or less the same code in all the
> > drivers. I don't think that's a very nice alternative.
> 
> My plan (which is still somewhere on the todo but hasn't otherwise
> materilized) was to extract the dp aux handling code first. There's a lot
> of common code we could extract for i2c-over-dp-aux, handling branch
> devices and other stuff. Once we have that we can spill tons of little
> helper functions all over the place to decode interesting sink properties.

Quite a bit of that has already been done in the DP helpers. It should
be easy to extend that as we go along.

> Then hopefully we could tackle more hairy stuff like the probing. As Alex
> said we seem to need quite some flexibility in that area (e.g. not all hw
> supports per-lane training values), hence why I'd aim for lower-hanging
> fruit first.

That also means we'll have to duplicate the training in every new
driver. I was half expecting to be required to come up with the generic
code again, but if everyone is okay with this I won't bother with it for
now.

> Note that there's already a bit of abstraction for i2c over dp aux, but
> imo that's at the wrong level. At least reading through i915, gma500 and
> radeon code there's a lot more we could share with just a dp aux helper
> library (which then implements useful stuff on top of it).

I have some difficulty envisioning how the helper code can work without
some sort of driver-specific ops implementation. Currently the helpers
only use a snapshot of the DPCD to extract information. Eventually we'll
be bound to modify the DPCD, so some method of writing it back (or a
subset of it) will be needed. Otherwise the scope of the helper library
will be somewhat limited.

Once we have the callbacks, the current helpers could be reworked to not
use a buffer, but rather an "AUX channel object" and access the
registers directly. If there are concerns about performance, it could
possibly be implemented as a sort of cache, too. That would make it fast
to query the status. I don't think it'll be worth the added complexity,
though.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 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

Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Daniel Vetter]

On Mon, Nov 18, 2013 at 5:31 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> Note that there's already a bit of abstraction for i2c over dp aux, but
>> imo that's at the wrong level. At least reading through i915, gma500 and
>> radeon code there's a lot more we could share with just a dp aux helper
>> library (which then implements useful stuff on top of it).
>
> I have some difficulty envisioning how the helper code can work without
> some sort of driver-specific ops implementation. Currently the helpers
> only use a snapshot of the DPCD to extract information. Eventually we'll
> be bound to modify the DPCD, so some method of writing it back (or a
> subset of it) will be needed. Otherwise the scope of the helper library
> will be somewhat limited.
>
> Once we have the callbacks, the current helpers could be reworked to not
> use a buffer, but rather an "AUX channel object" and access the
> registers directly. If there are concerns about performance, it could
> possibly be implemented as a sort of cache, too. That would make it fast
> to query the status. I don't think it'll be worth the added complexity,
> though.

Oh, my idea is that the dp aux driver callback would at the level of
the intel_dp_aux_ch function in i915/intel_dp.c (gma500 and radeon
have something very similar). That alone would allow us to share a
considerable amount of code. Should have been a bit clearer, I've
discussed this in a bit more detail with Alex many moons ago ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1964 bytes --]

On Mon, Nov 18, 2013 at 05:38:18PM +0100, Daniel Vetter wrote:
> On Mon, Nov 18, 2013 at 5:31 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >> Note that there's already a bit of abstraction for i2c over dp aux, but
> >> imo that's at the wrong level. At least reading through i915, gma500 and
> >> radeon code there's a lot more we could share with just a dp aux helper
> >> library (which then implements useful stuff on top of it).
> >
> > I have some difficulty envisioning how the helper code can work without
> > some sort of driver-specific ops implementation. Currently the helpers
> > only use a snapshot of the DPCD to extract information. Eventually we'll
> > be bound to modify the DPCD, so some method of writing it back (or a
> > subset of it) will be needed. Otherwise the scope of the helper library
> > will be somewhat limited.
> >
> > Once we have the callbacks, the current helpers could be reworked to not
> > use a buffer, but rather an "AUX channel object" and access the
> > registers directly. If there are concerns about performance, it could
> > possibly be implemented as a sort of cache, too. That would make it fast
> > to query the status. I don't think it'll be worth the added complexity,
> > though.
> 
> Oh, my idea is that the dp aux driver callback would at the level of
> the intel_dp_aux_ch function in i915/intel_dp.c (gma500 and radeon
> have something very similar). That alone would allow us to share a
> considerable amount of code. Should have been a bit clearer, I've
> discussed this in a bit more detail with Alex many moons ago ...

Yeah, that's similar to what I had in mind. I think we may need
something slightly more complex, though. We want to support both AUX as
well as I2C over AUX transactions, so we'll probably need to add a mode
argument. I was thinking about adding a dp_aux_msg structure in order to
keep the argument list to a reasonable length.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 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