I have a sinking feeling it could be related to the intel opregion spec, I had problems with display artifacting before until intel_opregion.c was patched in 4.9.21. Looking at the diff, it appears that some more stuff was changed. I will compile with a backported 4.11.1 configuration and see if things improve or not.

Created attachment 131420 [details] [review]
The folllowing patch seems to fix the issue!

Ah, it was as I expected. The following patch appears to fix the issue! It looks like these exact same lines from the previous bug I filed at https://bugs.freedesktop.org/show_bug.cgi?id=97918 has come back to haunt us in a weird way. But the fix seems to really work!

--- a/drivers/gpu/drm/i915/intel_opregion.c	2017-05-13 16:19:49.000000000 -0400
+++ b/drivers/gpu/drm/i915/intel_opregion.c	2017-05-19 22:44:29.891397327 -0400
@@ -1021,23 +1021,6 @@
 	return err;
 }
 
-static int intel_use_opregion_panel_type_callback(const struct dmi_system_id *id)
-{
-	DRM_INFO("Using panel type from OpRegion on %s\n", id->ident);
-	return 1;
-}
-
-static const struct dmi_system_id intel_use_opregion_panel_type[] = {
-	{
-		.callback = intel_use_opregion_panel_type_callback,
-		.ident = "Conrac GmbH IX45GM2",
-		.matches = {DMI_MATCH(DMI_SYS_VENDOR, "Conrac GmbH"),
-			    DMI_MATCH(DMI_PRODUCT_NAME, "IX45GM2"),
-		},
-	},
-	{ }
-};
-
 int
 intel_opregion_get_panel_type(struct drm_i915_private *dev_priv)
 {
@@ -1063,15 +1046,5 @@
 		return -ENODEV;
 	}
 
-	/*
-	 * So far we know that some machined must use it, others must not use it.
-	 * There doesn't seem to be any way to determine which way to go, except
-	 * via a quirk list :(
-	 */
-	if (!dmi_check_system(intel_use_opregion_panel_type)) {
-		DRM_DEBUG_KMS("Ignoring OpRegion panel type (%d)\n", ret - 1);
-		return -ENODEV;
-	}
-
 	return ret - 1;
 }

Please attach the output of dmidecode.

Created attachment 131582 [details]
Here are the DMI table contents from dmidecode.

Here are the DMI table contents from dmidecode.

Sorry for the delay. The fix would be adding the system to the quirk table to use opregion panel type on it.

Created attachment 132605 [details] [review]
Add  Spectre x360 Convertible to OpRegion whitelist

If the fix is to add system to whitelist please add attached one too. It's similar (but not identical) as above.

moving back to REOPENED provided information requested

Please test the patch https://patchwork.freedesktop.org/patch/170500/
and check if it works.

Patch works good for me, thanks. Are there any downsides of using OpRegion panel type instead of default?

I spoke too early. I managed to trigger some kind of flickering never seen before.

In logs there was only:
[drm] Reducing the compressed framebuffer size. This may lead to less power savings than a non-reduced-size. Try to increase stolen memory size if available in BIOS

but I'm not sure if it's related. It happened only once and I didn't reproduced it yet.

Is there a way to get rid of useless lines like:

[drm:drm_mode_addfb2 [drm]] [FB:65]

from debugging logs? I can't enable debugging permanently because then I get one hundred lines per minute like this which makes logs completely unusable. Therefore usually when I spot a bug I have logging disabled and when I enable it's to late as I can't reproduce previous bug in reasonable time so I have to disable debugging again...

Is there any difference in the kind of flickers that you observe before and after applying the patch onto drm-tip?

The logs in comment 10 appear to be from frame-buffer compression which could be different issue. Let us limit the scope of this bug to the original flicker that is being observed and open a new issue for any new issues.

Those were different but happened only once as for now, could be unrelated. The old flickering is gone.

Can you answer if using OpRegion has any downsides? Flickering on never kernels is quite rare and I don't want be thrown under the bus in future with those changes. Also can you answer my question about logs above?

