| Message ID | 20180116134039.24178-1-gnidorah@ya.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
What branch CI tests against? I've created the patch against current torvalds/linux 16.01.2018, 17:42, "Patchwork" <patchwork@emeril.freedesktop.org>: > == Series Details == > > Series: drm/i915: Allow user to override PWM backlight frequency and duty cycle > URL : https://patchwork.freedesktop.org/series/36540/ > State : failure > > == Summary == > > Applying: drm/i915: Allow user to override PWM backlight frequency and duty cycle > error: Failed to merge in the changes. > Using index info to reconstruct a base tree... > M drivers/gpu/drm/i915/i915_params.c > M drivers/gpu/drm/i915/i915_params.h > M drivers/gpu/drm/i915/intel_panel.c > Falling back to patching base and 3-way merge... > Auto-merging drivers/gpu/drm/i915/intel_panel.c > Auto-merging drivers/gpu/drm/i915/i915_params.h > Auto-merging drivers/gpu/drm/i915/i915_params.c > CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_params.c > Patch failed at 0001 drm/i915: Allow user to override PWM backlight frequency and duty cycle > The copy of the patch that failed is found in: .git/rebase-apply/patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort".
HI, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > gnidorah@ya.ru > Sent: tiistai 16. tammikuuta 2018 17.36 > To: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Allow user to override > PWM backlight frequency and duty cycle > > What branch CI tests against? https://cgit.freedesktop.org/drm-tip > I've created the patch against current torvalds/linux > > 16.01.2018, 17:42, "Patchwork" <patchwork@emeril.freedesktop.org>: > > == Series Details == > > > > Series: drm/i915: Allow user to override PWM backlight frequency and > > duty cycle URL : https://patchwork.freedesktop.org/series/36540/ > > State : failure > > > > == Summary == > > > > Applying: drm/i915: Allow user to override PWM backlight frequency and > > duty cycle > > error: Failed to merge in the changes. > > Using index info to reconstruct a base tree... > > M drivers/gpu/drm/i915/i915_params.c > > M drivers/gpu/drm/i915/i915_params.h > > M drivers/gpu/drm/i915/intel_panel.c > > Falling back to patching base and 3-way merge... > > Auto-merging drivers/gpu/drm/i915/intel_panel.c > > Auto-merging drivers/gpu/drm/i915/i915_params.h > > Auto-merging drivers/gpu/drm/i915/i915_params.c > > CONFLICT (content): Merge conflict in > > drivers/gpu/drm/i915/i915_params.c > > Patch failed at 0001 drm/i915: Allow user to override PWM backlight > > frequency and duty cycle The copy of the patch that failed is found > > in: .git/rebase-apply/patch When you have resolved this problem, run "git am > --continue". > > If you prefer to skip this patch, run "git am --skip" instead. > > To restore the original branch and stop patching, run "git am --abort". Jani Saarinen Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
On Tue, 16 Jan 2018, Alex Ivanov <gnidorah@ya.ru> wrote: > Since vendor may set bad defaults or user just generally may prefer her health over > display life. > 1. Allow overriding of PWM frequency otherwise people use solutions like > http://devbraindom.blogspot.ru/2013/03/eliminate-led-screen-flicker-with-intel.html > which are bad because > I. frequency convertation logic varies from chip to chip, so people set totally wrong > frequences in most cases > II. writing to backlight control registers directly goes into conflict with > i915 driver doing the same > 2. Allow to override minimal brightness. Even if vendor limit was totally correct, > it may be too bright to use in night, plus the default will become most likely wrong > now, since frequency was overriden I'm divided. Clearly, the patch at hand is a technically better solution than what the blog post has. But I also think the folks over at the blog know they're directly hacking on graphics registers, and whatever it is they're doing is not supported. There's also warranted speculation this might be harmful to their displays. The modulation frequency and the minimum duty cycle have been chosen by the OEM to work with their board design and display specs. We don't have enough data to validate the values given by the user are within spec. If we add this, it'll get used, it'll be expected to be supported, the "unsafe" there will go unnoticed by folks copy-pasting the parameters from blogs and forums, and if it breaks displays for folks, they'll be angry at us, not at some random blog poster. I guess damned if you do, damned if you don't. BR, Jani. > > --- > drivers/gpu/drm/i915/i915_params.c | 6 ++++++ > drivers/gpu/drm/i915/i915_params.h | 2 ++ > drivers/gpu/drm/i915/intel_panel.c | 17 +++++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index b4faeb6aa2bd..ba37257846b0 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -190,3 +190,9 @@ i915_param_named(enable_dpcd_backlight, bool, 0600, > > i915_param_named(enable_gvt, bool, 0400, > "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); > + > +i915_param_named_unsafe(backlight_freq, uint, 0400, > + "Override PWM backlight frequency (set in Hz, 0=ignore [default])"); > + > +i915_param_named_unsafe(backlight_min_level, uint, 0400, > + "Override PWM backlight minimum level (duty cycle, 0=ignore [default])"); > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index c7292268ed43..7aba85db3da2 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -53,6 +53,8 @@ > param(int, edp_vswing, 0) \ > param(int, reset, 2) \ > param(unsigned int, inject_load_failure, 0) \ > + param(unsigned int, backlight_freq, 0) \ > + param(unsigned int, backlight_min_level, 0) \ > /* leave bools at the end to not create holes */ \ > param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \ > param(bool, enable_cmd_parser, true) \ > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index adc51e452e3e..f188d7c1d2de 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1479,6 +1479,12 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector) > > WARN_ON(panel->backlight.max == 0); > > + if (i915_modparams.backlight_min_level) { > + DRM_DEBUG_KMS("Override backlight duty cycle %u\n", > + i915_modparams.backlight_min_level); > + return i915_modparams.backlight_min_level; > + } > + > /* > * XXX: If the vbt value is 255, it makes min equal to max, which leads > * to problems. There are such machines out there. Either our > @@ -1795,6 +1801,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) > struct intel_connector *intel_connector = to_intel_connector(connector); > struct intel_panel *panel = &intel_connector->panel; > int ret; > + u32 pwm; > > if (!dev_priv->vbt.backlight.present) { > if (dev_priv->quirks & QUIRK_BACKLIGHT_PRESENT) { > @@ -1812,6 +1819,16 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) > /* set level and max in panel struct */ > mutex_lock(&dev_priv->backlight_lock); > ret = panel->backlight.setup(intel_connector, pipe); > + if (i915_modparams.backlight_freq) { > + if (panel->backlight.hz_to_pwm && > + (pwm = panel->backlight.hz_to_pwm(intel_connector, i915_modparams.backlight_freq))) { > + panel->backlight.max = pwm; > + DRM_DEBUG_KMS("Override backlight frequency %u Hz\n", > + i915_modparams.backlight_freq); > + } else { > + DRM_DEBUG_KMS("failed to override backlight frequency\n"); > + } > + } > mutex_unlock(&dev_priv->backlight_lock); > > if (ret) {
17.01.2018, 16:13, "Jani Nikula" <jani.nikula@linux.intel.com>: > I'm divided. Clearly, the patch at hand is a technically better solution > than what the blog post has. > > But I also think the folks over at the blog know they're directly > hacking on graphics registers, and whatever it is they're doing is not > supported. There's also warranted speculation this might be harmful to > their displays. But those using such calculators and guides don't know what precisely they are doing, because authors only provided setup logic for *their own* hardware, and not for other chips. This leads to more harm than it should would be there an official way for doing overrides. > The modulation frequency and the minimum duty cycle have been chosen by > the OEM to work with their board design and display specs. Unfortunately some vendors tend to use "whatever defaults" that are not good for humans. That's the reason why people poke with PWM registers. > We don't have enough data to validate the values given by the user are within spec. If > we add this, it'll get used, it'll be expected to be supported, the > "unsafe" there will go unnoticed by folks copy-pasting the parameters > from blogs and forums I can rename the knobs into e.g. "debug_backlight_freq" and "debug_backlight_min_level" and add warnings to descriptions so there will be FAT warnings. > and if it breaks displays for folks, they'll be > angry at us, not at some random blog poster. The license on i915 driver says that its provided "as is" without any warranty. With added warnings on kernel parameters people MUST know what they're doing and take all responsibility. > > I guess damned if you do, damned if you don't. > > BR, > Jani. >
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b4faeb6aa2bd..ba37257846b0 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -190,3 +190,9 @@ i915_param_named(enable_dpcd_backlight, bool, 0600, i915_param_named(enable_gvt, bool, 0400, "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); + +i915_param_named_unsafe(backlight_freq, uint, 0400, + "Override PWM backlight frequency (set in Hz, 0=ignore [default])"); + +i915_param_named_unsafe(backlight_min_level, uint, 0400, + "Override PWM backlight minimum level (duty cycle, 0=ignore [default])"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index c7292268ed43..7aba85db3da2 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -53,6 +53,8 @@ param(int, edp_vswing, 0) \ param(int, reset, 2) \ param(unsigned int, inject_load_failure, 0) \ + param(unsigned int, backlight_freq, 0) \ + param(unsigned int, backlight_min_level, 0) \ /* leave bools at the end to not create holes */ \ param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \ param(bool, enable_cmd_parser, true) \ diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index adc51e452e3e..f188d7c1d2de 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1479,6 +1479,12 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector) WARN_ON(panel->backlight.max == 0); + if (i915_modparams.backlight_min_level) { + DRM_DEBUG_KMS("Override backlight duty cycle %u\n", + i915_modparams.backlight_min_level); + return i915_modparams.backlight_min_level; + } + /* * XXX: If the vbt value is 255, it makes min equal to max, which leads * to problems. There are such machines out there. Either our @@ -1795,6 +1801,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) struct intel_connector *intel_connector = to_intel_connector(connector); struct intel_panel *panel = &intel_connector->panel; int ret; + u32 pwm; if (!dev_priv->vbt.backlight.present) { if (dev_priv->quirks & QUIRK_BACKLIGHT_PRESENT) { @@ -1812,6 +1819,16 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) /* set level and max in panel struct */ mutex_lock(&dev_priv->backlight_lock); ret = panel->backlight.setup(intel_connector, pipe); + if (i915_modparams.backlight_freq) { + if (panel->backlight.hz_to_pwm && + (pwm = panel->backlight.hz_to_pwm(intel_connector, i915_modparams.backlight_freq))) { + panel->backlight.max = pwm; + DRM_DEBUG_KMS("Override backlight frequency %u Hz\n", + i915_modparams.backlight_freq); + } else { + DRM_DEBUG_KMS("failed to override backlight frequency\n"); + } + } mutex_unlock(&dev_priv->backlight_lock); if (ret) {