[PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Maxime Ripard]

Hi,

The proprietary stack for the RaspberryPi allows for a number of video
parameters widely used by their users, but yet don't have any equivalents
in the mainline kernel.

Those options are detailed here:
https://www.raspberrypi.org/documentation/configuration/config-txt/video.md

While not all of them are desirable to have in the mainline kernel, some of
them still have value, such as properties to initialise the overscan or
rotation parameters, or the one to deal with broken displays.

This series is an attempt to support those, and is based on a rewrite of
the command line parser I did a couple of years ago that never reached
upstream (due to a lack of time on my side). While this parser was
initially made to deal with named modes (in order to support TV modes), it
also allowed to extend it more easily, which is why it's resurrected.

Since a change of the command line parser can pretty easily get things
wrong and introduce regressions, I also worked with a number of unit tests
that you can find here: http://code.bulix.org/tpo7dg-607264?raw

Eventually, I guess those tests should be part of the kernel somewhere, but
I haven't found a suitable place for them to be included yet.

Let me know what you think,
Maxime

Maxime Ripard (7):
  drm/vc4: hdmi: Check that the monitor supports HDMI audio
  drm/edid: Allow to ignore the audio EDID data
  drm/edid: Allow to ignore the HDMI monitor mode
  drm/modes: Rewrite the command line parser
  drm/modes: Support modes names on the command line
  drm/modes: Allow to specify rotation and reflection on the commandline
  drm/modes: Parse overscan properties

 drivers/gpu/drm/drm_connector.c |   3 +-
 drivers/gpu/drm/drm_edid.c      |  16 +-
 drivers/gpu/drm/drm_fb_helper.c |  55 ++++-
 drivers/gpu/drm/drm_modes.c     | 441 ++++++++++++++++++++++++---------
 drivers/gpu/drm/vc4/vc4_hdmi.c  |   6 +-
 include/drm/drm_connector.h     |   3 +-
 6 files changed, 408 insertions(+), 116 deletions(-)

base-commit: e179d8e074e05a913a0915ae3c4b82f1434d9f4e
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/7] drm/vc4: hdmi: Check that the monitor supports HDMI audio

[Maxime Ripard]

The current code assumes as soon as the device is an HDMI one that it
supports an audio sink. However, strictly speaking, this is exposed as a
separate part of EDID.

This can be checked through the drm_detect_monitor_audio function, so let's
use it and make sure that we can use the HDMI monitor as an output before
sending sound.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 88fd5df7e7dc..a1bdc065c47c 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -109,6 +109,7 @@ struct vc4_hdmi_encoder {
 	struct vc4_encoder base;
 	bool hdmi_monitor;
 	bool limited_rgb_range;
+	bool monitor_has_audio;
 };
 
 static inline struct vc4_hdmi_encoder *
@@ -278,6 +279,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 		return -ENODEV;
 
 	vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid);
+	vc4_encoder->monitor_has_audio = drm_detect_monitor_audio(edid);
 
 	drm_connector_update_edid_property(connector, edid);
 	ret = drm_add_edid_modes(connector, edid);
