* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 9:24 ` Daniel Vetter
0 siblings, 0 replies; 43+ messages in thread
From: Daniel Vetter @ 2021-07-23 9:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ben Skeggs, David Airlie, Lyude Paul, Arnd Bergmann,
Ville Syrjälä, Nikola Cornij, dri-devel, Nouveau Dev,
Linux Kernel Mailing List
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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 9:24 ` Daniel Vetter
0 siblings, 0 replies; 43+ messages in thread
From: Daniel Vetter @ 2021-07-23 9:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 9:24 ` Daniel Vetter
@ 2021-07-23 10:10 ` Karol Herbst
-1 siblings, 0 replies; 43+ messages in thread
From: Karol Herbst @ 2021-07-23 10:10 UTC (permalink / raw)
To: Daniel Vetter
Cc: Arnd Bergmann, Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 10:10 ` Karol Herbst
0 siblings, 0 replies; 43+ messages in thread
From: Karol Herbst @ 2021-07-23 10:10 UTC (permalink / raw)
To: Daniel Vetter
Cc: Arnd Bergmann, Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 10:10 ` Karol Herbst
@ 2021-07-23 10:12 ` Karol Herbst
-1 siblings, 0 replies; 43+ messages in thread
From: Karol Herbst @ 2021-07-23 10:12 UTC (permalink / raw)
To: Daniel Vetter
Cc: Arnd Bergmann, Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
> >
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 10:12 ` Karol Herbst
0 siblings, 0 replies; 43+ messages in thread
From: Karol Herbst @ 2021-07-23 10:12 UTC (permalink / raw)
To: Daniel Vetter
Cc: Arnd Bergmann, Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
> >
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 9:24 ` Daniel Vetter
(?)
@ 2021-07-23 10:16 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2021-07-23 10:16 UTC (permalink / raw)
To: Daniel Vetter
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs,
Ville Syrjälä
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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 10:16 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2021-07-23 10:16 UTC (permalink / raw)
To: Daniel Vetter
Cc: Ben Skeggs, David Airlie, Lyude Paul, Arnd Bergmann,
Ville Syrjälä, Nikola Cornij, dri-devel, Nouveau Dev,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 10:16 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2021-07-23 10:16 UTC (permalink / raw)
To: Daniel Vetter
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 10:16 ` Arnd Bergmann
(?)
@ 2021-07-23 15:23 ` Daniel Vetter
-1 siblings, 0 replies; 43+ messages in thread
From: Daniel Vetter @ 2021-07-23 15:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs,
Daniel Vetter, Ville Syrjälä
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
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 15:23 ` Daniel Vetter
0 siblings, 0 replies; 43+ messages in thread
From: Daniel Vetter @ 2021-07-23 15:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Daniel Vetter, Ben Skeggs, David Airlie, Lyude Paul,
Arnd Bergmann, Ville Syrjälä, Nikola Cornij, dri-devel,
Nouveau Dev, Linux Kernel Mailing List
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
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 15:23 ` Daniel Vetter
0 siblings, 0 replies; 43+ messages in thread
From: Daniel Vetter @ 2021-07-23 15:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 15:23 ` Daniel Vetter
(?)
@ 2021-07-23 19:14 ` Cornij, Nikola
-1 siblings, 0 replies; 43+ messages in thread
From: Cornij, Nikola @ 2021-07-23 19:14 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Ben Skeggs, Wentland, Harry,
Ville Syrjälä
[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.ch%2F&data=04%7C01%7Cnikola.cornij%40amd.com%7C48104ef3c5724531e8f708d94dedcf83%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637626506075627495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KWVzpQyZsBh4jo522b36pru1zPBxxW2wvXUAjJ6u3G8%3D&reserved=0
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* RE: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 19:14 ` Cornij, Nikola
0 siblings, 0 replies; 43+ messages in thread
From: Cornij, Nikola @ 2021-07-23 19:14 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Ben Skeggs, David Airlie, Lyude Paul, Arnd Bergmann,
Ville Syrjälä, dri-devel, Nouveau Dev,
Linux Kernel Mailing List, Wentland, Harry
[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.ch%2F&data=04%7C01%7Cnikola.cornij%40amd.com%7C48104ef3c5724531e8f708d94dedcf83%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637626506075627495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KWVzpQyZsBh4jo522b36pru1zPBxxW2wvXUAjJ6u3G8%3D&reserved=0
^ permalink raw reply [flat|nested] 43+ messages in thread* RE: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 19:14 ` Cornij, Nikola
0 siblings, 0 replies; 43+ messages in thread
From: Cornij, Nikola @ 2021-07-23 19:14 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Ben Skeggs
[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.ch%2F&data=04%7C01%7Cnikola.cornij%40amd.com%7C48104ef3c5724531e8f708d94dedcf83%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637626506075627495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KWVzpQyZsBh4jo522b36pru1zPBxxW2wvXUAjJ6u3G8%3D&reserved=0
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 19:14 ` Cornij, Nikola
(?)
@ 2021-07-23 19:54 ` Harry Wentland
-1 siblings, 0 replies; 43+ messages in thread
From: Harry Wentland @ 2021-07-23 19:54 UTC (permalink / raw)
To: Cornij, Nikola, Daniel Vetter, Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Ben Skeggs,
Ville Syrjälä
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/>>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 19:54 ` Harry Wentland
0 siblings, 0 replies; 43+ messages in thread
From: Harry Wentland @ 2021-07-23 19:54 UTC (permalink / raw)
To: Cornij, Nikola, Daniel Vetter, Arnd Bergmann
Cc: Ben Skeggs, David Airlie, Lyude Paul, Arnd Bergmann,
Ville Syrjälä, dri-devel, Nouveau Dev,
Linux Kernel Mailing List
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/>>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 19:54 ` Harry Wentland
0 siblings, 0 replies; 43+ messages in thread
From: Harry Wentland @ 2021-07-23 19:54 UTC (permalink / raw)
To: Cornij, Nikola, Daniel Vetter, Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Ben Skeggs
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/>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
2021-07-23 9:24 ` Daniel Vetter
(?)
@ 2021-07-23 16:12 ` Lyude Paul
-1 siblings, 0 replies; 43+ messages in thread
From: Lyude Paul @ 2021-07-23 16:12 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs,
Ville Syrjälä
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
> >
>
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 16:12 ` Lyude Paul
0 siblings, 0 replies; 43+ messages in thread
From: Lyude Paul @ 2021-07-23 16:12 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Ben Skeggs, David Airlie, Arnd Bergmann, Ville Syrjälä,
Nikola Cornij, dri-devel, Nouveau Dev, Linux Kernel Mailing List
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
> >
>
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
@ 2021-07-23 16:12 ` Lyude Paul
0 siblings, 0 replies; 43+ messages in thread
From: Lyude Paul @ 2021-07-23 16:12 UTC (permalink / raw)
To: Daniel Vetter, Arnd Bergmann
Cc: Arnd Bergmann, David Airlie, Nouveau Dev,
Linux Kernel Mailing List, dri-devel, Nikola Cornij, Ben Skeggs
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
> >
>
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 43+ messages in thread