I'm sorry for disturbing you.

One more thing. As we don't know what really causes this bug, do you think it's possible that it's connected with frame-buffer compression?

One thing that I would do to check if it is a fbc related flicker is to boot with command line parameter i915.enable_fbc=0 and check if the flicker happens. Since you no longer see the original flicker, I would suggest to raise a different bug for the new flicker that you are observing with relevant logs.
@Nicholas: What is the current drm debug level that you are using? can you do
"echo 0x6 > /sys/module/drm/parameters/debug" and see if you are still observing FB logs?
I do not see any down sides by adding to the quirk table. Jani Nikula can you confirm?

Please add drm.debug=14 module parameter, attach dmesg from the boot, *without* the patch.

Please attach /sys/kernel/debug/dri/0/i915_vbt

Please also check the dmesg *with* the patch, and see what the dmesg has about panel type. It'll be one of these debug messages:

DRM_DEBUG_KMS("Panel type: %d (OpRegion)\n", panel_type);
DRM_DEBUG_KMS("Panel type: %d (VBT)\n", panel_type);

i.e. I want to know what the panel type reported by opregion is.

Everything was attached in earlier bug report here: https://bugs.freedesktop.org/show_bug.cgi?id=97918

I used drm.debug=0xe 

The "new" flickering was observed only once so I bet it was unrelated, please ignore.

If using OpRegion has no downsides, then in my opinion the patch is ready and issue fixed.