@@ -785,9 +787,13 @@ static int vc4_hdmi_audio_startup(struct snd_pcm_substream *substream,
 {
 	struct vc4_hdmi *hdmi = dai_to_hdmi(dai);
 	struct drm_encoder *encoder = hdmi->encoder;
+	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	struct vc4_dev *vc4 = to_vc4_dev(encoder->dev);
 	int ret;
 
+	if (!vc4_encoder->monitor_has_audio)
+		return -ENODEV;
+
 	if (hdmi->audio.substream && hdmi->audio.substream != substream)
 		return -EINVAL;
 
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/vc4: hdmi: Check that the monitor supports HDMI audio

[Paul Kocialkowski]

Hi,

On Mon, 2019-03-04 at 15:52 +0100, Maxime Ripard wrote:
> The current code assumes as soon as the device is an HDMI one that it
> supports an audio sink. However, strictly speaking, this is exposed as a
> separate part of EDID.
> 
> This can be checked through the drm_detect_monitor_audio function, so let's
> use it and make sure that we can use the HDMI monitor as an output before
> sending sound.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 88fd5df7e7dc..a1bdc065c47c 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -109,6 +109,7 @@ struct vc4_hdmi_encoder {
>  	struct vc4_encoder base;
>  	bool hdmi_monitor;
>  	bool limited_rgb_range;
> +	bool monitor_has_audio;
>  };
>  
>  static inline struct vc4_hdmi_encoder *
> @@ -278,6 +279,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
>  		return -ENODEV;
>  
>  	vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid);
> +	vc4_encoder->monitor_has_audio = drm_detect_monitor_audio(edid);
>  
>  	drm_connector_update_edid_property(connector, edid);
>  	ret = drm_add_edid_modes(connector, edid);
> @@ -785,9 +787,13 @@ static int vc4_hdmi_audio_startup(struct snd_pcm_substream *substream,
>  {
>  	struct vc4_hdmi *hdmi = dai_to_hdmi(dai);
>  	struct drm_encoder *encoder = hdmi->encoder;
> +	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
>  	struct vc4_dev *vc4 = to_vc4_dev(encoder->dev);
>  	int ret;
>  
> +	if (!vc4_encoder->monitor_has_audio)
> +		return -ENODEV;
> +
>  	if (hdmi->audio.substream && hdmi->audio.substream != substream)
>  		return -EINVAL;
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/vc4: hdmi: Check that the monitor supports HDMI audio

[Stefan Wahren]

Hi Maxime,

Am 04.03.2019 um 15:52 schrieb Maxime Ripard:
> The current code assumes as soon as the device is an HDMI one that it
> supports an audio sink. However, strictly speaking, this is exposed as a
> separate part of EDID.
>
> This can be checked through the drm_detect_monitor_audio function, so let's
> use it and make sure that we can use the HDMI monitor as an output before
> sending sound.

does the audio output work in the following setup after applying this patch?

VC4 --- HDMI Audio extractor --- Non audio capable monitor

>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/vc4: hdmi: Check that the monitor supports HDMI audio

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 727 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

> Hi Maxime,
>
> Am 04.03.2019 um 15:52 schrieb Maxime Ripard:
>> The current code assumes as soon as the device is an HDMI one that it
>> supports an audio sink. However, strictly speaking, this is exposed as a
>> separate part of EDID.
>>
>> This can be checked through the drm_detect_monitor_audio function, so let's
>> use it and make sure that we can use the HDMI monitor as an output before
>> sending sound.
>
> does the audio output work in the following setup after applying this patch?
>
> VC4 --- HDMI Audio extractor --- Non audio capable monitor

A 1-minute google of audio extractors says they manage the EDID.  Do you
have some reason to think this wouldn't work?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/vc4: hdmi: Check that the monitor supports HDMI audio

[Stefan Wahren]

> Eric Anholt <eric@anholt.net> hat am 4. März 2019 um 19:28 geschrieben:
> 
> 
> Stefan Wahren <stefan.wahren@i2se.com> writes:
> 
> > Hi Maxime,
> >
> > Am 04.03.2019 um 15:52 schrieb Maxime Ripard:
> >> The current code assumes as soon as the device is an HDMI one that it
> >> supports an audio sink. However, strictly speaking, this is exposed as a
> >> separate part of EDID.
> >>
> >> This can be checked through the drm_detect_monitor_audio function, so let's
> >> use it and make sure that we can use the HDMI monitor as an output before
> >> sending sound.
> >
> > does the audio output work in the following setup after applying this patch?
> >
> > VC4 --- HDMI Audio extractor --- Non audio capable monitor
> 
> A 1-minute google of audio extractors says they manage the EDID.  Do you
> have some reason to think this wouldn't work?

My only concern is the existence of some audio extractors which doesn't care about EDID. I that case we need to provide some kind of force switch.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/vc4: hdmi: Check that the monitor supports HDMI audio

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1260 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

>> Eric Anholt <eric@anholt.net> hat am 4. März 2019 um 19:28 geschrieben:
>> 
>> 
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>> 
>> > Hi Maxime,
>> >
>> > Am 04.03.2019 um 15:52 schrieb Maxime Ripard:
>> >> The current code assumes as soon as the device is an HDMI one that it
>> >> supports an audio sink. However, strictly speaking, this is exposed as a
>> >> separate part of EDID.
>> >>
>> >> This can be checked through the drm_detect_monitor_audio function, so let's
>> >> use it and make sure that we can use the HDMI monitor as an output before
>> >> sending sound.
>> >
>> > does the audio output work in the following setup after applying this patch?
>> >
>> > VC4 --- HDMI Audio extractor --- Non audio capable monitor
>> 
>> A 1-minute google of audio extractors says they manage the EDID.  Do you
>> have some reason to think this wouldn't work?
>
> My only concern is the existence of some audio extractors which doesn't care about EDID. I that case we need to provide some kind of force switch.

I'm having a hard time imagining such a product existing, given that I
don't think Windows will have "ignore the EDID, force audio anyway"
switches in their UIs.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Maxime Ripard]

In some cases, in order to accomodate with displays with poor EDIDs, we
need to ignore that the monitor alledgedly supports audio output and
disable the audio output.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_edid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 990b1909f9d7..c0258b011bb2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
 }
 EXPORT_SYMBOL(drm_detect_hdmi_monitor);
 
+static bool ignore_edid_audio = false;
+module_param(ignore_edid_audio, bool, 0644);
+MODULE_PARM_DESC(ignore_edid_audio,
+		 "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
+
 /**
  * drm_detect_monitor_audio - check monitor audio capability
  * @edid: EDID block to scan
@@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
 	bool has_audio = false;
 	int start_offset, end_offset;
 
+	if (ignore_edid_audio)
+		goto end;
+
 	edid_ext = drm_find_cea_extension(edid);
 	if (!edid_ext)
 		goto end;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Jani Nikula]

On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> In some cases, in order to accomodate with displays with poor EDIDs, we
> need to ignore that the monitor alledgedly supports audio output and
> disable the audio output.

*sad trombone*

Trying to figure this out automatically in kernel is better than a
quirk.

A quirk is better than requiring the user to provide an override EDID
via the firmware loader (drm.edid_firmware parameter).

Requiring an override EDID is better than adding a module parameter.

I'd much rather we exhausted the other options before adding module
parameters to address specific issues with EDIDs. That's a rabbit hole
with no end.


BR,
Jani.


>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 990b1909f9d7..c0258b011bb2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>  }
>  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>  
> +static bool ignore_edid_audio = false;
> +module_param(ignore_edid_audio, bool, 0644);
> +MODULE_PARM_DESC(ignore_edid_audio,
> +		 "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> +
>  /**
>   * drm_detect_monitor_audio - check monitor audio capability
>   * @edid: EDID block to scan
> @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>  	bool has_audio = false;
>  	int start_offset, end_offset;
>  
> +	if (ignore_edid_audio)
> +		goto end;
> +
>  	edid_ext = drm_find_cea_extension(edid);
>  	if (!edid_ext)
>  		goto end;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Adam Jackson]

On Mon, 2019-03-04 at 17:47 +0200, Jani Nikula wrote:
> On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > In some cases, in order to accomodate with displays with poor EDIDs, we
> > need to ignore that the monitor alledgedly supports audio output and
> > disable the audio output.
> 
> *sad trombone*
> 
> Trying to figure this out automatically in kernel is better than a
> quirk.
> 
> A quirk is better than requiring the user to provide an override EDID
> via the firmware loader (drm.edid_firmware parameter).
> 
> Requiring an override EDID is better than adding a module parameter.

Also, this and 3/ apply to _every_ monitor attached to the system. No.

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1733 bytes --]

On Mon, Mar 04, 2019 at 05:47:09PM +0200, Jani Nikula wrote:
> On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > In some cases, in order to accomodate with displays with poor EDIDs, we
> > need to ignore that the monitor alledgedly supports audio output and
> > disable the audio output.
> 
> *sad trombone*
> 
> Trying to figure this out automatically in kernel is better than a
> quirk.
> 
> A quirk is better than requiring the user to provide an override EDID
> via the firmware loader (drm.edid_firmware parameter).
> 
> Requiring an override EDID is better than adding a module parameter.
> 
> I'd much rather we exhausted the other options before adding module
> parameters to address specific issues with EDIDs. That's a rabbit hole
> with no end.

We should also consider the usability of these solutions.

Sure, the quirks are the ideal solution long term, but do we really
expect the average user that just got its device from Amazon and
connected it to its display to figure out:

 - That if it's display doesn't work, it's because the display is
   broken
 - That it is broken due to poor EDIDs
 - To find out that it's supposed to be handled in DRM through a quirk
 - How to make such a quirk
 - How to recompile the kernel on its distro of choice
 - That they need to send a patch later on to upstream Linux, and then
   wait for a year or so (depending on their distro) before it's
   actually working.

Chances are that they would stop at 1, call the device trash and never
submit any quirk, therefore making the quirk approach useless in the
process.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Jani Nikula]

On Tue, 05 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Mon, Mar 04, 2019 at 05:47:09PM +0200, Jani Nikula wrote:
>> On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>> > In some cases, in order to accomodate with displays with poor EDIDs, we
>> > need to ignore that the monitor alledgedly supports audio output and
>> > disable the audio output.
>> 
>> *sad trombone*
>> 
>> Trying to figure this out automatically in kernel is better than a
>> quirk.
>> 
>> A quirk is better than requiring the user to provide an override EDID
>> via the firmware loader (drm.edid_firmware parameter).
>> 
>> Requiring an override EDID is better than adding a module parameter.
>> 
>> I'd much rather we exhausted the other options before adding module
>> parameters to address specific issues with EDIDs. That's a rabbit hole
>> with no end.
>
> We should also consider the usability of these solutions.
>
> Sure, the quirks are the ideal solution long term, but do we really
> expect the average user that just got its device from Amazon and
> connected it to its display to figure out:
>
>  - That if it's display doesn't work, it's because the display is
>    broken
>  - That it is broken due to poor EDIDs
>  - To find out that it's supposed to be handled in DRM through a quirk
>  - How to make such a quirk
>  - How to recompile the kernel on its distro of choice
>  - That they need to send a patch later on to upstream Linux, and then
>    wait for a year or so (depending on their distro) before it's
>    actually working.
>
> Chances are that they would stop at 1, call the device trash and never
> submit any quirk, therefore making the quirk approach useless in the
> process.

It only takes one user to reach the end of the list, and have the quirk
figured out and backported to stable kernels, fixing it for the rest of
the average users.

Adding this to drm core means *I* will also have to care about this for
i915 users that find ways to abuse this for whatever reason. And they
*will* find ways, because hey, someone on some forum wrote that this
fixed their issue on some random other machine.

We've added too many driver specific debug knobs as module parameters
over the years, and a good portion of the bug reports we get about
basically anything come with a combination of random module parameters,
some of which have ceased to exist years ago. People just copy-paste
them from forums and wikis.

Looking at [1], I'm sure it didn't start out that massive either. It was
probably a knob or two, as a quick fix for the problem at hand, and then
it was easy to add more.

What looks like the easy route now really isn't.


BR,
Jani.


[1] https://www.raspberrypi.org/documentation/configuration/config-txt/video.md

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Adam Jackson]

On Tue, 2019-03-05 at 09:08 +0100, Maxime Ripard wrote:

> Chances are that they would stop at 1, call the device trash and never
> submit any quirk, therefore making the quirk approach useless in the
> process.

But the device _is_ trash. It's not like writing an accurate EDID is
difficult, if you're the manufacturer. Why enable hardware vendors to
be incompetent?

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Ville Syrjälä]

On Mon, Mar 04, 2019 at 03:52:35PM +0100, Maxime Ripard wrote:
> In some cases, in order to accomodate with displays with poor EDIDs, we
> need to ignore that the monitor alledgedly supports audio output and
> disable the audio output.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 990b1909f9d7..c0258b011bb2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>  }
>  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>  
> +static bool ignore_edid_audio = false;
> +module_param(ignore_edid_audio, bool, 0644);
> +MODULE_PARM_DESC(ignore_edid_audio,
> +		 "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> +

I would suggest that this is not the best apporach. Years of experience
from i915 says that more modparams means random forums full of people
trading  cargo culted settings. And as soon as the average user comes
across the magic incantation that works they are likely to not file the
appropriate bug report. Also years later we still see people using
modparams that stopped working five hardware generations ago. So at
least for i915 new modparams are generally frowned upon.

Bad EDIDs at least should be quirked. Which means we really need the
bug reports, and hence a modparam can be somewhat counter productive.

For allowing the user to force the DVI vs. HDMI and audio vs. not i915
does have the "audio" connector property. Other drivers could adopt the
same thing. Though I'm not sure even i915 should be exposing this for
the reasons already mentioned. There is one hardware generation where
it can actually be useful on i915 as the hardware is only capably of
sending infoframes/audio to a single HDMI port at a time. So with this
property the user can at least select which display gets to do those
things.

I do agree that there is an unfortnate problem with fbcon vs.
initial property values. I've sometimes pondered about exposing
kms properties in a generic fashion via sysfs and/or kernel
cmdline somehow. IIRC devicetree/something similar has also been
proposed occasionally to solve this problem.

>  /**
>   * drm_detect_monitor_audio - check monitor audio capability
>   * @edid: EDID block to scan
> @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>  	bool has_audio = false;
>  	int start_offset, end_offset;
>  
> +	if (ignore_edid_audio)
> +		goto end;
> +
>  	edid_ext = drm_find_cea_extension(edid);
>  	if (!edid_ext)
>  		goto end;
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1618 bytes --]

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> In some cases, in order to accomodate with displays with poor EDIDs, we
> need to ignore that the monitor alledgedly supports audio output and
> disable the audio output.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 990b1909f9d7..c0258b011bb2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>  }
>  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>  
> +static bool ignore_edid_audio = false;
> +module_param(ignore_edid_audio, bool, 0644);
> +MODULE_PARM_DESC(ignore_edid_audio,
> +		 "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> +
>  /**
>   * drm_detect_monitor_audio - check monitor audio capability
>   * @edid: EDID block to scan
> @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>  	bool has_audio = false;
>  	int start_offset, end_offset;
>  
> +	if (ignore_edid_audio)
> +		goto end;
> +
>  	edid_ext = drm_find_cea_extension(edid);
>  	if (!edid_ext)
>  		goto end;

It looks like the motivation for the original flag on Raspberry Pi was
"I've got a non-audio monitor, but the system comes up trying to play
audio to HDMI instead of the analog jack".  Do we have some way for DRM
to communicate to ALSA that this is not the right place to try to play
audio by default?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Alex Deucher]

On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
>
> Maxime Ripard <maxime.ripard@bootlin.com> writes:
>
> > In some cases, in order to accomodate with displays with poor EDIDs, we
> > need to ignore that the monitor alledgedly supports audio output and
> > disable the audio output.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 990b1909f9d7..c0258b011bb2 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> >  }
> >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> >
> > +static bool ignore_edid_audio = false;
> > +module_param(ignore_edid_audio, bool, 0644);
> > +MODULE_PARM_DESC(ignore_edid_audio,
> > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > +
> >  /**
> >   * drm_detect_monitor_audio - check monitor audio capability
> >   * @edid: EDID block to scan
> > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> >       bool has_audio = false;
> >       int start_offset, end_offset;
> >
> > +     if (ignore_edid_audio)
> > +             goto end;
> > +
> >       edid_ext = drm_find_cea_extension(edid);
> >       if (!edid_ext)
> >               goto end;
>
> It looks like the motivation for the original flag on Raspberry Pi was
> "I've got a non-audio monitor, but the system comes up trying to play
> audio to HDMI instead of the analog jack".  Do we have some way for DRM
> to communicate to ALSA that this is not the right place to try to play
> audio by default?

Apparently not.  We have users using debug knobs in our drivers to
disable display audio because ALSA defaults to that rather than other
audio.

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2437 bytes --]

On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> >
> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> >
> > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > need to ignore that the monitor alledgedly supports audio output and
> > > disable the audio output.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 990b1909f9d7..c0258b011bb2 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > >  }
> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > >
> > > +static bool ignore_edid_audio = false;
> > > +module_param(ignore_edid_audio, bool, 0644);
> > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > +
> > >  /**
> > >   * drm_detect_monitor_audio - check monitor audio capability
> > >   * @edid: EDID block to scan
> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > >       bool has_audio = false;
> > >       int start_offset, end_offset;
> > >
> > > +     if (ignore_edid_audio)
> > > +             goto end;
> > > +
> > >       edid_ext = drm_find_cea_extension(edid);
> > >       if (!edid_ext)
> > >               goto end;
> >
> > It looks like the motivation for the original flag on Raspberry Pi was
> > "I've got a non-audio monitor, but the system comes up trying to play
> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > to communicate to ALSA that this is not the right place to try to play
> > audio by default?
> 
> Apparently not.  We have users using debug knobs in our drivers to
> disable display audio because ALSA defaults to that rather than other
> audio.

I guess one way to do this would be to register the card only when an
audio-capable monitor is connected instead of doing this at probe
time. I'm not sure how convenient it is for userspace though.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Ville Syrjälä]

On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote:
> On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> > >
> > > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> > >
> > > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > > need to ignore that the monitor alledgedly supports audio output and
> > > > disable the audio output.
> > > >
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index 990b1909f9d7..c0258b011bb2 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > > >
> > > > +static bool ignore_edid_audio = false;
> > > > +module_param(ignore_edid_audio, bool, 0644);
> > > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > > +
> > > >  /**
> > > >   * drm_detect_monitor_audio - check monitor audio capability
> > > >   * @edid: EDID block to scan
> > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > > >       bool has_audio = false;
> > > >       int start_offset, end_offset;
> > > >
> > > > +     if (ignore_edid_audio)
> > > > +             goto end;
> > > > +
> > > >       edid_ext = drm_find_cea_extension(edid);
> > > >       if (!edid_ext)
> > > >               goto end;
> > >
> > > It looks like the motivation for the original flag on Raspberry Pi was
> > > "I've got a non-audio monitor, but the system comes up trying to play
> > > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > > to communicate to ALSA that this is not the right place to try to play
> > > audio by default?
> > 
> > Apparently not.  We have users using debug knobs in our drivers to
> > disable display audio because ALSA defaults to that rather than other
> > audio.
> 
> I guess one way to do this would be to register the card only when an
> audio-capable monitor is connected instead of doing this at probe
> time. I'm not sure how convenient it is for userspace though.

We already provide the ELD to alsa. I'm pretty sure pulseaudio uses
that stuff somehow to figure out whether to play audio over HDMI.
But since I don't use pulseaudio myself I can't be 100% sure.

Cc:ing Takashi/alsa folks for confirmation.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Ville Syrjälä]

On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote:
> > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> > > >
> > > > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> > > >
> > > > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > > > need to ignore that the monitor alledgedly supports audio output and
> > > > > disable the audio output.
> > > > >
> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 990b1909f9d7..c0258b011bb2 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > > > >
> > > > > +static bool ignore_edid_audio = false;
> > > > > +module_param(ignore_edid_audio, bool, 0644);
> > > > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > > > +
> > > > >  /**
> > > > >   * drm_detect_monitor_audio - check monitor audio capability
> > > > >   * @edid: EDID block to scan
> > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > > > >       bool has_audio = false;
> > > > >       int start_offset, end_offset;
> > > > >
> > > > > +     if (ignore_edid_audio)
> > > > > +             goto end;
> > > > > +
> > > > >       edid_ext = drm_find_cea_extension(edid);
> > > > >       if (!edid_ext)
> > > > >               goto end;
> > > >
> > > > It looks like the motivation for the original flag on Raspberry Pi was
> > > > "I've got a non-audio monitor, but the system comes up trying to play
> > > > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > > > to communicate to ALSA that this is not the right place to try to play
> > > > audio by default?
> > > 
> > > Apparently not.  We have users using debug knobs in our drivers to
> > > disable display audio because ALSA defaults to that rather than other
> > > audio.
> > 
> > I guess one way to do this would be to register the card only when an
> > audio-capable monitor is connected instead of doing this at probe
> > time. I'm not sure how convenient it is for userspace though.
> 
> We already provide the ELD to alsa. I'm pretty sure pulseaudio uses
> that stuff somehow to figure out whether to play audio over HDMI.
> But since I don't use pulseaudio myself I can't be 100% sure.
> 
> Cc:ing Takashi/alsa folks for confirmation.

I forgot that the .pin_eld_notify() stuff is i915 specific. But
I see some kind of hdmi_codec_ops thing used by some other drivers.
I guess that is supposed to achieve the same thing more or less?
I'm not immediately seeing any kind of drm->alsa notification
hook in there though.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Alex Deucher]

On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote:
> > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> > > > >
> > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> > > > >
> > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > > > > need to ignore that the monitor alledgedly supports audio output and
> > > > > > disable the audio output.
> > > > > >
> > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > > index 990b1909f9d7..c0258b011bb2 100644
> > > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > > > > >
> > > > > > +static bool ignore_edid_audio = false;
> > > > > > +module_param(ignore_edid_audio, bool, 0644);
> > > > > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > > > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > > > > +
> > > > > >  /**
> > > > > >   * drm_detect_monitor_audio - check monitor audio capability
> > > > > >   * @edid: EDID block to scan
> > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > > > > >       bool has_audio = false;
> > > > > >       int start_offset, end_offset;
> > > > > >
> > > > > > +     if (ignore_edid_audio)
> > > > > > +             goto end;
> > > > > > +
> > > > > >       edid_ext = drm_find_cea_extension(edid);
> > > > > >       if (!edid_ext)
> > > > > >               goto end;
> > > > >
> > > > > It looks like the motivation for the original flag on Raspberry Pi was
> > > > > "I've got a non-audio monitor, but the system comes up trying to play
> > > > > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > > > > to communicate to ALSA that this is not the right place to try to play
> > > > > audio by default?
> > > >
> > > > Apparently not.  We have users using debug knobs in our drivers to
> > > > disable display audio because ALSA defaults to that rather than other
> > > > audio.
> > >
> > > I guess one way to do this would be to register the card only when an
> > > audio-capable monitor is connected instead of doing this at probe
> > > time. I'm not sure how convenient it is for userspace though.
> >
> > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses
> > that stuff somehow to figure out whether to play audio over HDMI.
> > But since I don't use pulseaudio myself I can't be 100% sure.
> >
> > Cc:ing Takashi/alsa folks for confirmation.
>
> I forgot that the .pin_eld_notify() stuff is i915 specific. But
> I see some kind of hdmi_codec_ops thing used by some other drivers.
> I guess that is supposed to achieve the same thing more or less?
> I'm not immediately seeing any kind of drm->alsa notification
> hook in there though.

On AMD hw, the GPU has backdoor access to some of the audio state, so
when stuff happens on the GPU side, it's reflected on the audio side
automatically.

Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Ville Syrjälä]

On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote:
> On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote:
> > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> > > > > >
> > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> > > > > >
> > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > > > > > need to ignore that the monitor alledgedly supports audio output and
> > > > > > > disable the audio output.
> > > > > > >
> > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > > > > > >  1 file changed, 8 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > > > index 990b1909f9d7..c0258b011bb2 100644
> > > > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > > > > > >
> > > > > > > +static bool ignore_edid_audio = false;
> > > > > > > +module_param(ignore_edid_audio, bool, 0644);
> > > > > > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > > > > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * drm_detect_monitor_audio - check monitor audio capability
> > > > > > >   * @edid: EDID block to scan
> > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > > > > > >       bool has_audio = false;
> > > > > > >       int start_offset, end_offset;
> > > > > > >
> > > > > > > +     if (ignore_edid_audio)
> > > > > > > +             goto end;
> > > > > > > +
> > > > > > >       edid_ext = drm_find_cea_extension(edid);
> > > > > > >       if (!edid_ext)
> > > > > > >               goto end;
> > > > > >
> > > > > > It looks like the motivation for the original flag on Raspberry Pi was
> > > > > > "I've got a non-audio monitor, but the system comes up trying to play
> > > > > > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > > > > > to communicate to ALSA that this is not the right place to try to play
> > > > > > audio by default?
> > > > >
> > > > > Apparently not.  We have users using debug knobs in our drivers to
> > > > > disable display audio because ALSA defaults to that rather than other
> > > > > audio.
> > > >
> > > > I guess one way to do this would be to register the card only when an
> > > > audio-capable monitor is connected instead of doing this at probe
> > > > time. I'm not sure how convenient it is for userspace though.
> > >
> > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses
> > > that stuff somehow to figure out whether to play audio over HDMI.
> > > But since I don't use pulseaudio myself I can't be 100% sure.
> > >
> > > Cc:ing Takashi/alsa folks for confirmation.
> >
> > I forgot that the .pin_eld_notify() stuff is i915 specific. But
> > I see some kind of hdmi_codec_ops thing used by some other drivers.
> > I guess that is supposed to achieve the same thing more or less?
> > I'm not immediately seeing any kind of drm->alsa notification
> > hook in there though.
> 
> On AMD hw, the GPU has backdoor access to some of the audio state, so
> when stuff happens on the GPU side, it's reflected on the audio side
> automatically.

Right. i915 has a similar thing (my theory is that it's basically
an industry wide hardware workaround for inflexible Windows driver
architecture). But that was problematic for some power management
related reasons (IIRC) so we added a software mechanism for it.
Though I believe we still write the ELD into the hardware buffer
as well.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Takashi Iwai]

On Tue, 05 Mar 2019 20:36:38 +0100,
Ville Syrjälä wrote:
> 
> On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote:
> > On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote:
> > > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> > > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> > > > > > >
> > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> > > > > > >
> > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > > > > > > need to ignore that the monitor alledgedly supports audio output and
> > > > > > > > disable the audio output.
> > > > > > > >
> > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > > > > > > >  1 file changed, 8 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > > > > index 990b1909f9d7..c0258b011bb2 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > > > > > > >
> > > > > > > > +static bool ignore_edid_audio = false;
> > > > > > > > +module_param(ignore_edid_audio, bool, 0644);
> > > > > > > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > > > > > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > > > > > > +
> > > > > > > >  /**
> > > > > > > >   * drm_detect_monitor_audio - check monitor audio capability
> > > > > > > >   * @edid: EDID block to scan
> > > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > > > > > > >       bool has_audio = false;
> > > > > > > >       int start_offset, end_offset;
> > > > > > > >
> > > > > > > > +     if (ignore_edid_audio)
> > > > > > > > +             goto end;
> > > > > > > > +
> > > > > > > >       edid_ext = drm_find_cea_extension(edid);
> > > > > > > >       if (!edid_ext)
> > > > > > > >               goto end;
> > > > > > >
> > > > > > > It looks like the motivation for the original flag on Raspberry Pi was
> > > > > > > "I've got a non-audio monitor, but the system comes up trying to play
> > > > > > > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > > > > > > to communicate to ALSA that this is not the right place to try to play
> > > > > > > audio by default?
> > > > > >
> > > > > > Apparently not.  We have users using debug knobs in our drivers to
> > > > > > disable display audio because ALSA defaults to that rather than other
> > > > > > audio.
> > > > >
> > > > > I guess one way to do this would be to register the card only when an
> > > > > audio-capable monitor is connected instead of doing this at probe
> > > > > time. I'm not sure how convenient it is for userspace though.
> > > >
> > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses
> > > > that stuff somehow to figure out whether to play audio over HDMI.
> > > > But since I don't use pulseaudio myself I can't be 100% sure.
> > > >
> > > > Cc:ing Takashi/alsa folks for confirmation.
> > >
> > > I forgot that the .pin_eld_notify() stuff is i915 specific. But
> > > I see some kind of hdmi_codec_ops thing used by some other drivers.
> > > I guess that is supposed to achieve the same thing more or less?
> > > I'm not immediately seeing any kind of drm->alsa notification
> > > hook in there though.
> > 
> > On AMD hw, the GPU has backdoor access to some of the audio state, so
> > when stuff happens on the GPU side, it's reflected on the audio side
> > automatically.
> 
> Right. i915 has a similar thing (my theory is that it's basically
> an industry wide hardware workaround for inflexible Windows driver
> architecture). But that was problematic for some power management
> related reasons (IIRC) so we added a software mechanism for it.
> Though I believe we still write the ELD into the hardware buffer
> as well.

I'm late to the game as I've been off in the last week, and here is
just the confirmation.

The direct communication from drm to ALSA via component has been
implemented currently only for i915.  I had some patches to enable the
feature for radeon and amdgpu, but the enablement on amdgpu DC is
still missing, and the work is pending for now.

For other drivers, we'd need more or less similar mechanism.
We might want to choose a better infrastructure than the component
binding, but it's a thing to be discussed.


thanks,

Takashi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5728 bytes --]

Hi,

On Wed, Mar 13, 2019 at 11:44:17AM +0100, Takashi Iwai wrote:
> On Tue, 05 Mar 2019 20:36:38 +0100,
> Ville Syrjälä wrote:
> > 
> > On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote:
> > > On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote:
> > > > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> > > > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> > > > > > > >
> > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> > > > > > > >
> > > > > > > > > In some cases, in order to accomodate with displays with poor EDIDs, we
> > > > > > > > > need to ignore that the monitor alledgedly supports audio output and
> > > > > > > > > disable the audio output.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> > > > > > > > >  1 file changed, 8 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > > > > > index 990b1909f9d7..c0258b011bb2 100644
> > > > > > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > > > > > > > >
> > > > > > > > > +static bool ignore_edid_audio = false;
> > > > > > > > > +module_param(ignore_edid_audio, bool, 0644);
> > > > > > > > > +MODULE_PARM_DESC(ignore_edid_audio,
> > > > > > > > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> > > > > > > > > +
> > > > > > > > >  /**
> > > > > > > > >   * drm_detect_monitor_audio - check monitor audio capability
> > > > > > > > >   * @edid: EDID block to scan
> > > > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> > > > > > > > >       bool has_audio = false;
> > > > > > > > >       int start_offset, end_offset;
> > > > > > > > >
> > > > > > > > > +     if (ignore_edid_audio)
> > > > > > > > > +             goto end;
> > > > > > > > > +
> > > > > > > > >       edid_ext = drm_find_cea_extension(edid);
> > > > > > > > >       if (!edid_ext)
> > > > > > > > >               goto end;
> > > > > > > >
> > > > > > > > It looks like the motivation for the original flag on Raspberry Pi was
> > > > > > > > "I've got a non-audio monitor, but the system comes up trying to play
> > > > > > > > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> > > > > > > > to communicate to ALSA that this is not the right place to try to play
> > > > > > > > audio by default?
> > > > > > >
> > > > > > > Apparently not.  We have users using debug knobs in our drivers to
> > > > > > > disable display audio because ALSA defaults to that rather than other
> > > > > > > audio.
> > > > > >
> > > > > > I guess one way to do this would be to register the card only when an
> > > > > > audio-capable monitor is connected instead of doing this at probe
> > > > > > time. I'm not sure how convenient it is for userspace though.
> > > > >
> > > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses
> > > > > that stuff somehow to figure out whether to play audio over HDMI.
> > > > > But since I don't use pulseaudio myself I can't be 100% sure.
> > > > >
> > > > > Cc:ing Takashi/alsa folks for confirmation.
> > > >
> > > > I forgot that the .pin_eld_notify() stuff is i915 specific. But
> > > > I see some kind of hdmi_codec_ops thing used by some other drivers.
> > > > I guess that is supposed to achieve the same thing more or less?
> > > > I'm not immediately seeing any kind of drm->alsa notification
> > > > hook in there though.
> > > 
> > > On AMD hw, the GPU has backdoor access to some of the audio state, so
> > > when stuff happens on the GPU side, it's reflected on the audio side
> > > automatically.
> > 
> > Right. i915 has a similar thing (my theory is that it's basically
> > an industry wide hardware workaround for inflexible Windows driver
> > architecture). But that was problematic for some power management
> > related reasons (IIRC) so we added a software mechanism for it.
> > Though I believe we still write the ELD into the hardware buffer
> > as well.
> 
> I'm late to the game as I've been off in the last week, and here is
> just the confirmation.
> 
> The direct communication from drm to ALSA via component has been
> implemented currently only for i915.  I had some patches to enable the
> feature for radeon and amdgpu, but the enablement on amdgpu DC is
> still missing, and the work is pending for now.
> 
> For other drivers, we'd need more or less similar mechanism.
> We might want to choose a better infrastructure than the component
> binding, but it's a thing to be discussed.

I guess I would be interested in working on this.

I've looked at the ELD situation on vc4, and even though the control
is exposed for the vc4 driver, and that content seems to be ok,
pulseaudio doesn't pick it up.

My understanding is that pulseaudio waits on an ALSA event before
looking at the ELD control, and only does so for the device that are
considered HDMI, but I'm not quite sure what policy it has for that
yet...

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2725 bytes --]

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> [ Unknown signature status ]
> On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
>> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
>> >
>> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
>> >
>> > > In some cases, in order to accomodate with displays with poor EDIDs, we
>> > > need to ignore that the monitor alledgedly supports audio output and
>> > > disable the audio output.
>> > >
>> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> > > ---
>> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>> > >  1 file changed, 8 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > > index 990b1909f9d7..c0258b011bb2 100644
>> > > --- a/drivers/gpu/drm/drm_edid.c
>> > > +++ b/drivers/gpu/drm/drm_edid.c
>> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>> > >  }
>> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>> > >
>> > > +static bool ignore_edid_audio = false;
>> > > +module_param(ignore_edid_audio, bool, 0644);
>> > > +MODULE_PARM_DESC(ignore_edid_audio,
>> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
>> > > +
>> > >  /**
>> > >   * drm_detect_monitor_audio - check monitor audio capability
>> > >   * @edid: EDID block to scan
>> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>> > >       bool has_audio = false;
>> > >       int start_offset, end_offset;
>> > >
>> > > +     if (ignore_edid_audio)
>> > > +             goto end;
>> > > +
>> > >       edid_ext = drm_find_cea_extension(edid);
>> > >       if (!edid_ext)
>> > >               goto end;
>> >
>> > It looks like the motivation for the original flag on Raspberry Pi was
>> > "I've got a non-audio monitor, but the system comes up trying to play
>> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
>> > to communicate to ALSA that this is not the right place to try to play
>> > audio by default?
>> 
>> Apparently not.  We have users using debug knobs in our drivers to
>> disable display audio because ALSA defaults to that rather than other
>> audio.
>
> I guess one way to do this would be to register the card only when an
> audio-capable monitor is connected instead of doing this at probe
> time. I'm not sure how convenient it is for userspace though.

Yeah, I have no idea how this is supposed to work, but pulseaudio keeps
doing reasonable things on my intel desktop so I'm wondering if we're
just missing some bit of the HDMI driver communicating to ALSA about the
state of the audio sink.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Daniel Vetter]

On Tue, Mar 05, 2019 at 10:11:51AM -0800, Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> writes:
> 
> > [ Unknown signature status ]
> > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> >> >
> >> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> >> >
> >> > > In some cases, in order to accomodate with displays with poor EDIDs, we
> >> > > need to ignore that the monitor alledgedly supports audio output and
> >> > > disable the audio output.
> >> > >
> >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> > > ---
> >> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> >> > >  1 file changed, 8 insertions(+)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > > index 990b1909f9d7..c0258b011bb2 100644
> >> > > --- a/drivers/gpu/drm/drm_edid.c
> >> > > +++ b/drivers/gpu/drm/drm_edid.c
> >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> >> > >  }
> >> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> >> > >
> >> > > +static bool ignore_edid_audio = false;
> >> > > +module_param(ignore_edid_audio, bool, 0644);
> >> > > +MODULE_PARM_DESC(ignore_edid_audio,
> >> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> >> > > +
> >> > >  /**
> >> > >   * drm_detect_monitor_audio - check monitor audio capability
> >> > >   * @edid: EDID block to scan
> >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> >> > >       bool has_audio = false;
> >> > >       int start_offset, end_offset;
> >> > >
> >> > > +     if (ignore_edid_audio)
> >> > > +             goto end;
> >> > > +
> >> > >       edid_ext = drm_find_cea_extension(edid);
> >> > >       if (!edid_ext)
> >> > >               goto end;
> >> >
> >> > It looks like the motivation for the original flag on Raspberry Pi was
> >> > "I've got a non-audio monitor, but the system comes up trying to play
> >> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> >> > to communicate to ALSA that this is not the right place to try to play
> >> > audio by default?
> >> 
> >> Apparently not.  We have users using debug knobs in our drivers to
> >> disable display audio because ALSA defaults to that rather than other
> >> audio.
> >
> > I guess one way to do this would be to register the card only when an
> > audio-capable monitor is connected instead of doing this at probe
> > time. I'm not sure how convenient it is for userspace though.
> 
> Yeah, I have no idea how this is supposed to work, but pulseaudio keeps
> doing reasonable things on my intel desktop so I'm wondering if we're
> just missing some bit of the HDMI driver communicating to ALSA about the
> state of the audio sink.

We transport (either through the i915/snd-hda component or hw backdoors)
both the "can this sink do audio" and the current eld describing the sinks
audio capability to the alsa side. Afaiui the "can this sink do audio"
even reflects whether the crtc is running or not (so the audio doesn't
disappear into silence if you've dpms off'ed the screen). I think it's
reflected into some alsa output sense/hotplug flag, that pulseaudio should
take into account by default.
-Daniel
-- 
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

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2720 bytes --]

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> [ Unknown signature status ]
> On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
>> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
>> >
>> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
>> >
>> > > In some cases, in order to accomodate with displays with poor EDIDs, we
>> > > need to ignore that the monitor alledgedly supports audio output and
>> > > disable the audio output.
>> > >
>> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> > > ---
>> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>> > >  1 file changed, 8 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > > index 990b1909f9d7..c0258b011bb2 100644
>> > > --- a/drivers/gpu/drm/drm_edid.c
>> > > +++ b/drivers/gpu/drm/drm_edid.c
>> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>> > >  }
>> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>> > >
>> > > +static bool ignore_edid_audio = false;
>> > > +module_param(ignore_edid_audio, bool, 0644);
>> > > +MODULE_PARM_DESC(ignore_edid_audio,
>> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
>> > > +
>> > >  /**
>> > >   * drm_detect_monitor_audio - check monitor audio capability
>> > >   * @edid: EDID block to scan
>> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>> > >       bool has_audio = false;
>> > >       int start_offset, end_offset;
>> > >
>> > > +     if (ignore_edid_audio)
>> > > +             goto end;
>> > > +
>> > >       edid_ext = drm_find_cea_extension(edid);
>> > >       if (!edid_ext)
>> > >               goto end;
>> >
>> > It looks like the motivation for the original flag on Raspberry Pi was
>> > "I've got a non-audio monitor, but the system comes up trying to play
>> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
>> > to communicate to ALSA that this is not the right place to try to play
>> > audio by default?
>> 
>> Apparently not.  We have users using debug knobs in our drivers to
>> disable display audio because ALSA defaults to that rather than other
>> audio.
>
> I guess one way to do this would be to register the card only when an
> audio-capable monitor is connected instead of doing this at probe
> time. I'm not sure how convenient it is for userspace though.

Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets
to use that.  So, open source is already doing the right thing, and the
problem was that the old driver talking to the firmware wouldn't, thus
the need for a flag.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3049 bytes --]

On Tue, Mar 05, 2019 at 01:47:38PM -0800, Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> writes:
> 
> > [ Unknown signature status ]
> > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> >> >
> >> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> >> >
> >> > > In some cases, in order to accomodate with displays with poor EDIDs, we
> >> > > need to ignore that the monitor alledgedly supports audio output and
> >> > > disable the audio output.
> >> > >
> >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> > > ---
> >> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> >> > >  1 file changed, 8 insertions(+)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > > index 990b1909f9d7..c0258b011bb2 100644
> >> > > --- a/drivers/gpu/drm/drm_edid.c
> >> > > +++ b/drivers/gpu/drm/drm_edid.c
> >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> >> > >  }
> >> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> >> > >
> >> > > +static bool ignore_edid_audio = false;
> >> > > +module_param(ignore_edid_audio, bool, 0644);
> >> > > +MODULE_PARM_DESC(ignore_edid_audio,
> >> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> >> > > +
> >> > >  /**
> >> > >   * drm_detect_monitor_audio - check monitor audio capability
> >> > >   * @edid: EDID block to scan
> >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> >> > >       bool has_audio = false;
> >> > >       int start_offset, end_offset;
> >> > >
> >> > > +     if (ignore_edid_audio)
> >> > > +             goto end;
> >> > > +
> >> > >       edid_ext = drm_find_cea_extension(edid);
> >> > >       if (!edid_ext)
> >> > >               goto end;
> >> >
> >> > It looks like the motivation for the original flag on Raspberry Pi was
> >> > "I've got a non-audio monitor, but the system comes up trying to play
> >> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> >> > to communicate to ALSA that this is not the right place to try to play
> >> > audio by default?
> >> 
> >> Apparently not.  We have users using debug knobs in our drivers to
> >> disable display audio because ALSA defaults to that rather than other
> >> audio.
> >
> > I guess one way to do this would be to register the card only when an
> > audio-capable monitor is connected instead of doing this at probe
> > time. I'm not sure how convenient it is for userspace though.
> 
> Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets
> to use that.  So, open source is already doing the right thing, and the
> problem was that the old driver talking to the firmware wouldn't, thus
> the need for a flag.

Ok, I'll drop that patch then. Thanks!

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3282 bytes --]

On Tue, Mar 05, 2019 at 01:47:38PM -0800, Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> writes:
> 
> > [ Unknown signature status ]
> > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
> >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
> >> >
> >> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
> >> >
> >> > > In some cases, in order to accomodate with displays with poor EDIDs, we
> >> > > need to ignore that the monitor alledgedly supports audio output and
> >> > > disable the audio output.
> >> > >
> >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> > > ---
> >> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
> >> > >  1 file changed, 8 insertions(+)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > > index 990b1909f9d7..c0258b011bb2 100644
> >> > > --- a/drivers/gpu/drm/drm_edid.c
> >> > > +++ b/drivers/gpu/drm/drm_edid.c
> >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
> >> > >  }
> >> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> >> > >
> >> > > +static bool ignore_edid_audio = false;
> >> > > +module_param(ignore_edid_audio, bool, 0644);
> >> > > +MODULE_PARM_DESC(ignore_edid_audio,
> >> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
> >> > > +
> >> > >  /**
> >> > >   * drm_detect_monitor_audio - check monitor audio capability
> >> > >   * @edid: EDID block to scan
> >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
> >> > >       bool has_audio = false;
> >> > >       int start_offset, end_offset;
> >> > >
> >> > > +     if (ignore_edid_audio)
> >> > > +             goto end;
> >> > > +
> >> > >       edid_ext = drm_find_cea_extension(edid);
> >> > >       if (!edid_ext)
> >> > >               goto end;
> >> >
> >> > It looks like the motivation for the original flag on Raspberry Pi was
> >> > "I've got a non-audio monitor, but the system comes up trying to play
> >> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
> >> > to communicate to ALSA that this is not the right place to try to play
> >> > audio by default?
> >> 
> >> Apparently not.  We have users using debug knobs in our drivers to
> >> disable display audio because ALSA defaults to that rather than other
> >> audio.
> >
> > I guess one way to do this would be to register the card only when an
> > audio-capable monitor is connected instead of doing this at probe
> > time. I'm not sure how convenient it is for userspace though.
> 
> Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets
> to use that.  So, open source is already doing the right thing, and the
> problem was that the old driver talking to the firmware wouldn't, thus
> the need for a flag.

I started to look into this, and while on my laptop, the ELD seems to
be exposed as part of /proc/asound/card0/eld*, there's no such file on
the RPi with a 5.0 kernel (and an HDMI monitor with audio support,
obviously). Is it something that used to work at some point?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 3407 bytes --]

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> [ Unknown signature status ]
> On Tue, Mar 05, 2019 at 01:47:38PM -0800, Eric Anholt wrote:
>> Maxime Ripard <maxime.ripard@bootlin.com> writes:
>> 
>> > [ Unknown signature status ]
>> > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote:
>> >> On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt <eric@anholt.net> wrote:
>> >> >
>> >> > Maxime Ripard <maxime.ripard@bootlin.com> writes:
>> >> >
>> >> > > In some cases, in order to accomodate with displays with poor EDIDs, we
>> >> > > need to ignore that the monitor alledgedly supports audio output and
>> >> > > disable the audio output.
>> >> > >
>> >> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> >> > > ---
>> >> > >  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>> >> > >  1 file changed, 8 insertions(+)
>> >> > >
>> >> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> >> > > index 990b1909f9d7..c0258b011bb2 100644
>> >> > > --- a/drivers/gpu/drm/drm_edid.c
>> >> > > +++ b/drivers/gpu/drm/drm_edid.c
>> >> > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>> >> > >  }
>> >> > >  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>> >> > >
>> >> > > +static bool ignore_edid_audio = false;
>> >> > > +module_param(ignore_edid_audio, bool, 0644);
>> >> > > +MODULE_PARM_DESC(ignore_edid_audio,
>> >> > > +              "Ignore the EDID and always consider that a monitor doesn't have audio capabilities");
>> >> > > +
>> >> > >  /**
>> >> > >   * drm_detect_monitor_audio - check monitor audio capability
>> >> > >   * @edid: EDID block to scan
>> >> > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>> >> > >       bool has_audio = false;
>> >> > >       int start_offset, end_offset;
>> >> > >
>> >> > > +     if (ignore_edid_audio)
>> >> > > +             goto end;
>> >> > > +
>> >> > >       edid_ext = drm_find_cea_extension(edid);
>> >> > >       if (!edid_ext)
>> >> > >               goto end;
>> >> >
>> >> > It looks like the motivation for the original flag on Raspberry Pi was
>> >> > "I've got a non-audio monitor, but the system comes up trying to play
>> >> > audio to HDMI instead of the analog jack".  Do we have some way for DRM
>> >> > to communicate to ALSA that this is not the right place to try to play
>> >> > audio by default?
>> >> 
>> >> Apparently not.  We have users using debug knobs in our drivers to
>> >> disable display audio because ALSA defaults to that rather than other
>> >> audio.
>> >
>> > I guess one way to do this would be to register the card only when an
>> > audio-capable monitor is connected instead of doing this at probe
>> > time. I'm not sure how convenient it is for userspace though.
>> 
>> Oh, right, the HDMI encoder passes the ELD to ALSA, and userspace gets
>> to use that.  So, open source is already doing the right thing, and the
>> problem was that the old driver talking to the firmware wouldn't, thus
>> the need for a flag.
>
> I started to look into this, and while on my laptop, the ELD seems to
> be exposed as part of /proc/asound/card0/eld*, there's no such file on
> the RPi with a 5.0 kernel (and an HDMI monitor with audio support,
> obviously). Is it something that used to work at some point?

I don't know, personally.  Sounds like it's worth investigating.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/7] drm/edid: Allow to ignore the HDMI monitor mode

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_edid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c0258b011bb2..2f6df10ed9f1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4156,6 +4156,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_av_sync_delay);
 
+static bool force_dvi_monitor = false;
+module_param(force_dvi_monitor, bool, 0644);
+MODULE_PARM_DESC(force_dvi_monitor,
+		 "Ignore the EDID and always consider the monitor as DVI instead of HDMI");
+
 /**
  * drm_detect_hdmi_monitor - detect whether monitor is HDMI
  * @edid: monitor EDID information
@@ -4170,6 +4175,9 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
 	int i;
 	int start_offset, end_offset;
 
+	if (force_dvi_monitor)
+		return false;
+
 	edid_ext = drm_find_cea_extension(edid);
 	if (!edid_ext)
 		return false;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/edid: Allow to ignore the HDMI monitor mode

[Paul Kocialkowski]

Hi,

On Mon, 2019-03-04 at 15:52 +0100, Maxime Ripard wrote:
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c0258b011bb2..2f6df10ed9f1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4156,6 +4156,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_av_sync_delay);
>  
> +static bool force_dvi_monitor = false;
> +module_param(force_dvi_monitor, bool, 0644);
> +MODULE_PARM_DESC(force_dvi_monitor,
> +		 "Ignore the EDID and always consider the monitor as DVI instead of HDMI");
> +
>  /**
>   * drm_detect_hdmi_monitor - detect whether monitor is HDMI
>   * @edid: monitor EDID information
> @@ -4170,6 +4175,9 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>  	int i;
>  	int start_offset, end_offset;
>  
> +	if (force_dvi_monitor)
> +		return false;
> +
>  	edid_ext = drm_find_cea_extension(edid);
>  	if (!edid_ext)
>  		return false;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/edid: Allow to ignore the HDMI monitor mode

[Jani Nikula]

On Mon, 04 Mar 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c0258b011bb2..2f6df10ed9f1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4156,6 +4156,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_av_sync_delay);
>  
> +static bool force_dvi_monitor = false;
> +module_param(force_dvi_monitor, bool, 0644);
> +MODULE_PARM_DESC(force_dvi_monitor,
> +		 "Ignore the EDID and always consider the monitor as DVI instead of HDMI");
> +

Same reply as with patch 2/7.

BR,
Jani.

>  /**
>   * drm_detect_hdmi_monitor - detect whether monitor is HDMI
>   * @edid: monitor EDID information
> @@ -4170,6 +4175,9 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>  	int i;
>  	int start_offset, end_offset;
>  
> +	if (force_dvi_monitor)
> +		return false;
> +
>  	edid_ext = drm_find_cea_extension(edid);
>  	if (!edid_ext)
>  		return false;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/edid: Allow to ignore the HDMI monitor mode

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 663 bytes --]

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Googling for users of the firmware's hdmi_drive= flag, I'm seeing lots
of people with hdmi_drive=2 (force HDMI mode) due to Raspberry Pi not
allowing HDMI audio with DMT modes, which it looks like DRM does allow.

The only users of hdmi_drive=1 (force HDMI mode) I'm seeing are people
setting things up for non-EDID monitors, but I think the expectation in
DRM is that people would provide an EDID.  So, I'm not sure it makes
sense to support an equivalent of this flag?  Or, it should probably be
part of specifying a custom mode on the command line.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/edid: Allow to ignore the HDMI monitor mode

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1141 bytes --]

Hi Eric,

On Mon, Mar 04, 2019 at 12:02:16PM -0800, Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> writes:
> 
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Googling for users of the firmware's hdmi_drive= flag, I'm seeing lots
> of people with hdmi_drive=2 (force HDMI mode) due to Raspberry Pi not
> allowing HDMI audio with DMT modes, which it looks like DRM does allow.
> 
> The only users of hdmi_drive=1 (force HDMI mode) I'm seeing are people
> setting things up for non-EDID monitors, but I think the expectation in
> DRM is that people would provide an EDID.  So, I'm not sure it makes
> sense to support an equivalent of this flag?  Or, it should probably be
> part of specifying a custom mode on the command line.

That would be adding one more check before registering the card, or
even coming up with something else entirely to mark the output as
non-working at the moment.

Maybe we should start the discussion with Mark Brown about this, and
see what his opinion is?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm/modes: Rewrite the command line parser

[Maxime Ripard]

From: Maxime Ripard <maxime.ripard@free-electrons.com>

Rewrite the command line parser in order to get away from the state machine
parsing the video mode lines.

Hopefully, this will allow to extend it more easily to support named modes
and / or properties set directly on the command line.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_modes.c | 308 +++++++++++++++++++++++--------------
 1 file changed, 193 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 869ac6f4671e..31d61940c007 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -30,6 +30,7 @@
  * authorization from the copyright holder(s) and author(s).
  */
 
+#include <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/list_sort.h>
 #include <linux/export.h>
@@ -1405,6 +1406,131 @@ void drm_connector_list_update(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_list_update);
 
+static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
+				      struct drm_cmdline_mode *mode)
+{
+	if (str[0] != '-')
+		return -EINVAL;
+
+	mode->bpp = simple_strtol(str + 1, end_ptr, 10);
+	mode->bpp_specified = true;
+
+	return 0;
+}
+
+static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
+					  struct drm_cmdline_mode *mode)
+{
+	if (str[0] != '@')
+		return -EINVAL;
+
+	mode->refresh = simple_strtol(str + 1, end_ptr, 10);
+	mode->refresh_specified = true;
+
+	return 0;
+}
+
+static int drm_mode_parse_cmdline_extra(const char *str, int length,
+					struct drm_connector *connector,
+					struct drm_cmdline_mode *mode)
+{
+	int i;
+
+	for (i = 0; i < length; i++) {
+		switch (str[i]) {
+		case 'i':
+			mode->interlace = true;
+			break;
+		case 'm':
+			mode->margins = true;
+			break;
+		case 'D':
+			if (mode->force != DRM_FORCE_UNSPECIFIED)
+				return -EINVAL;
+
+			if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&
+			    (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))
+				mode->force = DRM_FORCE_ON;
+			else
+				mode->force = DRM_FORCE_ON_DIGITAL;
+			break;
+		case 'd':
+			if (mode->force != DRM_FORCE_UNSPECIFIED)
+				return -EINVAL;
+
+			mode->force = DRM_FORCE_OFF;
+			break;
+		case 'e':
+			if (mode->force != DRM_FORCE_UNSPECIFIED)
+				return -EINVAL;
+
+			mode->force = DRM_FORCE_ON;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
+					   bool extras,
+					   struct drm_connector *connector,
+					   struct drm_cmdline_mode *mode)
+{
+	bool rb = false, cvt = false;
+	int xres = 0, yres = 0;
+	int remaining, i;
+	char *end_ptr;
+
+	xres = simple_strtol(str, &end_ptr, 10);
+
+	if (end_ptr[0] != 'x')
+		return -EINVAL;
+	end_ptr++;
+
+	yres = simple_strtol(end_ptr, &end_ptr, 10);
+
+	remaining = length - (end_ptr - str);
+	if (remaining < 0)
+		return -EINVAL;
+
+	for (i = 0; i < remaining; i++) {
+		switch (end_ptr[i]) {
+		case 'M':
+			cvt = true;
+			break;
+		case 'R':
+			rb = true;
+			break;
+		default:
+			/*
+			 * Try to pass that to our extras parsing
+			 * function to handle the case where the
+			 * extras are directly after the resolution
+			 */
+			if (extras) {
+				int ret = drm_mode_parse_cmdline_extra(end_ptr + i,
+								       1,
+								       connector,
+								       mode);
+				if (ret)
+					return ret;
+			} else {
+				return -EINVAL;
+			}
+		}
+	}
+
+	mode->xres = xres;
+	mode->yres = yres;
+	mode->cvt = cvt;
+	mode->rb = rb;
+
+	return 0;
+}
+
 /**
  * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
  * @mode_option: optional per connector mode option
@@ -1431,13 +1557,12 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 					       struct drm_cmdline_mode *mode)
 {
 	const char *name;
-	unsigned int namelen;
-	bool res_specified = false, bpp_specified = false, refresh_specified = false;
-	unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0;
-	bool yres_specified = false, cvt = false, rb = false;
-	bool interlace = false, margins = false, was_digit = false;
-	int i;
-	enum drm_connector_force force = DRM_FORCE_UNSPECIFIED;
+	bool parse_extras = false;
+	unsigned int bpp_off = 0, refresh_off = 0;
+	unsigned int mode_end = 0;
+	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
+	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
+	int ret;
 
 #ifdef CONFIG_FB
 	if (!mode_option)
@@ -1450,127 +1575,80 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	}
 
 	name = mode_option;
-	namelen = strlen(name);
-	for (i = namelen-1; i >= 0; i--) {
-		switch (name[i]) {
-		case '@':
-			if (!refresh_specified && !bpp_specified &&
-			    !yres_specified && !cvt && !rb && was_digit) {
-				refresh = simple_strtol(&name[i+1], NULL, 10);
-				refresh_specified = true;
-				was_digit = false;
-			} else
-				goto done;
-			break;
-		case '-':
-			if (!bpp_specified && !yres_specified && !cvt &&
-			    !rb && was_digit) {
-				bpp = simple_strtol(&name[i+1], NULL, 10);
-				bpp_specified = true;
-				was_digit = false;
-			} else
-				goto done;
-			break;
-		case 'x':
-			if (!yres_specified && was_digit) {
-				yres = simple_strtol(&name[i+1], NULL, 10);
-				yres_specified = true;
-				was_digit = false;
-			} else
-				goto done;
-			break;
-		case '0' ... '9':
-			was_digit = true;
-			break;
-		case 'M':
-			if (yres_specified || cvt || was_digit)
-				goto done;
-			cvt = true;
-			break;
-		case 'R':
-			if (yres_specified || cvt || rb || was_digit)
-				goto done;
-			rb = true;
-			break;
-		case 'm':
-			if (cvt || yres_specified || was_digit)
-				goto done;
-			margins = true;
-			break;
-		case 'i':
-			if (cvt || yres_specified || was_digit)
-				goto done;
-			interlace = true;
-			break;
-		case 'e':
-			if (yres_specified || bpp_specified || refresh_specified ||
-			    was_digit || (force != DRM_FORCE_UNSPECIFIED))
-				goto done;
 
-			force = DRM_FORCE_ON;
-			break;
-		case 'D':
-			if (yres_specified || bpp_specified || refresh_specified ||
-			    was_digit || (force != DRM_FORCE_UNSPECIFIED))
-				goto done;
+	if (!isdigit(name[0]))
+		return false;
 
-			if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&
-			    (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))
-				force = DRM_FORCE_ON;
-			else
-				force = DRM_FORCE_ON_DIGITAL;
-			break;
-		case 'd':
-			if (yres_specified || bpp_specified || refresh_specified ||
-			    was_digit || (force != DRM_FORCE_UNSPECIFIED))
-				goto done;
+	/* Try to locate the bpp and refresh specifiers, if any */
+	bpp_ptr = strchr(name, '-');
+	if (bpp_ptr) {
+		bpp_off = bpp_ptr - name;
+		mode->bpp_specified = true;
+	}
 
-			force = DRM_FORCE_OFF;
-			break;
-		default:
-			goto done;
-		}
+	refresh_ptr = strchr(name, '@');
+	if (refresh_ptr) {
+		if (named_mode)
+			return false;
+
+		refresh_off = refresh_ptr - name;
+		mode->refresh_specified = true;
 	}
 
