Hey! Saw your comment on the RHBZ regarding this, figured I'd take a look since I've got some DPCD backlight related fixes for one of your other laptops on the list as well. On Thu, 2019-10-10 at 00:13 +0800, Lee Shawn C wrote: > This panel (manufacturer is SDC, product ID is 0x4141) > used manufacturer defined DPCD register to control brightness > that not defined in eDP spec so far. This change follow panel > vendor's instruction to support brightness adjustment. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97883 > > V2: To check sink OUI instead of EDID quirk. > According to TCON's capability to decide to enable this > method for brightness control. > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > Cc: Adam Jackson <ajax@xxxxxxxxxx> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx> > Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx> > > Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx> > --- > drivers/gpu/drm/drm_dp_helper.c | 18 ++- > drivers/gpu/drm/i915/display/intel_dp.c | 7 + > .../drm/i915/display/intel_dp_aux_backlight.c | 138 +++++++++++++++++- > include/drm/drm_dp_helper.h | 20 +++ > 4 files changed, 176 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c > b/drivers/gpu/drm/drm_dp_helper.c > index f373798d82f6..02f79587c337 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -1268,6 +1268,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = { > { OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false, > BIT(DP_DPCD_QUIRK_NO_PSR) }, > /* CH7511 seems to leave SINK_COUNT zeroed */ > { OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'), > false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) }, > + /* Samsung eDP panel */ > + { OUI(0xba, 0x41, 0x59), DEVICE_ID_ANY, false, > BIT(DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL) }, We definitely need some more info on how this is supposed to work, since it's not in the eDP spec and it would be hard for anyone to fix or move around code on this upstream if upstream doesn't really know how it works. Additionally, we really should take care to label these macros and quirks correctly. CUSTOMIZE_BRIGHTNESS_CONTROL is pretty vague, and doesn't really make it clear that these are non-eDP compliant displays with proprietary backlight controls, not part of the VESA spec. If there's a name for the backlight protocol that Samsung uses on these panels we should name it after that, or just name it after samsung (something like DP_DPCD_QUIRK_INTEL_BRIGHTNESS_CONTROL). > }; > > #undef OUI > @@ -1281,7 +1283,7 @@ static const struct dpcd_quirk dpcd_quirk_list[] = { > * to device identification string and hardware/firmware revisions later. > */ > static u32 > -drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch) > +drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch, u8 > *tcon_cap) > { > const struct dpcd_quirk *quirk; > u32 quirks = 0; > @@ -1301,6 +1303,11 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident > *ident, bool is_branch) > memcmp(quirk->device_id, ident->device_id, sizeof(ident- > >device_id)) != 0) > continue; > > + if (quirk->quirks == > DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL && > + (!(tcon_cap[1] & DP_BRIGHTNESS_CONTROL_NITS) || > + !(tcon_cap[2] & DP_BRIGHTNESS_CONTROL_BY_AUX))) > + continue; > + > quirks |= quirk->quirks; > } > > @@ -1327,12 +1334,19 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct > drm_dp_desc *desc, > struct drm_dp_dpcd_ident *ident = &desc->ident; > unsigned int offset = is_branch ? DP_BRANCH_OUI : DP_SINK_OUI; > int ret, dev_id_len; > + u8 tcon_cap[4] = {0}; > > ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident)); > if (ret < 0) > return ret; > > - desc->quirks = drm_dp_get_quirks(ident, is_branch); > + if (offset == DP_SINK_OUI) { > + ret = drm_dp_dpcd_read(aux, DP_EDP_TCON_CAPABILITY_BYTE0, > tcon_cap, sizeof(tcon_cap)); > + if (ret < 0) > + return ret; > + } > + > + desc->quirks = drm_dp_get_quirks(ident, is_branch, tcon_cap); > > dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id)); > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 2b1e71f992b0..766b4b252147 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -7082,6 +7082,13 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > goto out_vdd_off; > } > > + if (drm_dp_has_quirk(&intel_dp->desc, > + DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) { > + i915_modparams.enable_dpcd_backlight = true; > + i915_modparams.fastboot = false; > + DRM_DEBUG_KMS("Using specific DPCD to control brightness\n"); > + } > + mhhhh, changing kernel module parameters like this in code is probably not a great idea. I agree with Jani as well that for something like this, we should I would take a look at how I implemented the quirk to work around the X1 Extreme 2nd Generation's broken vbios (which forgets to specify that the built-in display uses standard DPCD backlight controls): https://patchwork.freedesktop.org/patch/342166/?series=69914&rev=1 Also, we should probably just change the default for enable_dpcd_backlight to -1 anyway, and do so in a separate patch, since Intel sounds OK with doing that from the past discussions I've had with them. I've actually got a patch on the mailing list that's awaiting review which does this as well: https://patchwork.freedesktop.org/patch/342165/?series=69914&rev=1 If intel's OK with it, we probably could push this along with your patch if yours gets reviewed before my series > mutex_lock(&dev->mode_config.mutex); > edid = drm_get_edid(connector, &intel_dp->aux.ddc); > if (edid) { > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 020422da2ae2..09480e772bd4 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -25,6 +25,111 @@ > #include "intel_display_types.h" > #include "intel_dp_aux_backlight.h" > > +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL (512) > + > +static uint32_t intel_dp_aux_get_customize_backlight(struct intel_connector > *connector) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder- > >base); > + uint8_t read_val[2] = { 0x0 }; > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS, > + &read_val, sizeof(read_val)) < 0) { > + DRM_DEBUG_KMS("Failed to read DPCD register %x\n", > + DP_EDP_BRIGHTNESS_NITS); > + return 0; > + } > + > + return (read_val[1] << 8 | read_val[0]); > +} > + > +static void > +intel_dp_aux_set_customize_backlight(const struct drm_connector_state > *conn_state, u32 level) > +{ > + struct intel_connector *connector = to_intel_connector(conn_state- > >connector); > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder- > >base); > + struct intel_panel *panel = &connector->panel; > + uint8_t new_vals[4]; > + > + if (level > panel->backlight.max) > + level = panel->backlight.max; > + > + new_vals[0] = level & 0xFF; > + new_vals[1] = (level & 0xFF00) >> 8; > + new_vals[2] = 0; > + new_vals[3] = 0; > + > + if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS, > new_vals, 4) < 0) > + DRM_DEBUG_KMS("Failed to write aux backlight level\n"); > +} > + > +static void intel_dp_aux_enable_customize_backlight(const struct > intel_crtc_state *crtc_state, > + const struct drm_connector_state > *conn_state) > +{ > + struct intel_connector *connector = to_intel_connector(conn_state- > >connector); > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder- > >base); > + uint8_t read_val[4], i; > + uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00, > 0x00}; > + > + if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_PANEL_LUMINANCE_OVERRIDE, > write_val, sizeof(write_val)) < 0) > + DRM_DEBUG_KMS("Failed to write panel luminance.\n"); > + > + if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BRIGHTNESS_OPTIMIZATION, > 0x01) < 0) > + DRM_DEBUG_KMS("Failed to write %x\n", > + DP_EDP_BRIGHTNESS_OPTIMIZATION); > + /* write source OUI */ > + write_val[0] = 0x00; > + write_val[1] = 0xaa; > + write_val[2] = 0x01; This made me do one big double take for a moment, but I believe I understand what it does now that I've read through the rest of this thread. We should definitely leave a comment here though. > + if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, write_val, 3) < > 0) > + DRM_DEBUG_KMS("Failed to write OUI\n"); > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, > read_val) != 1) > + DRM_DEBUG_KMS("Failed to read %x\n", > + DP_EDP_GETSET_CTRL_PARAMS); > + > + if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, 0) > != 1) > + DRM_DEBUG_KMS("Failed to write %x\n", > + DP_EDP_GETSET_CTRL_PARAMS); > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, > read_val) != 1) > + DRM_DEBUG_KMS("Failed to read %x\n", > + DP_EDP_GETSET_CTRL_PARAMS); > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE, > &read_val, sizeof(read_val)) < 0) > + DRM_DEBUG_KMS("Failed to read %x\n", > + DP_EDP_CONTENT_LUMINANCE); > + > + memset(read_val, 0x0, 4); > + if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE, > read_val, sizeof(read_val)) < 0) > + DRM_DEBUG_KMS("Failed to write %x\n", > + DP_EDP_CONTENT_LUMINANCE); > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, > read_val) != 1) > + DRM_DEBUG_KMS("Failed to read %x\n", > + DP_EDP_GETSET_CTRL_PARAMS); > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, read_val, 4) < 0) > + DRM_DEBUG_KMS("Failed to read OUI\n"); > + > + DRM_DEBUG_KMS("got OUI %3ph\n", read_val); > + > + for (i = 0; i < 6; i++) { > + intel_dp_aux_set_customize_backlight(conn_state, connector- > >panel.backlight.level); > + > + msleep(60); > + if (intel_dp_aux_get_customize_backlight(connector)) > + return; > + } > + > + DRM_DEBUG_KMS("Restore brightness may have problem.\n"); Why not just "Failed to restore brightness\n" > +} > + > +static void intel_dp_aux_disable_customize_backlight(const struct > drm_connector_state *old_conn_state) > +{ > + // do nothing > + return; > +} > + > static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool > enable) > { > u8 reg_val = 0; > @@ -225,6 +330,19 @@ static void intel_dp_aux_disable_backlight(const struct > drm_connector_state *old > set_aux_backlight_enable(enc_to_intel_dp(old_conn_state- > >best_encoder), false); > } > > +static int intel_dp_aux_setup_customize_backlight(struct intel_connector > *connector, > + enum pipe pipe) > +{ > + struct intel_panel *panel = &connector->panel; > + > + panel->backlight.max = EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL; > + panel->backlight.min = 0; > + panel->backlight.level = panel->backlight.get(connector); I can't really test this locally since I don't have this laptop, did you make sure this part actually returns the currently set brightness? I know on the X1 Carbon Extreme 2nd Gen (which uses standard eDP compliant DPCD backlight controls) the initial backlight value is reported as 0 when it's actually max, and we have to check the control register to guess what the actual brightness is: https://patchwork.freedesktop.org/patch/342160/?series=69914&rev=1 > + panel->backlight.enabled = panel->backlight.level != 0; > + > + return 0; > +} > + > static int intel_dp_aux_setup_backlight(struct intel_connector *connector, > enum pipe pipe) > { > @@ -265,6 +383,7 @@ int intel_dp_aux_init_backlight_funcs(struct > intel_connector *intel_connector) > { > struct intel_panel *panel = &intel_connector->panel; > struct drm_i915_private *dev_priv = to_i915(intel_connector- > >base.dev); > + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_connector->encoder- > >base); > > if (i915_modparams.enable_dpcd_backlight == 0 || > (i915_modparams.enable_dpcd_backlight == -1 && > @@ -274,11 +393,20 @@ int intel_dp_aux_init_backlight_funcs(struct > intel_connector *intel_connector) > if (!intel_dp_aux_display_control_capable(intel_connector)) > return -ENODEV; > > - panel->backlight.setup = intel_dp_aux_setup_backlight; > - panel->backlight.enable = intel_dp_aux_enable_backlight; > - panel->backlight.disable = intel_dp_aux_disable_backlight; > - panel->backlight.set = intel_dp_aux_set_backlight; > - panel->backlight.get = intel_dp_aux_get_backlight; > + if (drm_dp_has_quirk(&intel_dp->desc, > + DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) { > + panel->backlight.setup = > intel_dp_aux_setup_customize_backlight; > + panel->backlight.enable = > intel_dp_aux_enable_customize_backlight; > + panel->backlight.disable = > intel_dp_aux_disable_customize_backlight; > + panel->backlight.set = intel_dp_aux_set_customize_backlight; > + panel->backlight.get = intel_dp_aux_get_customize_backlight; > + } else { > + panel->backlight.setup = intel_dp_aux_setup_backlight; > + panel->backlight.enable = intel_dp_aux_enable_backlight; > + panel->backlight.disable = intel_dp_aux_disable_backlight; > + panel->backlight.set = intel_dp_aux_set_backlight; > + panel->backlight.get = intel_dp_aux_get_backlight; > + } > > return 0; > } > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index ed1a985745ba..581d1ba8435d 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -740,6 +740,20 @@ > /* up to ID_SLOT_63 at 0x2ff */ > > #define DP_SOURCE_OUI 0x300 > +#define DP_EDP_TCON_CAPABILITY_BYTE0 0x340 > + > +#define DP_EDP_TCON_CAPABILITY_BYTE1 0x341 > +# define DP_BRIGHTNESS_CONTROL_NITS 0x10 > + > +#define DP_EDP_TCON_CAPABILITY_BYTE2 0x342 > +# define DP_BRIGHTNESS_CONTROL_BY_AUX 0x01 > + > +#define DP_EDP_GETSET_CTRL_PARAMS 0x344 > +#define DP_EDP_CONTENT_LUMINANCE 0x346 > +#define DP_EDP_PANEL_LUMINANCE_OVERRIDE 0x34A > +#define DP_EDP_BRIGHTNESS_NITS 0x354 > +#define DP_EDP_BRIGHTNESS_OPTIMIZATION 0x358 > + These definitely need to be renamed as well. A prefix like DP_EDP_INTEL_BL or something named after the name of this protocol would work. I'd also leave a comment explaining what these are for > #define DP_SINK_OUI 0x400 > #define DP_BRANCH_OUI 0x500 > #define DP_BRANCH_ID 0x503 > @@ -1475,6 +1489,12 @@ enum drm_dp_quirk { > * The driver should ignore SINK_COUNT during detection. > */ > DP_DPCD_QUIRK_NO_SINK_COUNT, > + /** > + * @DP_DPCD_QUIRK_NON_STANDARD_BRIGHTNESS_CONTROL: > + * > + * The device used specific DPCD register to control brightness. > + */ > + DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL, This should be renamed as well, and should have a comment explaining that this is a non-standard form of brightness control over DPCD > }; > > /** -- Cheers, Lyude Paul _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
