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

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

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