-	if (i < 0 && yres_specified) {
-		char *ch;
-		xres = simple_strtol(name, &ch, 10);
-		if ((ch != NULL) && (*ch == 'x'))
-			res_specified = true;
-		else
-			i = ch - name;
-	} else if (!yres_specified && was_digit) {
-		/* catch mode that begins with digits but has no 'x' */
-		i = 0;
+	/* Locate the end of the name / resolution, and parse it */
+	if (bpp_ptr && refresh_ptr) {
+		mode_end = min(bpp_off, refresh_off);
+	} else if (bpp_ptr) {
+		mode_end = bpp_off;
+	} else if (refresh_ptr) {
+		mode_end = refresh_off;
+	} else {
+		mode_end = strlen(name);
+		parse_extras = true;
 	}
-done:
-	if (i >= 0) {
-		pr_warn("[drm] parse error at position %i in video mode '%s'\n",
-			i, name);
-		mode->specified = false;
+
+	ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
+					      parse_extras,
+					      connector,
+					      mode);
+	if (ret)
 		return false;
-	}
+	mode->specified = true;
 
-	if (res_specified) {
-		mode->specified = true;
-		mode->xres = xres;
-		mode->yres = yres;
+	if (bpp_ptr) {
+		ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode);
+		if (ret)
+			return false;
 	}
 
