[27/27] drm: Add default modes for connectors in unknown state
diff mbox series

Message ID 20200526011505.31884-28-laurent.pinchart+renesas@ideasonboard.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • [01/27] drm: bridge: adv7511: Split EDID read to a separate function
Related show

Commit Message

Laurent Pinchart May 26, 2020, 1:15 a.m. UTC
The DRM CRTC helpers add default modes to connectors in the connected
state if no mode can be retrieved from the connector. This behaviour is
useful for VGA or DVI outputs that have no connected DDC bus. However,
in such cases, the status of the output usually can't be retrieved and
is reported as connector_status_unknown.

Extend the addition of default modes to connectors in an unknown state
to support outputs that can retrieve neither the modes nor the
connection status.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
 include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Sam Ravnborg June 21, 2020, 8:40 a.m. UTC | #1
On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> The DRM CRTC helpers add default modes to connectors in the connected
> state if no mode can be retrieved from the connector. This behaviour is
> useful for VGA or DVI outputs that have no connected DDC bus. However,
> in such cases, the status of the output usually can't be retrieved and
> is reported as connector_status_unknown.
> 
> Extend the addition of default modes to connectors in an unknown state
> to support outputs that can retrieve neither the modes nor the
> connection status.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

From your description sounds like an OK approach.
But this is not something I feel too familiar with.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
>  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index f5d141e0400f..9055d9573c90 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  	if (count == 0 && connector->status == connector_status_connected)
>  		count = drm_add_override_edid_modes(connector);
>  
> -	if (count == 0 && connector->status == connector_status_connected)
> +	if (count == 0 && (connector->status == connector_status_connected ||
> +			   connector->status == connector_status_unknown))
>  		count = drm_add_modes_noedid(connector, 1024, 768);
>  	count += drm_helper_probe_add_cmdline_mode(connector);
>  	if (count == 0)
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 421a30f08463..afe55e2e93d2 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
>  	 * The usual way to implement this is to cache the EDID retrieved in the
>  	 * probe callback somewhere in the driver-private connector structure.
>  	 * In this function drivers then parse the modes in the EDID and add
> -	 * them by calling drm_add_edid_modes(). But connectors that driver a
> +	 * them by calling drm_add_edid_modes(). But connectors that drive a
>  	 * fixed panel can also manually add specific modes using
>  	 * drm_mode_probed_add(). Drivers which manually add modes should also
>  	 * make sure that the &drm_connector.display_info,
>  	 * &drm_connector.width_mm and &drm_connector.height_mm fields are
>  	 * filled in.
>  	 *
> +	 * Note that the caller function will automatically add standard VESA
> +	 * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> +	 * no mode and if the connector status is connector_status_connected or
> +	 * connector_status_unknown. There is no need to call
> +	 * drm_add_edid_modes() manually in that case.
> +	 *
>  	 * Virtual drivers that just want some standard VESA mode with a given
>  	 * resolution can call drm_add_modes_noedid(), and mark the preferred
>  	 * one using drm_set_preferred_mode().
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart June 24, 2020, 1:12 a.m. UTC | #2
Hi Sam,

On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > The DRM CRTC helpers add default modes to connectors in the connected
> > state if no mode can be retrieved from the connector. This behaviour is
> > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > in such cases, the status of the output usually can't be retrieved and
> > is reported as connector_status_unknown.
> > 
> > Extend the addition of default modes to connectors in an unknown state
> > to support outputs that can retrieve neither the modes nor the
> > connection status.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> From your description sounds like an OK approach.
> But this is not something I feel too familiar with.
> Acked-by: Sam Ravnborg <sam@ravnborg.org>

Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
too.

> > ---
> >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index f5d141e0400f..9055d9573c90 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >  	if (count == 0 && connector->status == connector_status_connected)
> >  		count = drm_add_override_edid_modes(connector);
> >  
> > -	if (count == 0 && connector->status == connector_status_connected)
> > +	if (count == 0 && (connector->status == connector_status_connected ||
> > +			   connector->status == connector_status_unknown))
> >  		count = drm_add_modes_noedid(connector, 1024, 768);
> >  	count += drm_helper_probe_add_cmdline_mode(connector);
> >  	if (count == 0)
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 421a30f08463..afe55e2e93d2 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> >  	 * The usual way to implement this is to cache the EDID retrieved in the
> >  	 * probe callback somewhere in the driver-private connector structure.
> >  	 * In this function drivers then parse the modes in the EDID and add
> > -	 * them by calling drm_add_edid_modes(). But connectors that driver a
> > +	 * them by calling drm_add_edid_modes(). But connectors that drive a
> >  	 * fixed panel can also manually add specific modes using
> >  	 * drm_mode_probed_add(). Drivers which manually add modes should also
> >  	 * make sure that the &drm_connector.display_info,
> >  	 * &drm_connector.width_mm and &drm_connector.height_mm fields are
> >  	 * filled in.
> >  	 *
> > +	 * Note that the caller function will automatically add standard VESA
> > +	 * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > +	 * no mode and if the connector status is connector_status_connected or
> > +	 * connector_status_unknown. There is no need to call
> > +	 * drm_add_edid_modes() manually in that case.
> > +	 *
> >  	 * Virtual drivers that just want some standard VESA mode with a given
> >  	 * resolution can call drm_add_modes_noedid(), and mark the preferred
> >  	 * one using drm_set_preferred_mode().
Daniel Vetter June 24, 2020, 7:23 a.m. UTC | #3
On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > The DRM CRTC helpers add default modes to connectors in the connected
> > > state if no mode can be retrieved from the connector. This behaviour is
> > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > in such cases, the status of the output usually can't be retrieved and
> > > is reported as connector_status_unknown.
> > > 
> > > Extend the addition of default modes to connectors in an unknown state
> > > to support outputs that can retrieve neither the modes nor the
> > > connection status.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > From your description sounds like an OK approach.
> > But this is not something I feel too familiar with.
> > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> too.

