Re: [Nouveau] [Intel-gfx] [PATCH v2 0/4] drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers
On Sat, 2021-10-02 at 11:14 +0200, Hans de Goede wrote: > Hi Lyude, > > On 10/2/21 12:53 AM, Lyude Paul wrote: > > When I originally moved all of the VESA backlight code in i915 into DRM > > helpers, one of the things I didn't have the hardware or time for > > testing was machines that used a combination of PWM and DPCD in order to > > control their backlights. This has since then caused some breakages and > > resulted in us disabling DPCD backlight support on such machines. This > > works fine, unless you have a machine that actually needs this > > functionality for backlight controls to work at all. Additionally, we > > will need to support PWM for when we start adding support for VESA's > > product (as in the product of multiplication) control mode for better > > brightness ranges. > > > > So - let's finally finish up implementing basic support for these types > > of backlights to solve these problems in our DP helpers, along with > > implementing support for this in i915. And since digging into this issue > > solved the last questions we really had about probing backlights in i915 > > for the most part, let's update some of the comments around that as > > well! > > Backlight control is a topic which I'm reasonably familiar with, > do you want me to review this series for you ? Possibly, although I definitely want to make sure that someone from Intel gets a chance to review this as well. I'm more curious if you might happen to have any systems that would be able to test this. > > Regards, > > Hans > > > > > > > Changes: > > * Fixup docs > > * Add patch to stop us from breaking nouveau > > > > Lyude Paul (4): > > drm/i915: Add support for panels with VESA backlights with PWM > > enable/disable > > drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux > > enable/brightness > > drm/dp, drm/i915: Add support for VESA backlights using PWM for > > brightness control > > drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs() > > > > drivers/gpu/drm/drm_dp_helper.c | 75 +++-- > > .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++- > > drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +- > > include/drm/drm_dp_helper.h | 7 +- > > 4 files changed, 122 insertions(+), 45 deletions(-) > > > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [Nouveau] [PATCH -next] drm/nouveau/gem: remove redundant semi-colon
Reviewed-by: Karol Herbst sorry for the late response though. On Fri, Apr 2, 2021 at 12:28 AM Yang Yingliang wrote: > > Signed-off-by: Yang Yingliang > --- > drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c > b/drivers/gpu/drm/nouveau/nouveau_gem.c > index c88cbb85f101..492e6794c5e6 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > @@ -303,7 +303,7 @@ nouveau_gem_set_domain(struct drm_gem_object *gem, > uint32_t read_domains, > struct ttm_buffer_object *bo = >bo; > uint32_t domains = valid_domains & nvbo->valid_domains & > (write_domains ? write_domains : read_domains); > - uint32_t pref_domains = 0;; > + uint32_t pref_domains = 0; > > if (!domains) > return -EINVAL; > -- > 2.25.1 > > ___ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau >
[Nouveau] [PATCH v3 3/5] drm/dp: Disable unsupported features in DP_EDP_BACKLIGHT_MODE_SET_REGISTER
As it turns out, apparently some machines will actually leave additional
backlight functionality like dynamic backlight control on before the OS
loads. Currently we don't take care to disable unsupported features when
writing back the backlight mode, which can lead to some rather strange
looking behavior when adjusting the backlight.
So, let's fix this by ensuring we only keep supported features enabled for
panel backlights - which should fix some of the issues we were seeing from
this on fi-bdw-samus.
Signed-off-by: Lyude Paul
Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM
helpers")
---
drivers/gpu/drm/drm_dp_helper.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4d0d1e8e51fa..d9a7f07f42fd 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3255,7 +3255,9 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
return ret < 0 ? ret : -EIO;
}
- new_dpcd_buf = dpcd_buf;
+ /* Disable any backlight functionality we don't support that might be
on */
+ new_dpcd_buf = dpcd_buf & (DP_EDP_BACKLIGHT_CONTROL_MODE_MASK |
+ DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE);
if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
@@ -3277,6 +3279,8 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
aux->name, ret);
else
new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+ } else {
+ new_dpcd_buf &= ~DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
}
if (new_dpcd_buf != dpcd_buf) {
--
2.31.1
[Nouveau] [PATCH v3 4/5] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control
Now that we've added support to i915 for controlling panel backlights that
need PWM to be enabled/disabled, let's finalize this and add support for
controlling brightness levels via PWM as well. This should hopefully put us
towards the path of supporting _ALL_ backlights via VESA's DPCD interface
which would allow us to finally start trusting the DPCD again.
Note however that we still don't enable using this by default on i915 when
it's not needed, primarily because I haven't yet had a chance to confirm if
it's safe to do this on the one machine in Intel's CI that had an issue
with this: samus-fi-bdw. I have done basic testing of this on other
machines though, by manually patching i915 to force it into PWM-only mode
on some of my laptops.
v2:
* Correct documentation (thanks Doug!)
* Get rid of backlight caps
Signed-off-by: Lyude Paul
Cc: Rajeev Nandan
Cc: Doug Anderson
Cc: Satadru Pramanik
---
drivers/gpu/drm/drm_dp_helper.c | 76 +--
.../drm/i915/display/intel_dp_aux_backlight.c | 48 +---
include/drm/drm_dp_helper.h | 7 +-
3 files changed, 94 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index d9a7f07f42fd..9bef21613370 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3173,6 +3173,10 @@ int drm_edp_backlight_set_level(struct drm_dp_aux *aux,
const struct drm_edp_bac
int ret;
u8 buf[2] = { 0 };
+ /* The panel uses the PWM for controlling brightness levels */
+ if (!bl->aux_set)
+ return 0;
+
if (bl->lsb_reg_used) {
buf[0] = (level & 0xff00) >> 8;
buf[1] = (level & 0x00ff);
@@ -3199,7 +3203,7 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
int ret;
u8 buf;
- /* The panel uses something other then DPCD for enabling its backlight
*/
+ /* This panel uses the EDP_BL_PWR GPIO for enablement */
if (!bl->aux_enable)
return 0;
@@ -3234,11 +3238,11 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
* restoring any important backlight state such as the given backlight level,
the brightness byte
* count, backlight frequency, etc.
*
- * Note that certain panels, while supporting brightness level controls over
DPCD, may not support
- * having their backlights enabled via the standard
%DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
- * _edp_backlight_info.aux_enable will be set to %false, this function
will skip the step of
- * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must
perform the required
- * implementation specific step for enabling the backlight after calling this
function.
+ * Note that certain panels do not support being enabled or disabled via DPCD,
but instead require
+ * that the driver handle enabling/disabling the panel through
implementation-specific means using
+ * the EDP_BL_PWR GPIO. For such panels, _edp_backlight_info.aux_enable
will be set to %false,
+ * this function becomes a no-op, and the driver is expected to handle
powering the panel on using
+ * the EDP_BL_PWR GPIO.
*
* Returns: %0 on success, negative error code on failure.
*/
@@ -3246,7 +3250,7 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
const u16 level)
{
int ret;
- u8 dpcd_buf, new_dpcd_buf;
+ u8 dpcd_buf, new_dpcd_buf, new_mode;
ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
_buf);
if (ret != 1) {
@@ -3259,9 +3263,14 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
new_dpcd_buf = dpcd_buf & (DP_EDP_BACKLIGHT_CONTROL_MODE_MASK |
DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE);
- if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
+ if (bl->aux_set)
+ new_mode = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+ else
+ new_mode = DP_EDP_BACKLIGHT_CONTROL_MODE_PWM;
+
+ if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != new_mode) {
new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
- new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+ new_dpcd_buf |= new_mode;
if (bl->pwmgen_bit_count) {
ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT,
bl->pwmgen_bit_count);
@@ -3308,12 +3317,13 @@ EXPORT_SYMBOL(drm_edp_backlight_enable);
* @aux: The DP AUX channel to use
* @bl: Backlight capability info from drm_edp_backlight_init()
*
- * This function handles disabling DPCD backlight controls on a panel over
AUX. Note that some
- * panels have backlights that are enabled/disabled by other means, despite
having their brightness
- * values controlled
[Nouveau] [PATCH v3 5/5] drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()
Hooray! We've managed to hit enough bugs upstream that I've been able to
come up with a pretty solid explanation for how backlight controls are
actually supposed to be detected and used these days. As well, having the
rest of the PWM bits in VESA's backlight interface implemented seems to
have fixed all of the problematic brightness controls laptop panels that
we've hit so far.
So, let's actually document this instead of just calling the laptop panels
liars. As well, I would like to formally apologize to all of the laptop
panels I called liars. I'm sorry laptop panels, hopefully you can all
forgive me and we can move past this~
Signed-off-by: Lyude Paul
---
.../drm/i915/display/intel_dp_aux_backlight.c| 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
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 91daf9ab50e8..04a52d6a74ed 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -455,11 +455,17 @@ int intel_dp_aux_init_backlight_funcs(struct
intel_connector *connector)
}
/*
-* A lot of eDP panels in the wild will report supporting both the
-* Intel proprietary backlight control interface, and the VESA
-* backlight control interface. Many of these panels are liars though,
-* and will only work with the Intel interface. So, always probe for
-* that first.
+* Since Intel has their own backlight control interface, the majority
of machines out there
+* using DPCD backlight controls with Intel GPUs will be using this
interface as opposed to
+* the VESA interface. However, other GPUs (such as Nvidia's) will
always use the VESA
+* interface. This means that there's quite a number of panels out
there that will advertise
+* support for both interfaces, primarily systems with Intel/Nvidia
hybrid GPU setups.
+*
+* There's a catch to this though: on many panels that advertise
support for both
+* interfaces, the VESA backlight interface will stop working once
we've programmed the
+* panel with Intel's OUI - which is also required for us to be able to
detect Intel's
+* backlight interface at all. This means that the only sensible way
for us to detect both
+* interfaces is to probe for Intel's first, and VESA's second.
*/
if (try_intel_interface &&
intel_dp_aux_supports_hdr_backlight(connector)) {
drm_dbg_kms(dev, "Using Intel proprietary eDP backlight
controls\n");
--
2.31.1
[Nouveau] [PATCH v3 2/5] drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux enable/brightness
Since we don't support hybrid AUX/PWM backlights in nouveau right now,
let's add some explicit checks so that we don't break nouveau once we
enable support for these backlights in other drivers.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 1cbd71abc80a..ae2f2abc8f5a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -308,7 +308,10 @@ nv50_backlight_init(struct nouveau_backlight *bl,
if (ret < 0)
return ret;
- if (drm_edp_backlight_supported(edp_dpcd)) {
+ /* TODO: Add support for hybrid PWM/DPCD panels */
+ if (drm_edp_backlight_supported(edp_dpcd) &&
+ (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+ (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
NV_DEBUG(drm, "DPCD backlight controls supported on
%s\n",
nv_conn->base.name);
--
2.31.1
[Nouveau] [PATCH v3 1/5] drm/i915: Add support for panels with VESA backlights with PWM enable/disable
This simply adds proper support for panel backlights that can be controlled
via VESA's backlight control protocol, but which also require that we
enable and disable the backlight via PWM instead of via the DPCD interface.
We also enable this by default, in order to fix some people's backlights
that were broken by not having this enabled.
For reference, backlights that require this and use VESA's backlight
interface tend to be laptops with hybrid GPUs, but this very well may
change in the future.
Signed-off-by: Lyude Paul
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/3680
Fixes: fe7d52bccab6 ("drm/i915/dp: Don't use DPCD backlights that need PWM
enable/disable")
Cc: # v5.12+
---
.../drm/i915/display/intel_dp_aux_backlight.c | 24 ++-
1 file changed, 18 insertions(+), 6 deletions(-)
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 569d17b4d00f..594fdc7453ca 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -293,6 +293,10 @@ intel_dp_aux_vesa_enable_backlight(const struct
intel_crtc_state *crtc_state,
struct intel_panel *panel = >panel;
struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
+ if (!panel->backlight.edp.vesa.info.aux_enable)
+ panel->backlight.pwm_funcs->enable(crtc_state, conn_state,
+
panel->backlight.pwm_level_max);
+
drm_edp_backlight_enable(_dp->aux,
>backlight.edp.vesa.info, level);
}
@@ -304,6 +308,10 @@ static void intel_dp_aux_vesa_disable_backlight(const
struct drm_connector_state
struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
drm_edp_backlight_disable(_dp->aux,
>backlight.edp.vesa.info);
+
+ if (!panel->backlight.edp.vesa.info.aux_enable)
+ panel->backlight.pwm_funcs->disable(old_conn_state,
+
intel_backlight_invert_pwm_level(connector, 0));
}
static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
*connector, enum pipe pipe)
@@ -321,6 +329,15 @@ static int intel_dp_aux_vesa_setup_backlight(struct
intel_connector *connector,
if (ret < 0)
return ret;
+ if (!panel->backlight.edp.vesa.info.aux_enable) {
+ ret = panel->backlight.pwm_funcs->setup(connector, pipe);
+ if (ret < 0) {
+ drm_err(>drm,
+ "Failed to setup PWM backlight controls for eDP
backlight: %d\n",
+ ret);
+ return ret;
+ }
+ }
panel->backlight.max = panel->backlight.edp.vesa.info.max;
panel->backlight.min = 0;
if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
@@ -340,12 +357,7 @@ intel_dp_aux_supports_vesa_backlight(struct
intel_connector *connector)
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
- /* TODO: We currently only support AUX only backlight configurations,
not backlights which
-* require a mix of PWM and AUX controls to work. In the mean time,
these machines typically
-* work just fine using normal PWM controls anyway.
-*/
- if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
- drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
+ if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
drm_dbg_kms(>drm, "AUX Backlight Control Supported!\n");
return true;
}
--
2.31.1
[Nouveau] [PATCH v3 0/5] drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers
When I originally moved all of the VESA backlight code in i915 into DRM helpers, one of the things I didn't have the hardware or time for testing was machines that used a combination of PWM and DPCD in order to control their backlights. This has since then caused some breakages and resulted in us disabling DPCD backlight support on such machines. This works fine, unless you have a machine that actually needs this functionality for backlight controls to work at all. Additionally, we will need to support PWM for when we start adding support for VESA's product (as in the product of multiplication) control mode for better brightness ranges. So - let's finally finish up implementing basic support for these types of backlights to solve these problems in our DP helpers, along with implementing support for this in i915. And since digging into this issue solved the last questions we really had about probing backlights in i915 for the most part, let's update some of the comments around that as well! Changes (v3): * Add likely fix for weird backlight scaling issues on samus-fi-bdw in intel's CI, which pointed out we've been leaving some (currently) unsupported backlight features on by mistake which certainly have the potential to cause problems. Changes (v2): * Fixup docs * Add patch to stop us from breaking nouveau Lyude Paul (5): drm/i915: Add support for panels with VESA backlights with PWM enable/disable drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux enable/brightness drm/dp: Disable unsupported features in DP_EDP_BACKLIGHT_MODE_SET_REGISTER drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs() drivers/gpu/drm/drm_dp_helper.c | 82 +-- .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++ drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +- include/drm/drm_dp_helper.h | 7 +- 4 files changed, 128 insertions(+), 46 deletions(-) -- 2.31.1
Re: [Nouveau] [PATCH] drm/nouveau/svm: Fix refcount leak bug and missing check against null bug
I think it makes sense to add a Fixes tag to this:
Fixes: 822cab6150d3 ("drm/nouveau/svm: check for SVM initialized
before migrating")
Reviewed-by: Karol Herbst
On Tue, Sep 7, 2021 at 3:20 PM Chenyuan Mi wrote:
>
> The reference counting issue happens in one exception handling path of
> nouveau_svmm_bind(). When cli->svm.svmm is null, the function forgets
> to decrease the refcount of mm increased by get_task_mm(), causing a
> refcount leak.
>
> Fix this issue by using mmput() to decrease the refcount in the
> exception handling path.
>
> Also, the function forgets to do check against null when get mm
> by get_task_mm().
>
> Fix this issue by adding null check after get mm by get_task_mm().
>
> Signed-off-by: Chenyuan Mi
> Signed-off-by: Xiyu Yang
> Signed-off-by: Xin Tan
> ---
> drivers/gpu/drm/nouveau/nouveau_svm.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
> b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index b0c3422cb01f..9985bfde015a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -162,10 +162,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
> */
>
> mm = get_task_mm(current);
> + if (!mm) {
> + return -EINVAL;
> + }
> mmap_read_lock(mm);
>
> if (!cli->svm.svmm) {
> mmap_read_unlock(mm);
> + mmput(mm);
> return -EINVAL;
> }
>
> --
> 2.17.1
>