-	if (refresh_specified) {
-		mode->refresh_specified = true;
-		mode->refresh = refresh;
+	if (refresh_ptr) {
+		ret = drm_mode_parse_cmdline_refresh(refresh_ptr,
+						     &refresh_end_ptr, mode);
+		if (ret)
+			return false;
 	}
 
-	if (bpp_specified) {
-		mode->bpp_specified = true;
-		mode->bpp = bpp;
+	/*
+	 * Locate the end of the bpp / refresh, and parse the extras
+	 * if relevant
+	 */
+	if (bpp_ptr && refresh_ptr)
+		extra_ptr = max(bpp_end_ptr, refresh_end_ptr);
+	else if (bpp_ptr)
+		extra_ptr = bpp_end_ptr;
+	else if (refresh_ptr)
+		extra_ptr = refresh_end_ptr;
+
+	if (extra_ptr) {
+		int remaining = strlen(name) - (extra_ptr - name);
+
+		/*
+		 * We still have characters to process, while
+		 * we shouldn't have any
+		 */
+		if (remaining > 0)
+			return false;
 	}
-	mode->rb = rb;
-	mode->cvt = cvt;
-	mode->interlace = interlace;
-	mode->margins = margins;
-	mode->force = force;
 
 	return true;
 }
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm/modes: Support modes names on the command line

[Maxime Ripard]

From: Maxime Ripard <maxime.ripard@free-electrons.com>

The drm subsystem also uses the video= kernel parameter, and in the
documentation refers to the fbdev documentation for that parameter.

However, that documentation also says that instead of giving the mode using
its resolution we can also give a name. However, DRM doesn't handle that
case at the moment. Even though in most case it shouldn't make any
difference, it might be useful for analog modes, where different standards
might have the same resolution, but still have a few different parameters
that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_connector.c |  3 +-
 drivers/gpu/drm/drm_fb_helper.c |  4 +++-
 drivers/gpu/drm/drm_modes.c     | 49 +++++++++++++++++++++++-----------
 include/drm/drm_connector.h     |  1 +-
 4 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index dd40eff0911c..799e932c114d 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -139,8 +139,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		connector->force = mode->force;
 	}
 