(In reply to Mark Spencer from comment #17)
> If using OpRegion has no downsides, then in my opinion the patch is ready
> and issue fixed.

I'd prefer understanding what exactly in this change fixes the flickering issues for you first.

Also, what makes this a regression? There was something we tried (which fixed stuff for you, but regressed for others) but we reverted back to how things were.

This patch forces my and similar systems to use panel type from OpRegion:

[drm] Using panel type from OpRegion on HP Spectre x360 Convertible


Actually that was you who proposed this:

(In reply to Jani Nikula from comment #5)
> Sorry for the delay. The fix would be adding the system to the quirk table
> to use opregion panel type on it.

This was pretty extensively discussed in https://bugs.freedesktop.org/show_bug.cgi?id=97918

I have to say that I didn't opened this bug and I didn't called it regression. I joined discussion because I saw your comment quoted above about fixing this which is exactly what I proposed many months ago which is also the same what Radhakrishna Sripada proposed in his patch (basing on my suggestion I guess).

Regression or not, this issue can be solved easily. If it's true that it doesn't has any downsides I personally don't see reason for not doing it.

In reply to Radhakrishna Sripada from comment #15)
> can you do "echo 0x6 > /sys/module/drm/parameters/debug" and see if you   > are still observing FB logs?

It doesn't help. Logs are still spammy as hell.

(In reply to Mark Spencer from comment #20)
> This patch forces my and similar systems to use panel type from OpRegion:
> 
> [drm] Using panel type from OpRegion on HP Spectre x360 Convertible

I'm just trying to look at the differences in the info used from the VBT based on the difference in panel type, to check what exactly is the change that causes flicker. The fact that it fixes the flicker may be purely coincidental, and we may have a bug someplace else. That's all.

Created attachment 133445 [details]
dmesg linux 4.12.5 without patch

Created attachment 133446 [details]
dmesg linux 4.12.5 with patch

Created attachment 133447 [details]
i915vbt

A attached what you asked for. Kernel 4.12.5 is latest I can currently get.

Actual flickering happened at the very end of dmesg without patch log.

excerpt from dmesg without patch:

[drm:intel_opregion_get_panel_type [i915]] Ignoring OpRegion panel type (0)
[drm:intel_bios_init [i915]] Panel type: 14 (VBT)

excerpt from dmesg with patch:

[drm] Using panel type from OpRegion on HP Spectre x360 Convertible
[drm:intel_bios_init [i915]] Panel type: 0 (OpRegion)

I hope it helps you.

After examining the VBT file the most likely difference between panel type #14 and Panel type #0 is the backlight PWM frequency. Panel #14 has pwm frequency defined at 200 Hz and Panel #0 has a frequency of 10000 Hz. The flicker most likely observed by the user is backlight induced.

Other differences are DRRS mode and some panel power sequence differences. Neither should cause a flicker after boot.

(In reply to Clinton Taylor from comment #25)
> After examining the VBT file the most likely difference between panel type
> #14 and Panel type #0 is the backlight PWM frequency. Panel #14 has pwm
> frequency defined at 200 Hz and Panel #0 has a frequency of 10000 Hz. The
> flicker most likely observed by the user is backlight induced.

Yeah I looked at the diff, but we always try to use the frequency that BIOS set up before using what's in VBT...

From intel-gpu-tools could you run "intel_reg dump" with and without the patch and provide us the outputs?

Created attachment 133512 [details]
intel_req_with_patch

Created attachment 133513 [details]
intel_req_without_patch

As requested.

Could you try the following diff without the patch mentioned in comment 8.

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 82b144cdfa1d..1605923100c6 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -259,6 +259,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
                break;
        }
 
+       dev_priv->vbt.drrs_type = DRRS_NOT_SUPPORTED;
        lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
        if (!lvds_lfp_data)
                return;

Your patch doesn't apply in it's current form. Did you mean this:

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 82b144cdfa1d..1605923100c6 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -259,6 +259,7 @@
 		break;
 	}
 
+	dev_priv->vbt.drrs_type = DRRS_NOT_SUPPORTED;
 	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
 	if (!lvds_lfp_data)
 		return;

Yes that code change should do. We are trying to see if DRRS is causing the flickers. Could you watch on /sys/kernel/debug/dri/0/i915_drrs_status with low granularity(around 0.1s) and report out any correlation between the flicker, output of the debugfs i915_drrs_status.

My output is only:

cat /sys/kernel/debug/dri/0/i915_drrs_status

CRTC 1:  eDP-1:
        VBT: DRRS_type: None

        DRRS Supported : No

It's static.

Do you see a flicker when you disable drrs and not include the system in the quirk i.e, drm-tip + change in comment 31?

Created attachment 133578 [details]
intel_reg_dump #31 patch

Sorry for the delay but I wanted to test this for some time. It seems flickering is gone with https://bugs.freedesktop.org/show_bug.cgi?id=101111#c31 patch alone.

Here's excerpt from logs:

[drm:intel_opregion_setup [i915]] Found valid VBT in ACPI OpRegion (Mailbox #4)
[drm:intel_opregion_get_panel_type [i915]] Ignoring OpRegion panel type (0)

I attached new intel_reg dump just in case.

I think that we can assume that this patch solves the issue. Thank you for all your help!

The patch posted in comment 31 was a hack to see if drrs is causing the flicker as that is also one of the differences observed between the two panels. I would be coming up with a new patch and would be posting the link.

DRRS will be enabled based on the panel's capability claim for two different clock rates (High and Low) along with platform DRRS capability through VBT.

To root cause this, We might want to check whether connector->probed_modes has two entries for same mode with only varying clock and whether drrs code is configuring the refresh rate as per the downclock received from the connector->probed_modes.

Could you attach the outputs of /sys/kernel/debug/dri/0/i915_display_info
and /sys/class/drm/card0-eDP-1/edid

Created attachment 133730 [details]
i915_display_info

Created attachment 133731 [details]
edid

As requested

Do you observe any correlation between the flicker and occurrence of the pattern "[drm:intel_dp_set_drrs_state.isra.16 [i915]] eDP Refresh Rate set to : 40Hz" in the dmesg logs?

Yeah, it's very possible those are connected. It occurred at the end of previous log I posted:

https://bugs.freedesktop.org/attachment.cgi?id=133445

quite same time as actual flickering occurred. Also I have no idea why 40hz refresh rate is available here.

With the configuration where you observe flickers, can you boot with cmd line parameter i915.enable_psr=0 and report if you see any flicker?

This bug is strictly connected to i915.enable_psr=1 setting and it dates back to 4.6 kernel when it was enabled by default for BDW. See:
https://bugs.freedesktop.org/show_bug.cgi?id=95176

For example here is report which connects flickering with 40hz refresh rate, same thing which you asked me before:
https://bugs.freedesktop.org/show_bug.cgi?id=95176#c26

It affected different machines and it's probably fixed for some of them.

This bug report and https://bugs.freedesktop.org/show_bug.cgi?id=97918 are about similar problem specific to Hp spectre x360 machine (could be connected to others but who knows).

There are no such issues with i915.enable_psr=0.

(In reply to Mark Spencer from comment #44)
> There are no such issues with i915.enable_psr=0.

And how about *without* i915.enable_psr parameter being explicitly set?

Well, i915.enable_psr defaults to 0 since 4.9.3 on HSW/BDW, see https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=2bdb638de2fc9c2c1aa15ecdf28e295786a02ea1 so i915.enable_psr=0 parameter is redundant unless I'm missing something.

drrs and psr are mutually exclusive features. They are not supposed to be enabled at the same time. I will be sending in a patch to disable drrs when psr is active.

Can you try and see if this patch fixes the issue?
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d3e5fdf0d2fa..ef6e32573cab 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5557,6 +5557,11 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp,
                return;
        }
 
+       if (dev_priv->psr.enabled == intel_dp) {
+               DRM_DEBUG_KMS("PSR active. Disabling DRRS.\n");
+               return;
+       }
+
        mutex_lock(&dev_priv->drrs.mutex);
        if (WARN_ON(dev_priv->drrs.dp)) {
                DRM_ERROR("DRRS already enabled\n");

(In reply to Mark Spencer from comment #46)
> Well, i915.enable_psr defaults to 0 since 4.9.3 on HSW/BDW, see
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/
> commit/?id=2bdb638de2fc9c2c1aa15ecdf28e295786a02ea1 so i915.enable_psr=0
> parameter is redundant unless I'm missing something.

So it now defaults to disabled for a reason. The parameter is "unsafe" for a reason. If you enable PSR when it's not enabled by default for your platform, all bets are off. We don't support it.

(In reply to Radhakrishna Sripada from comment #48)
> +       if (dev_priv->psr.enabled == intel_dp) {

Huh?

@Jani Nikula
PSR is disabled because it causes issues such as this among others which we try to fix here, so it can be enabled again somewhere in future. It seems we finally found source of this issue: DRRS is enabled with PSR which shouldn't be allowed. It puzzles me why after year long discussions, two bug reports, asking for providing more and more logs you finally come to conclusion that it isn't supported.


> +       if (dev_priv->psr.enabled == intel_dp) {

Is something wrong with this? Should I test it?

In the current context the code change is equivalent to the diff posted below as dev_priv->psr.enabled is a pointer to struct intel_dp. @Mark: you can use this diff to try out. 

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d3e5fdf0d2fa..dc7a6721e0dd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5469,11 +5469,6 @@ static void intel_dp_set_drrs_state(struct drm_i915_private *dev_priv,
                return;
        }
 
-       /*
-        * FIXME: This needs proper synchronization with psr state for some
-        * platforms that cannot have PSR and DRRS enabled at the same time.
-        */
-
        dig_port = dp_to_dig_port(intel_dp);
        encoder = &dig_port->base;
        intel_crtc = to_intel_crtc(encoder->base.crtc);
@@ -5557,6 +5552,11 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp,
                return;
        }
 
+       if (dev_priv->psr.enabled != NULL) {
+               DRM_DEBUG_KMS("PSR active. Disabling DRRS.\n");
+               return;
+       }
+
        mutex_lock(&dev_priv->drrs.mutex);
        if (WARN_ON(dev_priv->drrs.dp)) {
                DRM_ERROR("DRRS already enabled\n");

(In reply to Radhakrishna Sripada from comment #52)
> In the current context the code change is equivalent to the diff posted
> below as dev_priv->psr.enabled is a pointer to struct intel_dp. @Mark: you
> can use this diff to try out. 

I tested this patch for a while and there is no flickering.

Posted the changes onto ML. Link for the patch: https://patchwork.freedesktop.org/patch/174396/

*** Bug 97918 has been marked as a duplicate of this bug. ***

(In reply to Mark Spencer from comment #51)
> @Jani Nikula
> PSR is disabled because it causes issues such as this among others which we
> try to fix here, so it can be enabled again somewhere in future. It seems we
> finally found source of this issue: DRRS is enabled with PSR which shouldn't
> be allowed. It puzzles me why after year long discussions, two bug reports,
> asking for providing more and more logs you finally come to conclusion that
> it isn't supported.

There's a distinction between a) enabling PSR to actively work on fixing issues related to it, and b) enabling PSR because some forum said you should and then filing issues here because it doesn't work out for you.

We've disabled PSR by default because there have been so many issues with it, and we can't handle the influx of bugs. We keep fixing stuff in the background, but at the same time we close PSR bugs as not supported. For example, in the case of this bug, it took until comment #44 to realize this is a PSR related bug. You also insisted on merging what would have been the completely wrong patch, and would have made debugging future PSR problems even harder. My gut feeling in comment #21 was spot on.

So there'll be a happy ending to this bug, the driver will get a bit better now, and we're a bit closer to enabling PSR, thank you for that. But perhaps you'll see that there's a reason we generally close what we think are category b) bugs as not supported.

(In reply to Jani Nikula from comment #56)
> There's a distinction between a) enabling PSR to actively work on fixing
> issues related to it, and b) enabling PSR because some forum said you should
> and then filing issues here because it doesn't work out for you.


I didn't fill myself any new issue about this. I only joined discussion which already started here and on previous related bugreport. Also I have PSR in working state for more than a year with dirtyfix I created myself. Meanwhile I provided all information I can everytime one of developers asked. I recompiled my kernel with every patch that was proposed, tested it and posted logs.


> We've disabled PSR by default because there have been so many issues with
> it, and we can't handle the influx of bugs. We keep fixing stuff in the
> background, but at the same time we close PSR bugs as not supported. For
> example, in the case of this bug, it took until comment #44 to realize this
> is a PSR related bug. You also insisted on merging what would have been the
> completely wrong patch, and would have made debugging future PSR problems
> even harder. My gut feeling in comment #21 was spot on.


The problem is that I didn't submit those two bugreports myself and they weren't very clear in my opinion. I tried bring some new information but it was probably lost in translation. I think PSR was enabled in every log I posted. I wrote explicitly about PSR in https://bugs.freedesktop.org/show_bug.cgi?id=97918#c22 in February. Honestly I thought that devs knew this issue is strictly connected to PSR.

Also that was You who insisted on merging wrong solution https://bugs.freedesktop.org/show_bug.cgi?id=101111#c5 . My first post in this topic is after that comment of yours.

> So there'll be a happy ending to this bug, the driver will get a bit better
> now, and we're a bit closer to enabling PSR, thank you for that. But perhaps
> you'll see that there's a reason we generally close what we think are
> category b) bugs as not supported.

I agree with you but I hope you reconsider my role after reading my explanations above. I believe everything is clear now. Many thanks for you and all the rest of the devs who helped.

The patch mentioned in comment 54 got merged onto drm-tip
https://cgit.freedesktop.org/drm-tip/commit/?id=da83ef85f5356b4bdf534add20fb34dcc6b53fc1

Commit: da83ef85f5356b4bdf534add20fb34dcc6b53fc1
drm/i915: Do not enable DRRS when PSR is enabled

Thank you. I guess it's fixed then.

(In reply to Mark Spencer from comment #59)
> Thank you. I guess it's fixed then.

Apologies for being grumpy above. The first order of business on our side, including myself, should be checking that all module params are at defaults before jumping into conclusions one way or the other.

Thanks for your patience with the bug. All's well that ends well?