Makes sense, and at least pre-coffee me can't immediately think of a
scenario where we're going to regret this. _unknown status is pretty much
limited to old VGA and similar things where load detect somehow isn't well
supported by the hw.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> > > ---
> > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index f5d141e0400f..9055d9573c90 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > >  	if (count == 0 && connector->status == connector_status_connected)
> > >  		count = drm_add_override_edid_modes(connector);
> > >  
> > > -	if (count == 0 && connector->status == connector_status_connected)
> > > +	if (count == 0 && (connector->status == connector_status_connected ||
> > > +			   connector->status == connector_status_unknown))
> > >  		count = drm_add_modes_noedid(connector, 1024, 768);
> > >  	count += drm_helper_probe_add_cmdline_mode(connector);
> > >  	if (count == 0)
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > index 421a30f08463..afe55e2e93d2 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > >  	 * The usual way to implement this is to cache the EDID retrieved in the
> > >  	 * probe callback somewhere in the driver-private connector structure.
> > >  	 * In this function drivers then parse the modes in the EDID and add
> > > -	 * them by calling drm_add_edid_modes(). But connectors that driver a
> > > +	 * them by calling drm_add_edid_modes(). But connectors that drive a
> > >  	 * fixed panel can also manually add specific modes using
> > >  	 * drm_mode_probed_add(). Drivers which manually add modes should also
> > >  	 * make sure that the &drm_connector.display_info,
> > >  	 * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > >  	 * filled in.
> > >  	 *
> > > +	 * Note that the caller function will automatically add standard VESA
> > > +	 * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > +	 * no mode and if the connector status is connector_status_connected or
> > > +	 * connector_status_unknown. There is no need to call
> > > +	 * drm_add_edid_modes() manually in that case.

Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
... Personally I'd just leave out the last sentence, I think that only
confuses readers. Or I'm not grasphing what you're trying to tell here.

r-b with or without this change since imo super tiny nit.

Cheers, Daniel

> > > +	 *
> > >  	 * Virtual drivers that just want some standard VESA mode with a given
> > >  	 * resolution can call drm_add_modes_noedid(), and mark the preferred
> > >  	 * one using drm_set_preferred_mode().
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Alex Deucher June 24, 2020, 3:24 p.m. UTC | #4
On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > Hi Sam,
> >
> > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > in such cases, the status of the output usually can't be retrieved and
> > > > is reported as connector_status_unknown.
> > > >
> > > > Extend the addition of default modes to connectors in an unknown state
> > > > to support outputs that can retrieve neither the modes nor the
> > > > connection status.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >
> > > From your description sounds like an OK approach.
> > > But this is not something I feel too familiar with.
> > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> >
> > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > too.
>
> Makes sense, and at least pre-coffee me can't immediately think of a
> scenario where we're going to regret this. _unknown status is pretty much
> limited to old VGA and similar things where load detect somehow isn't well
> supported by the hw.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> >
> > > > ---
> > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > index f5d141e0400f..9055d9573c90 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > >   if (count == 0 && connector->status == connector_status_connected)
> > > >           count = drm_add_override_edid_modes(connector);
> > > >
> > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > +                    connector->status == connector_status_unknown))
> > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > >   if (count == 0)
> > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > index 421a30f08463..afe55e2e93d2 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > >    * probe callback somewhere in the driver-private connector structure.
> > > >    * In this function drivers then parse the modes in the EDID and add
> > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > >    * fixed panel can also manually add specific modes using
> > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > >    * make sure that the &drm_connector.display_info,
> > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > >    * filled in.
> > > >    *
> > > > +  * Note that the caller function will automatically add standard VESA
> > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > +  * no mode and if the connector status is connector_status_connected or
> > > > +  * connector_status_unknown. There is no need to call
> > > > +  * drm_add_edid_modes() manually in that case.
>
> Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> ... Personally I'd just leave out the last sentence, I think that only
> confuses readers. Or I'm not grasphing what you're trying to tell here.

IIRC, some drivers used and desktop environments expected unknown
rather than off for LVDS/eDP panels when the lid was shut or if the
mux was switched to another device in the case of hybrid laptops.

Alex


>
> r-b with or without this change since imo super tiny nit.
>
> Cheers, Daniel
>
> > > > +  *
> > > >    * Virtual drivers that just want some standard VESA mode with a given
> > > >    * resolution can call drm_add_modes_noedid(), and mark the preferred
> > > >    * one using drm_set_preferred_mode().
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter June 24, 2020, 7:31 p.m. UTC | #5
On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > Hi Sam,
> > >
> > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > is reported as connector_status_unknown.
> > > > >
> > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > to support outputs that can retrieve neither the modes nor the
> > > > > connection status.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > >
> > > > From your description sounds like an OK approach.
> > > > But this is not something I feel too familiar with.
> > > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > >
> > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > too.
> >
> > Makes sense, and at least pre-coffee me can't immediately think of a
> > scenario where we're going to regret this. _unknown status is pretty much
> > limited to old VGA and similar things where load detect somehow isn't well
> > supported by the hw.
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > >
> > > > > ---
> > > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > > >   if (count == 0 && connector->status == connector_status_connected)
> > > > >           count = drm_add_override_edid_modes(connector);
> > > > >
> > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > > +                    connector->status == connector_status_unknown))
> > > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > > >   if (count == 0)
> > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > > >    * probe callback somewhere in the driver-private connector structure.
> > > > >    * In this function drivers then parse the modes in the EDID and add
> > > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > >    * fixed panel can also manually add specific modes using
> > > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > > >    * make sure that the &drm_connector.display_info,
> > > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > >    * filled in.
> > > > >    *
> > > > > +  * Note that the caller function will automatically add standard VESA
> > > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > > +  * no mode and if the connector status is connector_status_connected or
> > > > > +  * connector_status_unknown. There is no need to call
> > > > > +  * drm_add_edid_modes() manually in that case.
> >
> > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > ... Personally I'd just leave out the last sentence, I think that only
> > confuses readers. Or I'm not grasphing what you're trying to tell here.
>
> IIRC, some drivers used and desktop environments expected unknown
> rather than off for LVDS/eDP panels when the lid was shut or if the
> mux was switched to another device in the case of hybrid laptops.