-	DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
+	DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
 		      connector->name,
+		      mode->name ? mode->name : "",
 		      mode->xres, mode->yres,
 		      mode->refresh_specified ? mode->refresh : 60,
 		      mode->rb ? " reduced blanking" : "",
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0e9349ff2d16..cacc4b56344e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2158,6 +2158,10 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
 	prefer_non_interlace = !cmdline_mode->interlace;
 again:
 	list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
+		/* Check (optional) mode name first */
+		if (!strcmp(mode->name, cmdline_mode->name))
+			return mode;
+
 		/* check width/height */
 		if (mode->hdisplay != cmdline_mode->xres ||
 		    mode->vdisplay != cmdline_mode->yres)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 31d61940c007..3d843d17370a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1557,7 +1557,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 					       struct drm_cmdline_mode *mode)
 {
 	const char *name;
-	bool parse_extras = false;
+	bool named_mode = false, parse_extras = false;
 	unsigned int bpp_off = 0, refresh_off = 0;
 	unsigned int mode_end = 0;
 	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
@@ -1576,8 +1576,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 
 	name = mode_option;
 
+	/*
+	 * If the first character is not a digit, then it means that
+	 * we have a named mode.
+	 */
 	if (!isdigit(name[0]))
-		return false;
+		named_mode = true;
+	else
+		named_mode = false;
 
 	/* Try to locate the bpp and refresh specifiers, if any */
 	bpp_ptr = strchr(name, '-');
@@ -1607,12 +1613,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		parse_extras = true;
 	}
 
