From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); - struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder); - struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux; - int ret; u8 pwr;
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT + struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); + struct nouveau_backlight *backlight = nv_connector->backlight; + if (backlight && backlight->uses_dpcd) { - ret = drm_edp_backlight_disable(aux, &backlight->edp_info); + int ret = drm_edp_backlight_disable(aux, &backlight->edp_info); + if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); } +#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;int ret; u8 pwr;+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);struct nouveau_backlight *backlight = nv_connector->backlight;if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);-- 2.29.2
On Fri, Jul 23, 2021 at 11:24 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
same, I'd just require BACKLIGHT as well tbh.
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;int ret; u8 pwr;+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);struct nouveau_backlight *backlight = nv_connector->backlight;if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);-- 2.29.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, Jul 23, 2021 at 12:10 PM Karol Herbst kherbst@redhat.com wrote:
On Fri, Jul 23, 2021 at 11:24 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
same, I'd just require BACKLIGHT as well tbh.
ehhh, get rid of DRM_NOUVEAU_BACKLIGHT I meant.
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;int ret; u8 pwr;+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);struct nouveau_backlight *backlight = nv_connector->backlight;if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);-- 2.29.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, Jul 23, 2021 at 11:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
Yes! I'd definitely be in favor of that, I've wasted way too much time trying to sort through dependency loops and other problems with backlight support.
Maybe we should leave the drivers/video/fbdev/ drivers untouched in this regard, at least for the moment, but for the drivers/gpu/drm users of backlight that would be a nice simplification, and even the smallest ones are unlikely to be used on systems that are too memory constrained to deal with 4KB extra .text.
Arnd
On Fri, Jul 23, 2021 at 12:16:31PM +0200, Arnd Bergmann wrote:
On Fri, Jul 23, 2021 at 11:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
Yes! I'd definitely be in favor of that, I've wasted way too much time trying to sort through dependency loops and other problems with backlight support.
Maybe we should leave the drivers/video/fbdev/ drivers untouched in this regard, at least for the moment, but for the drivers/gpu/drm users of backlight that would be a nice simplification, and even the smallest ones are unlikely to be used on systems that are too memory constrained to deal with 4KB extra .text.
Yeah I think we can do this entirely ad-hoc, i.e. any time the backlight wheel wobbles off again we nail it down for good for that driver with a depends on BACKGLIGHT and remove any lingering #ifdef all over.
If you want maybe start out with the biggest drm drivers in a series, I think if nouveau/amdgpu/i915 folks ack this you're good to go to just convert as things get in the way. -Daniel
[AMD Official Use Only]
+Harry
-----Original Message----- From: Daniel Vetter daniel@ffwll.ch Sent: Friday, July 23, 2021 11:23 AM To: Arnd Bergmann arnd@kernel.org Cc: Daniel Vetter daniel@ffwll.ch; Ben Skeggs bskeggs@redhat.com; David Airlie airlied@linux.ie; Lyude Paul lyude@redhat.com; Arnd Bergmann arnd@arndb.de; Ville Syrjälä ville.syrjala@linux.intel.com; Cornij, Nikola Nikola.Cornij@amd.com; dri-devel dri-devel@lists.freedesktop.org; Nouveau Dev nouveau@lists.freedesktop.org; Linux Kernel Mailing List linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
On Fri, Jul 23, 2021 at 12:16:31PM +0200, Arnd Bergmann wrote:
On Fri, Jul 23, 2021 at 11:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
Yes! I'd definitely be in favor of that, I've wasted way too much time trying to sort through dependency loops and other problems with backlight support.
Maybe we should leave the drivers/video/fbdev/ drivers untouched in this regard, at least for the moment, but for the drivers/gpu/drm users of backlight that would be a nice simplification, and even the smallest ones are unlikely to be used on systems that are too memory constrained to deal with 4KB extra .text.
Yeah I think we can do this entirely ad-hoc, i.e. any time the backlight wheel wobbles off again we nail it down for good for that driver with a depends on BACKGLIGHT and remove any lingering #ifdef all over.
If you want maybe start out with the biggest drm drivers in a series, I think if nouveau/amdgpu/i915 folks ack this you're good to go to just convert as things get in the way. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll....
On 2021-07-23 3:14 p.m., Cornij, Nikola wrote:
[AMD Official Use Only]
+Harry
-----Original Message----- From: Daniel Vetter daniel@ffwll.ch Sent: Friday, July 23, 2021 11:23 AM To: Arnd Bergmann arnd@kernel.org Cc: Daniel Vetter daniel@ffwll.ch; Ben Skeggs bskeggs@redhat.com; David Airlie airlied@linux.ie; Lyude Paul lyude@redhat.com; Arnd Bergmann arnd@arndb.de; Ville Syrjälä ville.syrjala@linux.intel.com; Cornij, Nikola Nikola.Cornij@amd.com; dri-devel dri-devel@lists.freedesktop.org; Nouveau Dev nouveau@lists.freedesktop.org; Linux Kernel Mailing List linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
On Fri, Jul 23, 2021 at 12:16:31PM +0200, Arnd Bergmann wrote:
On Fri, Jul 23, 2021 at 11:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
Yes! I'd definitely be in favor of that, I've wasted way too much time trying to sort through dependency loops and other problems with backlight support.
Maybe we should leave the drivers/video/fbdev/ drivers untouched in this regard, at least for the moment, but for the drivers/gpu/drm users of backlight that would be a nice simplification, and even the smallest ones are unlikely to be used on systems that are too memory constrained to deal with 4KB extra .text.
Yeah I think we can do this entirely ad-hoc, i.e. any time the backlight wheel wobbles off again we nail it down for good for that driver with a depends on BACKGLIGHT and remove any lingering #ifdef all over.
If you want maybe start out with the biggest drm drivers in a series, I think if nouveau/amdgpu/i915 folks ack this you're good to go to just convert as things get in the way.
Sounds like a good idea to me.
Harry
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch/%3E%3E
On Fri, 2021-07-23 at 11:24 +0200, Daniel Vetter wrote:
On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector-
backlight;
| ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight-
edp_info);
| ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
Can we just toss the idea that BACKTLIGHT=n is a reasonable config for drm drivers using backlights, and add depends BACKLIGHT to all of them?
Yeah - I'm fine with this IMHO, at least for the drivers actually supporting backlights in some manner (I assume this is most of them though)
I mean this is a perfect source of continued patch streams to keep us all busy, but beyond that I really don't see the point ... I frankly have better things to do, and especially with the big drivers we have making backlight optional saves comparitively nothing. -Daniel
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); - struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder); - struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux; - int ret; u8 pwr;
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT + struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); + struct nouveau_backlight *backlight = nv_connector->backlight;
if (backlight && backlight->uses_dpcd) { - ret = drm_edp_backlight_disable(aux, &backlight-
edp_info);
+ int ret = drm_edp_backlight_disable(aux, &backlight-
edp_info);
if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector-
base.name, ret);
} +#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr); -- 2.29.2
On 7/23/21 2:15 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
- struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
- struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;
- int ret; u8 pwr;
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
- struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
- struct nouveau_backlight *backlight = nv_connector->backlight;
- if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);- if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }
+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);
Hm, only Lyude Paul replied to this patch:
https://lore.kernel.org/lkml/20210714171523.413-1-rdunlap@infradead.org/
On Fri, Jul 23, 2021 at 5:10 PM Randy Dunlap rdunlap@infradead.org wrote:
On 7/23/21 2:15 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;int ret; u8 pwr;+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);struct nouveau_backlight *backlight = nv_connector->backlight;if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);Hm, only Lyude Paul replied to this patch:
https://lore.kernel.org/lkml/20210714171523.413-1-rdunlap@infradead.org/
what's actually the use case of compiling with CONFIG_DRM_NOUVEAU_BACKLIGHT=n anyway?
-- ~Randy
On 7/23/21 8:15 AM, Karol Herbst wrote:
On Fri, Jul 23, 2021 at 5:10 PM Randy Dunlap rdunlap@infradead.org wrote:
On 7/23/21 2:15 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;int ret; u8 pwr;+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);struct nouveau_backlight *backlight = nv_connector->backlight;if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);Hm, only Lyude Paul replied to this patch:
https://lore.kernel.org/lkml/20210714171523.413-1-rdunlap@infradead.org/
what's actually the use case of compiling with CONFIG_DRM_NOUVEAU_BACKLIGHT=n anyway?
Dunno. In this case it was just a randconfig. Still, it needs to be handled in some way - such as the other suggestion in this thread.
On Fri, Jul 23, 2021 at 6:31 PM Randy Dunlap rdunlap@infradead.org wrote:
On 7/23/21 8:15 AM, Karol Herbst wrote:
On Fri, Jul 23, 2021 at 5:10 PM Randy Dunlap rdunlap@infradead.org wrote:
On 7/23/21 2:15 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
When the backlight support is disabled, the driver fails to build:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_sor_atomic_disable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct nouveau_connector' has no member named 'backlight' 1665 | struct nouveau_backlight *backlight = nv_connector->backlight; | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of undefined type 'struct nouveau_backlight' 1670 | if (backlight && backlight->uses_dpcd) { | ^~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of undefined type 'struct nouveau_backlight' 1671 | ret = drm_edp_backlight_disable(aux, &backlight->edp_info); | ^~
The patch that introduced the problem already contains some #ifdef checks, so just add another one that makes it build again.
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/nouveau/dispnv50/disp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..fcf53e24db21 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1659,20 +1659,23 @@ static void nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);struct nouveau_backlight *backlight = nv_connector->backlight; struct drm_dp_aux *aux = &nv_connector->aux;int ret; u8 pwr;+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);struct nouveau_backlight *backlight = nv_connector->backlight;if (backlight && backlight->uses_dpcd) {
ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
int ret = drm_edp_backlight_disable(aux, &backlight->edp_info);if (ret < 0) NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n", nv_connector->base.base.id, nv_connector->base.name, ret); }+#endif
if (nv_encoder->dcb->type == DCB_OUTPUT_DP) { int ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &pwr);Hm, only Lyude Paul replied to this patch:
https://lore.kernel.org/lkml/20210714171523.413-1-rdunlap@infradead.org/
what's actually the use case of compiling with CONFIG_DRM_NOUVEAU_BACKLIGHT=n anyway?
Dunno. In this case it was just a randconfig. Still, it needs to be handled in some way - such as the other suggestion in this thread.
sure, I was just curious if there was a specific use case or just something random as you mentioned.
On Fri, Jul 23, 2021 at 6:34 PM Karol Herbst kherbst@redhat.com wrote:
On Fri, Jul 23, 2021 at 6:31 PM Randy Dunlap rdunlap@infradead.org wrote:
On 7/23/21 8:15 AM, Karol Herbst wrote:
On Fri, Jul 23, 2021 at 5:10 PM Randy Dunlap rdunlap@infradead.org wrote:
what's actually the use case of compiling with CONFIG_DRM_NOUVEAU_BACKLIGHT=n anyway?
Dunno. In this case it was just a randconfig. Still, it needs to be handled in some way - such as the other suggestion in this thread.
sure, I was just curious if there was a specific use case or just something random as you mentioned.
I think this is purely done because of tradition. A long time ago, we had tiny framebuffer drivers and most PCs did not have backlights, so it made sense to leave this optional.
This was probably just always carried over.
Arnd
On Fri, Jul 23, 2021 at 8:40 PM Arnd Bergmann arnd@kernel.org wrote:
On Fri, Jul 23, 2021 at 6:34 PM Karol Herbst kherbst@redhat.com wrote:
On Fri, Jul 23, 2021 at 6:31 PM Randy Dunlap rdunlap@infradead.org wrote:
On 7/23/21 8:15 AM, Karol Herbst wrote:
On Fri, Jul 23, 2021 at 5:10 PM Randy Dunlap rdunlap@infradead.org wrote:
what's actually the use case of compiling with CONFIG_DRM_NOUVEAU_BACKLIGHT=n anyway?
Dunno. In this case it was just a randconfig. Still, it needs to be handled in some way - such as the other suggestion in this thread.
sure, I was just curious if there was a specific use case or just something random as you mentioned.
I think this is purely done because of tradition. A long time ago, we had tiny framebuffer drivers and most PCs did not have backlights, so it made sense to leave this optional.
This was probably just always carried over.
Arnd
okay. I think I will write a patch for nouveau then and send it out soonish
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Cornij, Nikola -
Daniel Vetter -
Harry Wentland -
Karol Herbst -
Lyude Paul -
Randy Dunlap