We seem to have totally ditched that in

commit 05c72e77ccda89ff624108b1b59a0fc43843f343
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Tue Jul 17 20:42:14 2018 +0300

    drm/i915: Nuke the LVDS lid notifier

No screaming yet.

But I'm also a bit confused, for a panel there's generally an edid
around, or a fixed (list of) modes. That's enough to stop this
fallback from running, so should be all fine.
-Daniell

>
> Alex
>
>
> >
> > r-b with or without this change since imo super tiny nit.
> >
> > Cheers, Daniel
> >
> > > > > +  *
> > > > >    * Virtual drivers that just want some standard VESA mode with a given
> > > > >    * resolution can call drm_add_modes_noedid(), and mark the preferred
> > > > >    * one using drm_set_preferred_mode().
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Alex Deucher June 24, 2020, 7:40 p.m. UTC | #6
On Wed, Jun 24, 2020 at 3:31 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > > Hi Sam,
> > > >
> > > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > > is reported as connector_status_unknown.
> > > > > >
> > > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > > to support outputs that can retrieve neither the modes nor the
> > > > > > connection status.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > >
> > > > > From your description sounds like an OK approach.
> > > > > But this is not something I feel too familiar with.
> > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > > >
> > > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > > too.
> > >
> > > Makes sense, and at least pre-coffee me can't immediately think of a
> > > scenario where we're going to regret this. _unknown status is pretty much
> > > limited to old VGA and similar things where load detect somehow isn't well
> > > supported by the hw.
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > > > >   if (count == 0 && connector->status == connector_status_connected)
> > > > > >           count = drm_add_override_edid_modes(connector);
> > > > > >
> > > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > > > +                    connector->status == connector_status_unknown))
> > > > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > >   if (count == 0)
> > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > > > >    * probe callback somewhere in the driver-private connector structure.
> > > > > >    * In this function drivers then parse the modes in the EDID and add
> > > > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > > >    * fixed panel can also manually add specific modes using
> > > > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > > > >    * make sure that the &drm_connector.display_info,
> > > > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > > >    * filled in.
> > > > > >    *
> > > > > > +  * Note that the caller function will automatically add standard VESA
> > > > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > > > +  * no mode and if the connector status is connector_status_connected or
> > > > > > +  * connector_status_unknown. There is no need to call
> > > > > > +  * drm_add_edid_modes() manually in that case.
> > >
> > > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > > ... Personally I'd just leave out the last sentence, I think that only
> > > confuses readers. Or I'm not grasphing what you're trying to tell here.
> >
> > IIRC, some drivers used and desktop environments expected unknown
> > rather than off for LVDS/eDP panels when the lid was shut or if the
> > mux was switched to another device in the case of hybrid laptops.
>
> We seem to have totally ditched that in
>
> commit 05c72e77ccda89ff624108b1b59a0fc43843f343
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Tue Jul 17 20:42:14 2018 +0300
>
>     drm/i915: Nuke the LVDS lid notifier
>
> No screaming yet.
>
> But I'm also a bit confused, for a panel there's generally an edid
> around, or a fixed (list of) modes. That's enough to stop this
> fallback from running, so should be all fine.

No, you are right; you will have the EDID so this shouldn't be an
issue.  I was mis-remembering the original issue.  We originally
always reported connected for LVDS in radeon if the panel was present,
but then we got flack because some userspace expected unknown in
certain cases (e.g., lid or muxed displays).  Either way the EDID info
is still there.

Alex


> -Daniell
>
> >
> > Alex
> >
> >
> > >
> > > r-b with or without this change since imo super tiny nit.
> > >
> > > Cheers, Daniel
> > >
> > > > > > +  *
> > > > > >    * Virtual drivers that just want some standard VESA mode with a given
> > > > > >    * resolution can call drm_add_modes_noedid(), and mark the preferred
> > > > > >    * one using drm_set_preferred_mode().
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Laurent Pinchart June 24, 2020, 10:47 p.m. UTC | #7
Hi Daniel,