-	ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
-					      parse_extras,
-					      connector,
-					      mode);
-	if (ret)
-		return false;
+	if (named_mode) {
+		strncpy(mode->name, name, mode_end);
+	} else {
+		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
+						      parse_extras,
+						      connector,
+						      mode);
+		if (ret)
+			return false;
+	}
 	mode->specified = true;
 
 	if (bpp_ptr) {
@@ -1640,14 +1650,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		extra_ptr = refresh_end_ptr;
 
 	if (extra_ptr) {
-		int remaining = strlen(name) - (extra_ptr - name);
+		if (!named_mode) {
+			int len = strlen(name) - (extra_ptr - name);
 
-		/*
-		 * We still have characters to process, while
-		 * we shouldn't have any
-		 */
-		if (remaining > 0)
-			return false;
+			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+							   connector, mode);
+			if (ret)
+				return false;
+		} else {
+			int remaining = strlen(name) - (extra_ptr - name);
+
+			/*
+			 * We still have characters to process, while
+			 * we shouldn't have any
+			 */
+			if (remaining > 0)
+				return false;
+		}
 	}
 
 	return true;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8fe22abb1e10..215761153245 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -823,6 +823,7 @@ struct drm_connector_funcs {
 
 /* mode specified on the command line */
 struct drm_cmdline_mode {
+	char name[DRM_DISPLAY_MODE_LEN];
 	bool specified;
 	bool refresh_specified;
 	bool bpp_specified;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/7] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

Rotations and reflections setup are needed in some scenarios to initialise
properly the initial framebuffer. Some drivers already had a bunch of
quirks to deal with this, such as either a private kernel command line
parameter (omapdss) or on the device tree (various panels).

In order to accomodate this, let's create a video mode parameter to deal
with the rotation and reflexion.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_fb_helper.c |   4 +-
 drivers/gpu/drm/drm_modes.c     | 110 +++++++++++++++++++++++++++------
 include/drm/drm_connector.h     |   1 +-
 3 files changed, 95 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cacc4b56344e..1457a1d1a423 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2523,6 +2523,7 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
 				    struct drm_connector *connector)
 {
 	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
+	struct drm_cmdline_mode *mode = &connector->cmdline_mode;
 	uint64_t valid_mask = 0;
 	int i, rotation;
 
@@ -2542,6 +2543,9 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
 		rotation = DRM_MODE_ROTATE_0;
 	}
 
