commit eaf99c749d43 ("drm: Perform cmdline mode parsing during connector
initialisation") breaks HDMI output on BeagleBone Black with LG TV
(model 19LS4R-ZA) when "video=HDMI-A-1:1280x720@60" is specified on
the command line.
The reason is newly added mode
'"1280x720" 60 74440 1280 1336 1472 1664 720 721 724 746 0x20 0x6'
, which is added by function drm_helper_probe_add_cmdline_mode (introduced
in above mentioned commit). This mode causes TV to go black and show "No
signal" message.
When cmdline_mode is set, it is preferred to use matching mode obtained
from EDID, than mode calculated by function
drm_mode_create_from_cmdline_mode
Signed-off-by: Radek Dostal <[email protected]>
---
drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cac4229..72d6ed0 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1284,6 +1284,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
{
struct drm_cmdline_mode *cmdline_mode;
struct drm_display_mode *mode;
+ bool prefer_non_userdef;
bool prefer_non_interlace;
cmdline_mode = &fb_helper_conn->connector->cmdline_mode;
@@ -1296,6 +1297,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
if (cmdline_mode->rb || cmdline_mode->margins)
goto create_mode;
+ prefer_non_userdef = true;
prefer_non_interlace = !cmdline_mode->interlace;
again:
list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
@@ -1312,6 +1314,9 @@ again:
if (cmdline_mode->interlace) {
if (!(mode->flags & DRM_MODE_FLAG_INTERLACE))
continue;
+ } else if (prefer_non_userdef) {
+ if (mode->type & DRM_MODE_TYPE_USERDEF)
+ continue;
} else if (prefer_non_interlace) {
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
continue;
@@ -1319,6 +1324,11 @@ again:
return mode;
}
+ if (prefer_non_userdef) {
+ prefer_non_userdef = true;
+ goto again;
+ }
+
if (prefer_non_interlace) {
prefer_non_interlace = false;
goto again;
--
1.7.9.5
commit eaf99c749d43 ("drm: Perform cmdline mode parsing during connector
initialisation") breaks HDMI output on BeagleBone Black with LG TV
(model 19LS4R-ZA) when "video=HDMI-A-1:1280x720@60" is specified on
the command line.
The reason is newly added mode
'"1280x720" 60 74440 1280 1336 1472 1664 720 721 724 746 0x20 0x6'
, which is added by function drm_helper_probe_add_cmdline_mode (introduced
in above mentioned commit). This mode causes TV to go black and show "No
signal" message.
When cmdline_mode is set, it is preferred to use matching mode obtained
from EDID, than mode calculated by function
drm_mode_create_from_cmdline_mode
Signed-off-by: Radek Dostal <[email protected]>
---
v2: fixed if (prefer_non_userdef) condition, which was causing infinite
loop before.
drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cac4229..fac425e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1284,6 +1284,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
{
struct drm_cmdline_mode *cmdline_mode;
struct drm_display_mode *mode;
+ bool prefer_non_userdef;
bool prefer_non_interlace;
cmdline_mode = &fb_helper_conn->connector->cmdline_mode;
@@ -1296,6 +1297,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
if (cmdline_mode->rb || cmdline_mode->margins)
goto create_mode;
+ prefer_non_userdef = true;
prefer_non_interlace = !cmdline_mode->interlace;
again:
list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
@@ -1312,6 +1314,9 @@ again:
if (cmdline_mode->interlace) {
if (!(mode->flags & DRM_MODE_FLAG_INTERLACE))
continue;
+ } else if (prefer_non_userdef) {
+ if (mode->type & DRM_MODE_TYPE_USERDEF)
+ continue;
} else if (prefer_non_interlace) {
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
continue;
@@ -1319,6 +1324,11 @@ again:
return mode;
}
+ if (prefer_non_userdef) {
+ prefer_non_userdef = false;
+ goto again;
+ }
+
if (prefer_non_interlace) {
prefer_non_interlace = false;
goto again;
--
1.7.9.5
On Mon, Apr 20, 2015 at 07:26:33AM +0200, Radek Dostal wrote:
> commit eaf99c749d43 ("drm: Perform cmdline mode parsing during connector
> initialisation") breaks HDMI output on BeagleBone Black with LG TV
> (model 19LS4R-ZA) when "video=HDMI-A-1:1280x720@60" is specified on
> the command line.
>
> The reason is newly added mode
> '"1280x720" 60 74440 1280 1336 1472 1664 720 721 724 746 0x20 0x6'
> , which is added by function drm_helper_probe_add_cmdline_mode (introduced
> in above mentioned commit). This mode causes TV to go black and show "No
> signal" message.
>
> When cmdline_mode is set, it is preferred to use matching mode obtained
> from EDID, than mode calculated by function
> drm_mode_create_from_cmdline_mode
The EDID modes should be earlier in the list, and so higher priority
than the cmdline mode. The only instance I see that breaking down is if
the mode gets created by drm_pick_cmdline_mode, i.e.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cac422916c7a..d55c2de6a99f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1327,7 +1327,7 @@ again:
create_mode:
mode = drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev,
cmdline_mode);
- list_add(&mode->head, &fb_helper_conn->connector->modes);
+ list_add_tail(&mode->head, &fb_helper_conn->connector->modes);
return mode;
}
EXPORT_SYMBOL(drm_pick_cmdline_mode);
Can you please print the matching modes and trace where the userdef mode
gets added before the EDID modes?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Hi Chris,
On 04/20/2015 11:09 AM, Chris Wilson wrote:
> The EDID modes should be earlier in the list, and so higher priority
> than the cmdline mode. The only instance I see that breaking down is if
> the mode gets created by drm_pick_cmdline_mode, i.e.
indeed at the beginning the command line mode is added to the end of the
list, but later on it seems to me that all modes are reordered based on
the resolution and clock and than mode generated by
drm_helper_probe_add_cmdline_mode gets upper in the list as it has
higher clock value. Please see attached output of dmesg.
Additionally I am attaching the output with commit eaf99c749d43
reverted, where you can see that the mode "1280x720@60 with pixel clock
74440" is not even added to the list.
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
b/drivers/gpu/drm/drm_fb_helper.c
> index cac422916c7a..d55c2de6a99f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1327,7 +1327,7 @@ again:
> create_mode:
> mode =
drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev,
> cmdline_mode);
> - list_add(&mode->head, &fb_helper_conn->connector->modes);
> + list_add_tail(&mode->head, &fb_helper_conn->connector->modes);
> return mode;
> }
> EXPORT_SYMBOL(drm_pick_cmdline_mode);
I am pretty sure, this will not help as in my case it never makes it to
create_mode.
Thanks,
Radek
On Mon, Apr 20, 2015 at 11:36:04AM +0200, Radek Dost?l wrote:
> Hi Chris,
>
> On 04/20/2015 11:09 AM, Chris Wilson wrote:
> > The EDID modes should be earlier in the list, and so higher priority
> > than the cmdline mode. The only instance I see that breaking down is if
> > the mode gets created by drm_pick_cmdline_mode, i.e.
>
> indeed at the beginning the command line mode is added to the end of the
> list, but later on it seems to me that all modes are reordered based on
> the resolution and clock and than mode generated by
> drm_helper_probe_add_cmdline_mode gets upper in the list as it has
> higher clock value. Please see attached output of dmesg.
Ah thanks, drm_mode_sort()!
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 213b11e..9c8357f 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1127,6 +1127,7 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head
((a->type & DRM_MODE_TYPE_PREFERRED) != 0);
if (diff)
return diff;
+
diff = b->hdisplay * b->vdisplay - a->hdisplay * a->vdisplay;
if (diff)
return diff;
@@ -1136,7 +1137,16 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head
return diff;
diff = b->clock - a->clock;
- return diff;
+ if (diff)
+ return diff;
+
+ /* sort user-defined modes after reported modes of same resolution */
+ diff = ((a->type & DRM_MODE_TYPE_USERDEF) != 0) -
+ ((b->type & DRM_MODE_TYPE_USERDEF) != 0);
+ if (diff)
+ return diff;
+
+ return 0;
}
Perhaps?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Mon, Apr 20, 2015 at 10:46:08AM +0100, Chris Wilson wrote:
> On Mon, Apr 20, 2015 at 11:36:04AM +0200, Radek Dost?l wrote:
> > Hi Chris,
> >
> > On 04/20/2015 11:09 AM, Chris Wilson wrote:
> > > The EDID modes should be earlier in the list, and so higher priority
> > > than the cmdline mode. The only instance I see that breaking down is if
> > > the mode gets created by drm_pick_cmdline_mode, i.e.
> >
> > indeed at the beginning the command line mode is added to the end of the
> > list, but later on it seems to me that all modes are reordered based on
> > the resolution and clock and than mode generated by
> > drm_helper_probe_add_cmdline_mode gets upper in the list as it has
> > higher clock value. Please see attached output of dmesg.
>
> Ah thanks, drm_mode_sort()!
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 213b11e..9c8357f 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1127,6 +1127,7 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head
> ((a->type & DRM_MODE_TYPE_PREFERRED) != 0);
> if (diff)
> return diff;
> +
> diff = b->hdisplay * b->vdisplay - a->hdisplay * a->vdisplay;
> if (diff)
> return diff;
> @@ -1136,7 +1137,16 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head
> return diff;
>
> diff = b->clock - a->clock;
> - return diff;
> + if (diff)
> + return diff;
> +
> + /* sort user-defined modes after reported modes of same resolution */
> + diff = ((a->type & DRM_MODE_TYPE_USERDEF) != 0) -
> + ((b->type & DRM_MODE_TYPE_USERDEF) != 0);
> + if (diff)
> + return diff;
Hmm, so that should be before the clock comparison as well to fix your
example. Not as neat.
The other idea I was considering was not adding the GTF cmdline mode if
the probed modes already contain one of a matching resolution. The goal
here is to also not present a mode to userspace that is invalid.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Hi Chris,
On 04/20/2015 11:58 AM, Chris Wilson wrote:
> Hmm, so that should be before the clock comparison as well to fix your
> example. Not as neat.
indeed that is required.
> The other idea I was considering was not adding the GTF cmdline mode if
> the probed modes already contain one of a matching resolution. The goal
> here is to also not present a mode to userspace that is invalid.
Unfortunately you can not do that. I already tried. At the time when
drm_helper_probe_add_cmdline_mode is called EDID informations are not
yet available.
Only option would be to remove the GTF cmdline mode, when matching mode
is found in EDID.
Thanks,
Radek
On Mon, Apr 20, 2015 at 12:38:25PM +0200, Radek Dost?l wrote:
> Hi Chris,
>
> On 04/20/2015 11:58 AM, Chris Wilson wrote:
> > Hmm, so that should be before the clock comparison as well to fix your
> > example. Not as neat.
>
> indeed that is required.
>
> > The other idea I was considering was not adding the GTF cmdline mode if
> > the probed modes already contain one of a matching resolution. The goal
> > here is to also not present a mode to userspace that is invalid.
>
> Unfortunately you can not do that. I already tried. At the time when
> drm_helper_probe_add_cmdline_mode is called EDID informations are not
> yet available.
My understanding is that it should be. fb_helper.initial_config does a
probe first, and the intel_fb_initial_config() should only keep the
active mode.
> Only option would be to remove the GTF cmdline mode, when matching mode
> is found in EDID.
So basically
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 6350387..9212bec 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -79,13 +79,29 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
{
+ struct drm_cmdline_mode *cmdline_mode;
struct drm_display_mode *mode;
- if (!connector->cmdline_mode.specified)
+ cmdline_mode = &connector->cmdline_mode;
+ if (!cmdline_mode->specified)
return 0;
+ /* Only add a GTF mode if we find no matching probed modes */
+ list_for_each_entry(mode, &connector->modes, head) {
+ if (mode->hdisplay != cmdline_mode->xres ||
+ mode->vdisplay != cmdline_mode->yres)
+ continue;
+
+ if (cmdline_mode->refresh_specified) {
+ if (mode->vrefresh != cmdline_mode->refresh)
+ continue;
+ }
+
+ return 0;
+ }
+
mode = drm_mode_create_from_cmdline_mode(connector->dev,
- &connector->cmdline_mode);
+ cmdline_mode);
if (mode == NULL)
return 0;
is not sufficient?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Hi Chris,
On 04/20/2015 12:48 PM, Chris Wilson wrote:
>> Unfortunately you can not do that. I already tried. At the time when
>> > drm_helper_probe_add_cmdline_mode is called EDID informations are not
>> > yet available.
> My understanding is that it should be. fb_helper.initial_config does a
> probe first, and the intel_fb_initial_config() should only keep the
> active mode.
uff, sorry I am not sure that I follow here - I am not that familiar
with the whole subsystem.
>> > Only option would be to remove the GTF cmdline mode, when matching mode
>> > is found in EDID.
> So basically
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 6350387..9212bec 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -79,13 +79,29 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
>
> static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> {
> + struct drm_cmdline_mode *cmdline_mode;
> struct drm_display_mode *mode;
>
> - if (!connector->cmdline_mode.specified)
> + cmdline_mode = &connector->cmdline_mode;
> + if (!cmdline_mode->specified)
> return 0;
>
> + /* Only add a GTF mode if we find no matching probed modes */
> + list_for_each_entry(mode, &connector->modes, head) {
> + if (mode->hdisplay != cmdline_mode->xres ||
> + mode->vdisplay != cmdline_mode->yres)
> + continue;
> +
> + if (cmdline_mode->refresh_specified) {
> + if (mode->vrefresh != cmdline_mode->refresh)
> + continue;
> + }
> +
> + return 0;
> + }
> +
> mode = drm_mode_create_from_cmdline_mode(connector->dev,
> - &connector->cmdline_mode);
> + cmdline_mode);
> if (mode == NULL)
> return 0;
>
>
>
> is not sufficient?
indeed, this was my first idea how to fix this problem, but at the time
drm_helper_probe_add_cmdline_mode is called connector->modes is empty =>
the GTF cmdline mode is still added.
Thanks,
Radek
On Mon, Apr 20, 2015 at 12:57:33PM +0200, Radek Dost?l wrote:
> Hi Chris,
>
> On 04/20/2015 12:48 PM, Chris Wilson wrote:
> >> Unfortunately you can not do that. I already tried. At the time when
> >> > drm_helper_probe_add_cmdline_mode is called EDID informations are not
> >> > yet available.
> > My understanding is that it should be. fb_helper.initial_config does a
> > probe first, and the intel_fb_initial_config() should only keep the
> > active mode.
>
> uff, sorry I am not sure that I follow here - I am not that familiar
> with the whole subsystem.
>
> >> > Only option would be to remove the GTF cmdline mode, when matching mode
> >> > is found in EDID.
> > So basically
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 6350387..9212bec 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -79,13 +79,29 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
> >
> > static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> > {
> > + struct drm_cmdline_mode *cmdline_mode;
> > struct drm_display_mode *mode;
> >
> > - if (!connector->cmdline_mode.specified)
> > + cmdline_mode = &connector->cmdline_mode;
> > + if (!cmdline_mode->specified)
> > return 0;
> >
> > + /* Only add a GTF mode if we find no matching probed modes */
> > + list_for_each_entry(mode, &connector->modes, head) {
> > + if (mode->hdisplay != cmdline_mode->xres ||
> > + mode->vdisplay != cmdline_mode->yres)
> > + continue;
> > +
> > + if (cmdline_mode->refresh_specified) {
> > + if (mode->vrefresh != cmdline_mode->refresh)
> > + continue;
> > + }
> > +
> > + return 0;
> > + }
> > +
> > mode = drm_mode_create_from_cmdline_mode(connector->dev,
> > - &connector->cmdline_mode);
> > + cmdline_mode);
> > if (mode == NULL)
> > return 0;
> >
> >
> >
> > is not sufficient?
>
> indeed, this was my first idea how to fix this problem, but at the time
> drm_helper_probe_add_cmdline_mode is called connector->modes is empty =>
> the GTF cmdline mode is still added.
Can you do a WARN_ON(list_empty(&connector->modes)) here to see at what
point we set up the invalid GTF mode?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Hi Chris,
On 04/20/2015 01:00 PM, Chris Wilson wrote:
> Can you do a WARN_ON(list_empty(&connector->modes)) here to see at what
> point we set up the invalid GTF mode?
sure please see attached patch adding WARN_ON and corresponding output
of dmesg. As mentioned in the original commit, the mode is indeed added
in drm_helper_probe_add_cmdline_mode.
Thanks,
Radek
On Mon, Apr 20, 2015 at 01:20:05PM +0200, Radek Dost?l wrote:
> Hi Chris,
>
> On 04/20/2015 01:00 PM, Chris Wilson wrote:
> > Can you do a WARN_ON(list_empty(&connector->modes)) here to see at what
> > point we set up the invalid GTF mode?
>
> sure please see attached patch adding WARN_ON and corresponding output
> of dmesg. As mentioned in the original commit, the mode is indeed added
> in drm_helper_probe_add_cmdline_mode.
Ah, maybe this on top of the previous try:
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 88f5a74..5d22ca0 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -87,7 +87,7 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
return 0;
/* Only add a GTF mode if we find no matching probed modes */
- list_for_each_entry(mode, &connector->modes, head) {
+ list_for_each_entry(mode, &connector->probed_modes, head) {
if (mode->hdisplay != cmdline_mode->xres ||
mode->vdisplay != cmdline_mode->yres)
continue;
@@ -211,7 +211,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
mode_flags |= DRM_MODE_FLAG_3D_MASK;
list_for_each_entry(mode, &connector->modes, head) {
- mode->status = drm_mode_validate_basic(mode);
+ if (mode->status == MODE_OK)
+ mode->status = drm_mode_validate_basic(mode);
if (mode->status == MODE_OK)
mode->status = drm_mode_validate_size(mode, maxX, maxY
--
Chris Wilson, Intel Open Source Technology Centre
Hi Chris,
On 04/20/2015 01:44 PM, Chris Wilson wrote:
> Ah, maybe this on top of the previous try:
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 88f5a74..5d22ca0 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -87,7 +87,7 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> return 0;
>
> /* Only add a GTF mode if we find no matching probed modes */
> - list_for_each_entry(mode, &connector->modes, head) {
> + list_for_each_entry(mode, &connector->probed_modes, head) {
> if (mode->hdisplay != cmdline_mode->xres ||
> mode->vdisplay != cmdline_mode->yres)
> continue;
> @@ -211,7 +211,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> mode_flags |= DRM_MODE_FLAG_3D_MASK;
>
> list_for_each_entry(mode, &connector->modes, head) {
> - mode->status = drm_mode_validate_basic(mode);
> + if (mode->status == MODE_OK)
> + mode->status = drm_mode_validate_basic(mode);
>
> if (mode->status == MODE_OK)
> mode->status = drm_mode_validate_size(mode, maxX, maxY
perfect - this seems to work :)
Radek