On Wed, Jun 24, 2020 at 09:23:04AM +0200, Daniel Vetter wrote:
> On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > in such cases, the status of the output usually can't be retrieved and
> > > > is reported as connector_status_unknown.
> > > > 
> > > > Extend the addition of default modes to connectors in an unknown state
> > > > to support outputs that can retrieve neither the modes nor the
> > > > connection status.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > 
> > > From your description sounds like an OK approach.
> > > But this is not something I feel too familiar with.
> > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > 
> > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > too.
> 
> Makes sense, and at least pre-coffee me can't immediately think of a
> scenario where we're going to regret this. _unknown status is pretty much
> limited to old VGA and similar things where load detect somehow isn't well
> supported by the hw.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > > > ---
> > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > index f5d141e0400f..9055d9573c90 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > >  	if (count == 0 && connector->status == connector_status_connected)
> > > >  		count = drm_add_override_edid_modes(connector);
> > > >  
> > > > -	if (count == 0 && connector->status == connector_status_connected)
> > > > +	if (count == 0 && (connector->status == connector_status_connected ||
> > > > +			   connector->status == connector_status_unknown))
> > > >  		count = drm_add_modes_noedid(connector, 1024, 768);
> > > >  	count += drm_helper_probe_add_cmdline_mode(connector);
> > > >  	if (count == 0)
> > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > index 421a30f08463..afe55e2e93d2 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > >  	 * The usual way to implement this is to cache the EDID retrieved in the
> > > >  	 * probe callback somewhere in the driver-private connector structure.
> > > >  	 * In this function drivers then parse the modes in the EDID and add
> > > > -	 * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > +	 * them by calling drm_add_edid_modes(). But connectors that drive a
> > > >  	 * fixed panel can also manually add specific modes using
> > > >  	 * drm_mode_probed_add(). Drivers which manually add modes should also
> > > >  	 * make sure that the &drm_connector.display_info,
> > > >  	 * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > >  	 * filled in.
> > > >  	 *
> > > > +	 * Note that the caller function will automatically add standard VESA
> > > > +	 * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > +	 * no mode and if the connector status is connector_status_connected or
> > > > +	 * connector_status_unknown. There is no need to call
> > > > +	 * drm_add_edid_modes() manually in that case.
> 
> Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> ... Personally I'd just leave out the last sentence, I think that only
> confuses readers. Or I'm not grasphing what you're trying to tell here.

Sorry, I meant drm_add_modes_noedid(). Is that clearer ?

> r-b with or without this change since imo super tiny nit.
> 
> > > > +	 *
> > > >  	 * Virtual drivers that just want some standard VESA mode with a given
> > > >  	 * resolution can call drm_add_modes_noedid(), and mark the preferred
> > > >  	 * one using drm_set_preferred_mode().
Daniel Vetter June 25, 2020, 7:56 a.m. UTC | #8
On Wed, Jun 24, 2020 at 03:40:42PM -0400, Alex Deucher wrote:
> On Wed, Jun 24, 2020 at 3:31 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > > > Hi Sam,
> > > > >
> > > > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > > > is reported as connector_status_unknown.
> > > > > > >
> > > > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > > > to support outputs that can retrieve neither the modes nor the
> > > > > > > connection status.
> > > > > > >
> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > >
> > > > > > From your description sounds like an OK approach.
> > > > > > But this is not something I feel too familiar with.
> > > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > > > >
> > > > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > > > too.
> > > >
> > > > Makes sense, and at least pre-coffee me can't immediately think of a
> > > > scenario where we're going to regret this. _unknown status is pretty much
> > > > limited to old VGA and similar things where load detect somehow isn't well
> > > > supported by the hw.
> > > >
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > >
> > > > >
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > > > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > > > > >   if (count == 0 && connector->status == connector_status_connected)
> > > > > > >           count = drm_add_override_edid_modes(connector);
> > > > > > >
> > > > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > > > > +                    connector->status == connector_status_unknown))
> > > > > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > > >   if (count == 0)
> > > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > > > > >    * probe callback somewhere in the driver-private connector structure.
> > > > > > >    * In this function drivers then parse the modes in the EDID and add
> > > > > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > > > >    * fixed panel can also manually add specific modes using
> > > > > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > > > > >    * make sure that the &drm_connector.display_info,
> > > > > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > > > >    * filled in.
> > > > > > >    *
> > > > > > > +  * Note that the caller function will automatically add standard VESA
> > > > > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > > > > +  * no mode and if the connector status is connector_status_connected or
> > > > > > > +  * connector_status_unknown. There is no need to call
> > > > > > > +  * drm_add_edid_modes() manually in that case.
> > > >
> > > > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > > > ... Personally I'd just leave out the last sentence, I think that only
> > > > confuses readers. Or I'm not grasphing what you're trying to tell here.
> > >
> > > IIRC, some drivers used and desktop environments expected unknown
> > > rather than off for LVDS/eDP panels when the lid was shut or if the
> > > mux was switched to another device in the case of hybrid laptops.
> >
> > We seem to have totally ditched that in
> >
> > commit 05c72e77ccda89ff624108b1b59a0fc43843f343
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Tue Jul 17 20:42:14 2018 +0300
> >
> >     drm/i915: Nuke the LVDS lid notifier
> >
> > No screaming yet.
> >
> > But I'm also a bit confused, for a panel there's generally an edid
> > around, or a fixed (list of) modes. That's enough to stop this
> > fallback from running, so should be all fine.
> 
> No, you are right; you will have the EDID so this shouldn't be an
> issue.  I was mis-remembering the original issue.  We originally
> always reported connected for LVDS in radeon if the panel was present,
> but then we got flack because some userspace expected unknown in
> certain cases (e.g., lid or muxed displays).  Either way the EDID info
> is still there.

Yeah I think i915 started that habit, but I guess people realized it's
unreliable enough that they should have their own lid handler in the
desktop enviromnent doing whatever they want to do on lid close.

Should we perhaps document that somewhere, that panels are always marked
as connected? Not even sure where to put that in the docs ...

Maybe adding a few of the usual suspects from the compositor side, Simon,
Pekka?
-Daniel