+	if (mode->rotation != DRM_MODE_ROTATE_0)
+		fb_crtc->rotation = mode->rotation;
+
 	/*
 	 * TODO: support 90 / 270 degree hardware rotation,
 	 * depending on the hardware this may require the framebuffer
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 3d843d17370a..10c7e9322f76 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
 	return 0;
 }
 
+static int drm_mode_parse_cmdline_options(char *str, size_t len,
+					  struct drm_connector *connector,
+					  struct drm_cmdline_mode *mode)
+{
+	unsigned int rotation = 0;
+	char *sep = str;
+
+	while ((sep = strchr(sep, ','))) {
+		char *delim, *option;
+
+		option = sep + 1;
+		delim = strchr(option, '=');
+		if (!delim) {
+			delim = strchr(option, ',');
+
+			if (!delim)
+				delim = str + len;
+		}
+
+		if (!strncmp(option, "rotate", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int deg;
+
+			deg = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			switch (deg) {
+			case 0:
+				rotation |= DRM_MODE_ROTATE_0;
+				break;
+
+			case 90:
+				rotation |= DRM_MODE_ROTATE_90;
+				break;
+
+			case 180:
+				rotation |= DRM_MODE_ROTATE_180;
+				break;
+
+			case 270:
+				rotation |= DRM_MODE_ROTATE_270;
+				break;
+
+			default:
+				return -EINVAL;
+			}
+		} else if (!strncmp(option, "reflect_x", delim - option)) {
+			rotation |= DRM_MODE_REFLECT_X;
+			sep = delim;
+		} else if (!strncmp(option, "reflect_y", delim - option)) {
+			rotation |= DRM_MODE_REFLECT_Y;
+			sep = delim;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	mode->rotation = rotation;
+
+	return 0;
+}
+
 /**
  * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
  * @mode_option: optional per connector mode option
@@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 {
 	const char *name;
 	bool named_mode = false, parse_extras = false;
-	unsigned int bpp_off = 0, refresh_off = 0;
+	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
 	unsigned int mode_end = 0;
 	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
+	char *options_ptr = NULL;
 	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
 	int ret;
 
@@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		mode->refresh_specified = true;
 	}
 
+	/* Locate the start of named options */
+	options_ptr = strchr(name, ',');
+	if (options_ptr)
+		options_off = options_ptr - name;
+
 	/* Locate the end of the name / resolution, and parse it */