> 
> Alex
> 
> 
> > -Daniell
> >
> > >
> > > Alex
> > >
> > >
> > > >
> > > > r-b with or without this change since imo super tiny nit.
> > > >
> > > > Cheers, Daniel
> > > >
> > > > > > > +  *
> > > > > > >    * Virtual drivers that just want some standard VESA mode with a given
> > > > > > >    * resolution can call drm_add_modes_noedid(), and mark the preferred
> > > > > > >    * one using drm_set_preferred_mode().
> > > > >
> > > > > --
> > > > > Regards,
> > > > >
> > > > > Laurent Pinchart
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
Daniel Vetter June 25, 2020, 7:57 a.m. UTC | #9
On Thu, Jun 25, 2020 at 9:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jun 24, 2020 at 03:40:42PM -0400, Alex Deucher wrote:
> > On Wed, Jun 24, 2020 at 3:31 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > > > > Hi Sam,
> > > > > >
> > > > > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > > > > is reported as connector_status_unknown.
> > > > > > > >
> > > > > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > > > > to support outputs that can retrieve neither the modes nor the
> > > > > > > > connection status.
> > > > > > > >
> > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > >
> > > > > > > From your description sounds like an OK approach.
> > > > > > > But this is not something I feel too familiar with.
> > > > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > > > > >
> > > > > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > > > > too.
> > > > >
> > > > > Makes sense, and at least pre-coffee me can't immediately think of a
> > > > > scenario where we're going to regret this. _unknown status is pretty much
> > > > > limited to old VGA and similar things where load detect somehow isn't well
> > > > > supported by the hw.
> > > > >
> > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > >
> > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > > > > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > > > > > >   if (count == 0 && connector->status == connector_status_connected)
> > > > > > > >           count = drm_add_override_edid_modes(connector);
> > > > > > > >
> > > > > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > > > > > +                    connector->status == connector_status_unknown))
> > > > > > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > > > >   if (count == 0)
> > > > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > > > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > > > > > >    * probe callback somewhere in the driver-private connector structure.
> > > > > > > >    * In this function drivers then parse the modes in the EDID and add
> > > > > > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > > > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > > > > >    * fixed panel can also manually add specific modes using
> > > > > > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > > > > > >    * make sure that the &drm_connector.display_info,
> > > > > > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > > > > >    * filled in.
> > > > > > > >    *
> > > > > > > > +  * Note that the caller function will automatically add standard VESA
> > > > > > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > > > > > +  * no mode and if the connector status is connector_status_connected or
> > > > > > > > +  * connector_status_unknown. There is no need to call
> > > > > > > > +  * drm_add_edid_modes() manually in that case.
> > > > >
> > > > > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > > > > ... Personally I'd just leave out the last sentence, I think that only
> > > > > confuses readers. Or I'm not grasphing what you're trying to tell here.
> > > >
> > > > IIRC, some drivers used and desktop environments expected unknown
> > > > rather than off for LVDS/eDP panels when the lid was shut or if the
> > > > mux was switched to another device in the case of hybrid laptops.
> > >
> > > We seem to have totally ditched that in
> > >
> > > commit 05c72e77ccda89ff624108b1b59a0fc43843f343
> > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Date:   Tue Jul 17 20:42:14 2018 +0300
> > >
> > >     drm/i915: Nuke the LVDS lid notifier
> > >
> > > No screaming yet.
> > >
> > > But I'm also a bit confused, for a panel there's generally an edid
> > > around, or a fixed (list of) modes. That's enough to stop this
> > > fallback from running, so should be all fine.
> >
> > No, you are right; you will have the EDID so this shouldn't be an
> > issue.  I was mis-remembering the original issue.  We originally
> > always reported connected for LVDS in radeon if the panel was present,
> > but then we got flack because some userspace expected unknown in
> > certain cases (e.g., lid or muxed displays).  Either way the EDID info
> > is still there.
>
> Yeah I think i915 started that habit, but I guess people realized it's
> unreliable enough that they should have their own lid handler in the
> desktop enviromnent doing whatever they want to do on lid close.
>
> Should we perhaps document that somewhere, that panels are always marked
> as connected? Not even sure where to put that in the docs ...
>
> Maybe adding a few of the usual suspects from the compositor side, Simon,
> Pekka?

Actually adding Simon and Pekka this time around ...