-	if (bpp_ptr && refresh_ptr) {
-		mode_end = min(bpp_off, refresh_off);
-	} else if (bpp_ptr) {
+	if (bpp_ptr) {
 		mode_end = bpp_off;
 	} else if (refresh_ptr) {
 		mode_end = refresh_off;
+	} else if (options_ptr) {
+		mode_end = options_off;
 	} else {
 		mode_end = strlen(name);
 		parse_extras = true;
@@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	else if (refresh_ptr)
 		extra_ptr = refresh_end_ptr;
 
-	if (extra_ptr) {
-		if (!named_mode) {
-			int len = strlen(name) - (extra_ptr - name);
+	if (extra_ptr &&
+	    extra_ptr != options_ptr) {
+		int len = strlen(name) - (extra_ptr - name);
 
-			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
-							   connector, mode);
-			if (ret)
-				return false;
-		} else {
-			int remaining = strlen(name) - (extra_ptr - name);
+		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+						   connector, mode);
+		if (ret)
+			return false;
+	}
 
-			/*
-			 * We still have characters to process, while
-			 * we shouldn't have any
-			 */
-			if (remaining > 0)
-				return false;
-		}
+	if (options_ptr) {
+		int len = strlen(name) - (options_ptr - name);
+
+		ret = drm_mode_parse_cmdline_options(options_ptr, len,
+						     connector, mode);
+		if (ret)
+			return false;
 	}
 
 	return true;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 215761153245..cff18360f52a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -834,6 +834,7 @@ struct drm_cmdline_mode {
 	bool interlace;
 	bool cvt;
 	bool margins;
+	unsigned int rotation;
 	enum drm_connector_force force;
 };
 
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm/modes: Parse overscan properties

[Maxime Ripard]

Properly configuring the overscan properties might be needed for the
initial setup of the framebuffer for display that still have overscan.
Let's allow for more properties on the kernel command line to setup each
margin.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
 include/drm/drm_connector.h     |  1 +-
 3 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1457a1d1a423..3569016111a4 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2569,6 +2569,51 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
 	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
 }
 
+static void drm_setup_connector_margins(struct drm_connector *connector)
+{
+	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	struct drm_device *dev = connector->dev;
+	int ret;
+
+	if (!drm_drv_uses_atomic_modeset(dev))
+		return;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	state = drm_atomic_state_alloc(dev);
+	state->acquire_ctx = &ctx;
+
+retry:
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_left_margin_property,
+				cmdline->overscan_left);
+
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_right_margin_property,
+				cmdline->overscan_right);
+
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_top_margin_property,
+				cmdline->overscan_top);
+
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_bottom_margin_property,
+				cmdline->overscan_bottom);
+
+	ret = drm_atomic_commit(state);
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_atomic_state_put(state);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 			    u32 width, u32 height)
 {
@@ -2682,6 +2727,8 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
 		struct drm_connector *connector =
 					fb_helper->connector_info[i]->connector;
 
+		drm_setup_connector_margins(connector);
+
 		/* use first connected connector for the physical dimensions */
 		if (connector->status == connector_status_connected) {
 			info->var.width = connector->display_info.width_mm;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 10c7e9322f76..6613db04cf77 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
 		} else if (!strncmp(option, "reflect_y", delim - option)) {
 			rotation |= DRM_MODE_REFLECT_Y;
 			sep = delim;
+		} else if (!strncmp(option, "overscan_right", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_right = margin;
+		} else if (!strncmp(option, "overscan_left", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_left = margin;
+		} else if (!strncmp(option, "overscan_top", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_top = margin;
+		} else if (!strncmp(option, "overscan_bottom", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_bottom = margin;
 		} else {
 			return -EINVAL;
 		}
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index cff18360f52a..731a79752d13 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -830,6 +830,7 @@ struct drm_cmdline_mode {
 	int xres, yres;
 	int bpp;
 	int refresh;
+	int overscan_right, overscan_top, overscan_left, overscan_bottom;
 	bool rb;
 	bool interlace;
 	bool cvt;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Peter Stuge]

Hi,

Maxime Ripard wrote:
> properties to initialise the overscan or rotation parameters, or the
> one to deal with broken displays.

How does that work on systems with multiple connectors?

On SBCs with only one output I guess it's fine to have a global option,
but it may be important for new options to also be usable on more
complex systems. And maybe even SPI displays are relevant?

Also, some of the forward-ported patches still have your old email
address, maybe you want to change that.


Thanks

//Peter

Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 934 bytes --]

Hi Peter,

On Mon, Mar 04, 2019 at 03:21:35PM +0000, Peter Stuge wrote:
> Hi,
> 
> Maxime Ripard wrote:
> > properties to initialise the overscan or rotation parameters, or the
> > one to deal with broken displays.
> 
> How does that work on systems with multiple connectors?
> 
> On SBCs with only one output I guess it's fine to have a global option,
> but it may be important for new options to also be usable on more
> complex systems. And maybe even SPI displays are relevant?

These properties are per-connector, so only the display attached to
that connector will be affected, and you can have multiple connectors
with various properties and values defined if you want to

> Also, some of the forward-ported patches still have your old email
> address, maybe you want to change that.

Good catch, thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Stefan Wahren]

Hi Maxime,

Am 04.03.2019 um 15:52 schrieb Maxime Ripard:
> Hi,
>
> The proprietary stack for the RaspberryPi allows for a number of video
> parameters widely used by their users, but yet don't have any equivalents
> in the mainline kernel.
>
> Those options are detailed here:
> https://www.raspberrypi.org/documentation/configuration/config-txt/video.md
>
> While not all of them are desirable to have in the mainline kernel, some of
> them still have value, such as properties to initialise the overscan or
> rotation parameters, or the one to deal with broken displays.
>
> This series is an attempt to support those, and is based on a rewrite of
> the command line parser I did a couple of years ago that never reached
> upstream (due to a lack of time on my side). While this parser was
> initially made to deal with named modes (in order to support TV modes), it
> also allowed to extend it more easily, which is why it's resurrected.
>
> Since a change of the command line parser can pretty easily get things
> wrong and introduce regressions, I also worked with a number of unit tests
> that you can find here: http://code.bulix.org/tpo7dg-607264?raw
>
> Eventually, I guess those tests should be part of the kernel somewhere, but
> I haven't found a suitable place for them to be included yet.
>
> Let me know what you think,
> Maxime
>
thanks for this series. In case you want some feedback from the 
Foundation, please add Phil Elwell instead of Eben to CC.

Stefan

Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1570 bytes --]

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> Hi,
>
> The proprietary stack for the RaspberryPi allows for a number of video
> parameters widely used by their users, but yet don't have any equivalents
> in the mainline kernel.
>
> Those options are detailed here:
> https://www.raspberrypi.org/documentation/configuration/config-txt/video.md
>
> While not all of them are desirable to have in the mainline kernel, some of
> them still have value, such as properties to initialise the overscan or
> rotation parameters, or the one to deal with broken displays.
>
> This series is an attempt to support those, and is based on a rewrite of
> the command line parser I did a couple of years ago that never reached
> upstream (due to a lack of time on my side). While this parser was
> initially made to deal with named modes (in order to support TV modes), it
> also allowed to extend it more easily, which is why it's resurrected.

FWIW for other reviewers, the overscan and rotation are the really
important parts of this series.  Since Raspberry Pi ends up connected to
TVs so frequently, there are many users of the overscan workaround.
Rotation is important for supporting the official DSI touchscreen panel,
which is unfortunately mounted upside down in most mounts you'll find.

> Since a change of the command line parser can pretty easily get things
> wrong and introduce regressions, I also worked with a number of unit tests
> that you can find here: http://code.bulix.org/tpo7dg-607264?raw

Would kselftests be an appropriate way to include these, maybe?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 553 bytes --]

Hi,

On Mon, Mar 04, 2019 at 12:06:01PM -0800, Eric Anholt wrote:
> > Since a change of the command line parser can pretty easily get things
> > wrong and introduce regressions, I also worked with a number of unit tests
> > that you can find here: http://code.bulix.org/tpo7dg-607264?raw
> 
> Would kselftests be an appropriate way to include these, maybe?

That looks like a good way to do this indeed. I'll try it and let you know

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration

[Daniel Vetter]

On Tue, Mar 05, 2019 at 10:14:16AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Mar 04, 2019 at 12:06:01PM -0800, Eric Anholt wrote:
> > > Since a change of the command line parser can pretty easily get things
> > > wrong and introduce regressions, I also worked with a number of unit tests
> > > that you can find here: http://code.bulix.org/tpo7dg-607264?raw
> > 
> > Would kselftests be an appropriate way to include these, maybe?
> 
> That looks like a good way to do this indeed. I'll try it and let you know

drm/selftests already has a pile of infrastructure, together with igt and
will be run in intel-gfx-ci to make sure stuff keeps working.

And yeah parser like this is imo the perfect selftest use-case.

If you wonder about kselftest and kunit and all that: We have some compat
glue, but I do hope that longterm we can all unify these internal
self/unit tests under one framework. For now I recommend the drm one,
since that's CI'ed.

Cheers, Daniel
-- 
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