> -Daniel
>
> >
> > Alex
> >
> >
> > > -Daniell
> > >
> > > >
> > > > Alex
> > > >
> > > >
> > > > >
> > > > > r-b with or without this change since imo super tiny nit.
> > > > >
> > > > > Cheers, Daniel
> > > > >
> > > > > > > > +  *
> > > > > > > >    * Virtual drivers that just want some standard VESA mode with a given
> > > > > > > >    * resolution can call drm_add_modes_noedid(), and mark the preferred
> > > > > > > >    * one using drm_set_preferred_mode().
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > >
> > > > > > Laurent Pinchart
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Pekka Paalanen June 25, 2020, 10:31 a.m. UTC | #10
On Thu, 25 Jun 2020 09:57:44 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 25, 2020 at 9:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Jun 24, 2020 at 03:40:42PM -0400, Alex Deucher wrote:  
> > > On Wed, Jun 24, 2020 at 3:31 PM Daniel Vetter <daniel@ffwll.ch> wrote:  
> > > >
> > > > On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher <alexdeucher@gmail.com> wrote:  
> > > > >
> > > > > On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:  
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:  
> > > > > > > Hi Sam,
> > > > > > >
> > > > > > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:  
> > > > > > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:  
> > > > > > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > > > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > > > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > > > > > is reported as connector_status_unknown.
> > > > > > > > >
> > > > > > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > > > > > to support outputs that can retrieve neither the modes nor the
> > > > > > > > > connection status.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>  
> > > > > > > >
> > > > > > > > From your description sounds like an OK approach.
> > > > > > > > But this is not something I feel too familiar with.
> > > > > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org>  
> > > > > > >
> > > > > > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > > > > > too.  
> > > > > >
> > > > > > Makes sense, and at least pre-coffee me can't immediately think of a
> > > > > > scenario where we're going to regret this. _unknown status is pretty much
> > > > > > limited to old VGA and similar things where load detect somehow isn't well
> > > > > > supported by the hw.
> > > > > >
> > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > >  
> > > > > > >  
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > > > > > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > > > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > > > > > > >   if (count == 0 && connector->status == connector_status_connected)
> > > > > > > > >           count = drm_add_override_edid_modes(connector);
> > > > > > > > >
> > > > > > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > > > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > > > > > > +                    connector->status == connector_status_unknown))
> > > > > > > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > > > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > > > > >   if (count == 0)
> > > > > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > > > > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > > > > > > >    * probe callback somewhere in the driver-private connector structure.
> > > > > > > > >    * In this function drivers then parse the modes in the EDID and add
> > > > > > > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > > > > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > > > > > >    * fixed panel can also manually add specific modes using
> > > > > > > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > > > > > > >    * make sure that the &drm_connector.display_info,
> > > > > > > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > > > > > >    * filled in.
> > > > > > > > >    *
> > > > > > > > > +  * Note that the caller function will automatically add standard VESA
> > > > > > > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > > > > > > +  * no mode and if the connector status is connector_status_connected or
> > > > > > > > > +  * connector_status_unknown. There is no need to call
> > > > > > > > > +  * drm_add_edid_modes() manually in that case.  
> > > > > >
> > > > > > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > > > > > ... Personally I'd just leave out the last sentence, I think that only
> > > > > > confuses readers. Or I'm not grasphing what you're trying to tell here.  
> > > > >
> > > > > IIRC, some drivers used and desktop environments expected unknown
> > > > > rather than off for LVDS/eDP panels when the lid was shut or if the
> > > > > mux was switched to another device in the case of hybrid laptops.  
> > > >
> > > > We seem to have totally ditched that in
> > > >
> > > > commit 05c72e77ccda89ff624108b1b59a0fc43843f343
> > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Date:   Tue Jul 17 20:42:14 2018 +0300
> > > >
> > > >     drm/i915: Nuke the LVDS lid notifier
> > > >
> > > > No screaming yet.
> > > >
> > > > But I'm also a bit confused, for a panel there's generally an edid
> > > > around, or a fixed (list of) modes. That's enough to stop this
> > > > fallback from running, so should be all fine.  
> > >
> > > No, you are right; you will have the EDID so this shouldn't be an
> > > issue.  I was mis-remembering the original issue.  We originally
> > > always reported connected for LVDS in radeon if the panel was present,
> > > but then we got flack because some userspace expected unknown in
> > > certain cases (e.g., lid or muxed displays).  Either way the EDID info
> > > is still there.  
> >
> > Yeah I think i915 started that habit, but I guess people realized it's
> > unreliable enough that they should have their own lid handler in the
> > desktop enviromnent doing whatever they want to do on lid close.
> >
> > Should we perhaps document that somewhere, that panels are always marked
> > as connected? Not even sure where to put that in the docs ...
> >
> > Maybe adding a few of the usual suspects from the compositor side, Simon,
> > Pekka?  
> 
> Actually adding Simon and Pekka this time around ...

I don't know what anyone else does, but Weston (is not a DE) does not
look at any lid switch, and assumes that if connector status is not
DRM_MODE_CONNECTED, then it is disconnected. So, if a driver switched
to "Unknown" status, it would be taken as disconnected.

I never knew what "Unknown" was relevant for. In weston.ini you can
also force a connector on, so users could drive it regardless.

However, I would also say that Weston is not supposed to react to any
lid action exactly because it does not watch the lid switch. Personally
I would expect a built-in screen to stay on even if lid closed.

Panels that are always connected showing up as always connected, sounds
good to me.


Thanks,
pq
Daniel Vetter June 25, 2020, 10:44 a.m. UTC | #11
On Thu, Jun 25, 2020 at 12:32 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Thu, 25 Jun 2020 09:57:44 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Thu, Jun 25, 2020 at 9:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 03:40:42PM -0400, Alex Deucher wrote:
> > > > On Wed, Jun 24, 2020 at 3:31 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > >
> > > > > > > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > > > > > > Hi Sam,
> > > > > > > >
> > > > > > > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > > > > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > > > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > > > > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > > > > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > > > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > > > > > > is reported as connector_status_unknown.
> > > > > > > > > >
> > > > > > > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > > > > > > to support outputs that can retrieve neither the modes nor the
> > > > > > > > > > connection status.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > > > >
> > > > > > > > > From your description sounds like an OK approach.
> > > > > > > > > But this is not something I feel too familiar with.
> > > > > > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > > > > > > >
> > > > > > > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > > > > > > too.
> > > > > > >
> > > > > > > Makes sense, and at least pre-coffee me can't immediately think of a
> > > > > > > scenario where we're going to regret this. _unknown status is pretty much
> > > > > > > limited to old VGA and similar things where load detect somehow isn't well
> > > > > > > supported by the hw.
> > > > > > >
> > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > >
> > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c       | 3 ++-
> > > > > > > > > >  include/drm/drm_modeset_helper_vtables.h | 8 +++++++-
> > > > > > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > > > > > > > >   if (count == 0 && connector->status == connector_status_connected)
> > > > > > > > > >           count = drm_add_override_edid_modes(connector);
> > > > > > > > > >
> > > > > > > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > > > > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > > > > > > > +                    connector->status == connector_status_unknown))
> > > > > > > > > >           count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > > > > > >   count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > > > > > >   if (count == 0)
> > > > > > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > > > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > > > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > > > > > > >    * The usual way to implement this is to cache the EDID retrieved in the
> > > > > > > > > >    * probe callback somewhere in the driver-private connector structure.
> > > > > > > > > >    * In this function drivers then parse the modes in the EDID and add
> > > > > > > > > > -  * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > > > > > > > +  * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > > > > > > >    * fixed panel can also manually add specific modes using
> > > > > > > > > >    * drm_mode_probed_add(). Drivers which manually add modes should also
> > > > > > > > > >    * make sure that the &drm_connector.display_info,
> > > > > > > > > >    * &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > > > > > > >    * filled in.
> > > > > > > > > >    *
> > > > > > > > > > +  * Note that the caller function will automatically add standard VESA
> > > > > > > > > > +  * DMT modes up to 1024x768 if the .get_modes() helper operation returns
> > > > > > > > > > +  * no mode and if the connector status is connector_status_connected or
> > > > > > > > > > +  * connector_status_unknown. There is no need to call
> > > > > > > > > > +  * drm_add_edid_modes() manually in that case.
> > > > > > >
> > > > > > > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > > > > > > ... Personally I'd just leave out the last sentence, I think that only
> > > > > > > confuses readers. Or I'm not grasphing what you're trying to tell here.
> > > > > >
> > > > > > IIRC, some drivers used and desktop environments expected unknown
> > > > > > rather than off for LVDS/eDP panels when the lid was shut or if the
> > > > > > mux was switched to another device in the case of hybrid laptops.
> > > > >
> > > > > We seem to have totally ditched that in
> > > > >
> > > > > commit 05c72e77ccda89ff624108b1b59a0fc43843f343
> > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Date:   Tue Jul 17 20:42:14 2018 +0300
> > > > >
> > > > >     drm/i915: Nuke the LVDS lid notifier
> > > > >
> > > > > No screaming yet.
> > > > >
> > > > > But I'm also a bit confused, for a panel there's generally an edid
> > > > > around, or a fixed (list of) modes. That's enough to stop this
> > > > > fallback from running, so should be all fine.
> > > >
> > > > No, you are right; you will have the EDID so this shouldn't be an
> > > > issue.  I was mis-remembering the original issue.  We originally
> > > > always reported connected for LVDS in radeon if the panel was present,
> > > > but then we got flack because some userspace expected unknown in
> > > > certain cases (e.g., lid or muxed displays).  Either way the EDID info
> > > > is still there.
> > >
> > > Yeah I think i915 started that habit, but I guess people realized it's
> > > unreliable enough that they should have their own lid handler in the
> > > desktop enviromnent doing whatever they want to do on lid close.
> > >
> > > Should we perhaps document that somewhere, that panels are always marked
> > > as connected? Not even sure where to put that in the docs ...
> > >
> > > Maybe adding a few of the usual suspects from the compositor side, Simon,
> > > Pekka?
> >
> > Actually adding Simon and Pekka this time around ...
>
> I don't know what anyone else does, but Weston (is not a DE) does not
> look at any lid switch, and assumes that if connector status is not
> DRM_MODE_CONNECTED, then it is disconnected. So, if a driver switched
> to "Unknown" status, it would be taken as disconnected.

Maybe an aside, but the guideline is for autoconfiguration:
- Light up everything that has connector status connected.
- If nothing has that status, try to light up the connectors with
status "unknown".

This is only really relevant on older platforms, mostly for VGA and
somewhat for dvi outputs.

Maybe another thing we should put down somewhere in the uapi docs ...
-Daniel


>
> I never knew what "Unknown" was relevant for. In weston.ini you can
> also force a connector on, so users could drive it regardless.
>
> However, I would also say that Weston is not supposed to react to any
> lid action exactly because it does not watch the lid switch. Personally
> I would expect a built-in screen to stay on even if lid closed.
>
> Panels that are always connected showing up as always connected, sounds
> good to me.
>
>
> Thanks,
> pq
Pekka Paalanen June 26, 2020, 8:59 a.m. UTC | #12
On Thu, 25 Jun 2020 12:44:36 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 25, 2020 at 12:32 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Thu, 25 Jun 2020 09:57:44 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >  
> > > On Thu, Jun 25, 2020 at 9:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:  
> > > >
> > > > On Wed, Jun 24, 2020 at 03:40:42PM -0400, Alex Deucher wrote:  

...

> > > > > No, you are right; you will have the EDID so this shouldn't be an
> > > > > issue.  I was mis-remembering the original issue.  We originally
> > > > > always reported connected for LVDS in radeon if the panel was present,
> > > > > but then we got flack because some userspace expected unknown in
> > > > > certain cases (e.g., lid or muxed displays).  Either way the EDID info
> > > > > is still there.  
> > > >
> > > > Yeah I think i915 started that habit, but I guess people realized it's
> > > > unreliable enough that they should have their own lid handler in the
> > > > desktop enviromnent doing whatever they want to do on lid close.
> > > >
> > > > Should we perhaps document that somewhere, that panels are always marked
> > > > as connected? Not even sure where to put that in the docs ...
> > > >
> > > > Maybe adding a few of the usual suspects from the compositor side, Simon,
> > > > Pekka?  
> > >
> > > Actually adding Simon and Pekka this time around ...  
> >
> > I don't know what anyone else does, but Weston (is not a DE) does not
> > look at any lid switch, and assumes that if connector status is not
> > DRM_MODE_CONNECTED, then it is disconnected. So, if a driver switched
> > to "Unknown" status, it would be taken as disconnected.  
> 
> Maybe an aside, but the guideline is for autoconfiguration:
> - Light up everything that has connector status connected.
> - If nothing has that status, try to light up the connectors with
> status "unknown".
> 
> This is only really relevant on older platforms, mostly for VGA and
> somewhat for dvi outputs.
> 
> Maybe another thing we should put down somewhere in the uapi docs ...

As I had no idea what "unknown" means or when it can happen, I assumed
that it must mean "the hardware cannot know". If the hardware cannot
know, then I certainly will not be trying to enable that, unless
explicitly configured to do so. Having a phantom output is worse than
having a real output that does not light up, because it's not obvious at
first with phantom output that anything is wrong. You may just be
wondering where your windows disappear, or where did you mouse cursor
go, or why you see a wallpaper but no login dialog, etc.

I certainly do hope no-one uses this quirk of Weston to get their lid
do what they want... it's possible, OTOH the desktop user base of
Weston according to what I've heard is around one person. It's not me.


Thanks,
pq
Daniel Stone June 26, 2020, 9:25 a.m. UTC | #13
Hi,

On Fri, 26 Jun 2020 at 10:00, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Thu, 25 Jun 2020 12:44:36 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
> > Maybe an aside, but the guideline is for autoconfiguration:
> > - Light up everything that has connector status connected.
> > - If nothing has that status, try to light up the connectors with
> > status "unknown".
> >
> > This is only really relevant on older platforms, mostly for VGA and
> > somewhat for dvi outputs.
> >
> > Maybe another thing we should put down somewhere in the uapi docs ...
>
> As I had no idea what "unknown" means or when it can happen, I assumed
> that it must mean "the hardware cannot know". If the hardware cannot
> know, then I certainly will not be trying to enable that, unless
> explicitly configured to do so. Having a phantom output is worse than
> having a real output that does not light up, because it's not obvious at
> first with phantom output that anything is wrong. You may just be
> wondering where your windows disappear, or where did you mouse cursor
> go, or why you see a wallpaper but no login dialog, etc.

How about a refinement of Dan's suggestion, proceeding down this
logical order and breaking if true:
- ignore all disconnected outputs
- if any outputs are connected, ignore all unknown outputs
- if only one output is unknown, use only that output (with default
mode if need be)
- if any outputs are unknown but have EDID present, use only those outputs
- at this point, we have multiple unknown outputs with no EDID - break
and demand explicit user configuration

Cheers,
Daniel
Daniel Vetter June 26, 2020, 1:35 p.m. UTC | #14
On Fri, Jun 26, 2020 at 10:25:45AM +0100, Daniel Stone wrote:
> Hi,
> 
> On Fri, 26 Jun 2020 at 10:00, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Thu, 25 Jun 2020 12:44:36 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
> > > Maybe an aside, but the guideline is for autoconfiguration:
> > > - Light up everything that has connector status connected.
> > > - If nothing has that status, try to light up the connectors with
> > > status "unknown".
> > >
> > > This is only really relevant on older platforms, mostly for VGA and
> > > somewhat for dvi outputs.
> > >
> > > Maybe another thing we should put down somewhere in the uapi docs ...
> >
> > As I had no idea what "unknown" means or when it can happen, I assumed
> > that it must mean "the hardware cannot know". If the hardware cannot
> > know, then I certainly will not be trying to enable that, unless
> > explicitly configured to do so. Having a phantom output is worse than
> > having a real output that does not light up, because it's not obvious at
> > first with phantom output that anything is wrong. You may just be
> > wondering where your windows disappear, or where did you mouse cursor
> > go, or why you see a wallpaper but no login dialog, etc.
> 
> How about a refinement of Dan's suggestion, proceeding down this
> logical order and breaking if true:
> - ignore all disconnected outputs
> - if any outputs are connected, ignore all unknown outputs
> - if only one output is unknown, use only that output (with default
> mode if need be)
> - if any outputs are unknown but have EDID present, use only those outputs
> - at this point, we have multiple unknown outputs with no EDID - break
> and demand explicit user configuration

EDID present generally means the status will be "connected". So not much
of a refined.

I'd say if you have multiple unknown, use a cloned config to avoid the
"windows are disappearing" problem. Which is also what fbcon does, and
iirc also -modesetting by default.

But the most important part is to not light up "unknown" outputs if
there's another output with a solid "connected". That avoids the problems
Pekka points out, phantom outputs are bad. Really no need to refine beyond
that, since imo that's a kernel bug.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index f5d141e0400f..9055d9573c90 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -491,7 +491,8 @@  int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_override_edid_modes(connector);
 
-	if (count == 0 && connector->status == connector_status_connected)
+	if (count == 0 && (connector->status == connector_status_connected ||
+			   connector->status == connector_status_unknown))
 		count = drm_add_modes_noedid(connector, 1024, 768);
 	count += drm_helper_probe_add_cmdline_mode(connector);
 	if (count == 0)
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 421a30f08463..afe55e2e93d2 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -876,13 +876,19 @@  struct drm_connector_helper_funcs {
 	 * The usual way to implement this is to cache the EDID retrieved in the
 	 * probe callback somewhere in the driver-private connector structure.
 	 * In this function drivers then parse the modes in the EDID and add
-	 * them by calling drm_add_edid_modes(). But connectors that driver a
+	 * them by calling drm_add_edid_modes(). But connectors that drive a
 	 * fixed panel can also manually add specific modes using
 	 * drm_mode_probed_add(). Drivers which manually add modes should also
 	 * make sure that the &drm_connector.display_info,
 	 * &drm_connector.width_mm and &drm_connector.height_mm fields are
 	 * filled in.
 	 *
+	 * Note that the caller function will automatically add standard VESA
+	 * DMT modes up to 1024x768 if the .get_modes() helper operation returns
+	 * no mode and if the connector status is connector_status_connected or
+	 * connector_status_unknown. There is no need to call
+	 * drm_add_edid_modes() manually in that case.
+	 *
 	 * Virtual drivers that just want some standard VESA mode with a given
 	 * resolution can call drm_add_modes_noedid(), and mark the preferred
 	 * one using drm_set_preferred_mode().