Bug 85977

Summary: [BYT DSI PMIC backlight] Backlight support for Dell Venue 8 Pro
Product: DRI Reporter: Jan-Michael Brummer <jan.brummer>
Component: DRM/IntelAssignee: Shobhit <shobhit.kumar>
Status: CLOSED FIXED QA Contact: Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity: normal    
Priority: medium CC: adamw, bugzilla, darlor, intel-gfx-bugs, jacek.m.danecki, jan.brummer, mike
Version: DRI git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: BYT i915 features: display/backlight
Attachments:
Description Flags
Hack to add backlight support for DV8P
none
i915 opregion of Dell Venue 8 Pro
none
i915_opregion for Aava Inari 8
none
parameter-modifiable version of JMB's hack, rediffed for 3.19rc5
none
J-M B's hack, re-diffed for 3.19 final (by him)
none
J-M B's hack, further re-diffed for 3.20 current (by me) (doesn't appear to work)
none
Kernel config fie tested on AsusT100 none

Description Jan-Michael Brummer 2014-11-06 20:26:48 UTC
Created attachment 109061 [details]
Hack to add backlight support for DV8P

Backlight support for Dell Venue 8 Pro (8086:0f31) tablet is currently missing. The attached patch adds support for it in a ugly way and needs to be cleanup by someone more familiar with intel graphic. It is based on a hack made by Jon Pry for his Asus T100.
Comment 1 Jani Nikula 2014-11-07 08:15:13 UTC
Shobhit is working on it.
Comment 2 Jani Nikula 2014-11-07 08:16:06 UTC
Please attach /sys/kernel/debug/dri/0/i915_opregion.
Comment 3 Jan-Michael Brummer 2014-11-07 17:50:05 UTC
Created attachment 109099 [details]
i915 opregion of Dell Venue 8 Pro
Comment 4 Jan-Michael Brummer 2014-11-18 19:53:28 UTC
What's the current status of Shobhits proper backlight implementation? Is there any patch available that we can test on our devices yet?
Comment 5 Michael Shigorin 2014-11-26 21:36:51 UTC
(In reply to Jan-Michael Brummer from comment #0)
> Created attachment 109061 [details]
> Hack to add backlight support for DV8P
> 
> Backlight support for Dell Venue 8 Pro (8086:0f31) tablet is currently
> missing. The attached patch adds support for it in a ugly way and needs to
> be cleanup by someone more familiar with intel graphic. It is based on a
> hack made by Jon Pry for his Asus T100.

Got Aava Inari 8 (8086:0f31 too) with backlight level fixed at boot (and a weird "milk flooding" when DPMS kicks in...) -- should it be safe to try your patch given the same SoC but quite different panel and probably firmware (64-bit one)?

The magic constants in both
https://github.com/jonpry/t100_patches/blob/master/3.17.patch
and
https://github.com/AdamWill/baytrail-m/blob/master/kernel/baytrail-backlight.patch
look the same at least and the panel specific delay handling seems to have been dropped in the latter but that's about as much as I can read in interdiff.
Comment 6 Adam Williamson 2014-11-26 21:38:34 UTC
Michael: the patch in Fedlet is this patch combined with an additional one Jan-Michael sent to me by email which makes the change conditional on a kernel parameter.
Comment 7 Michael Shigorin 2014-11-26 21:43:15 UTC
Thank you, Adam.
Comment 8 Michael Shigorin 2014-11-26 22:09:18 UTC
Created attachment 110092 [details]
i915_opregion for Aava Inari 8

Forgot to attach /sys/kernel/debug/dri/0/i915_opregion
Comment 9 Michael Shigorin 2014-11-28 15:41:09 UTC
As a matter of fact, the patch works on Inari 8 as well.
Comment 10 Michael Shigorin 2014-12-02 10:07:24 UTC
Does anyone else get this BUG with i915.force_backlight_pmic=1?
(setting it back to 0 makes this disappear)

BUG: scheduling while atomic: Xorg/589/0x00000002
[...]
[  321.034563] Call Trace:
[  321.034580]  [<ffffffff8159679f>] dump_stack+0x4e/0x71
[  321.034595]  [<ffffffff8159386e>] __schedule_bug+0x45/0x53
[  321.034609]  [<ffffffff81599518>] __schedule+0x848/0x860
[  321.034623]  [<ffffffff81599694>] schedule+0x24/0x70
[  321.034637]  [<ffffffff8159c57c>] schedule_hrtimeout_range_clock+0x11c/0x150
[  321.034653]  [<ffffffff810d1a10>] ? update_rmtp+0x70/0x70
[  321.034669]  [<ffffffff810d24bf>] ? hrtimer_start_range_ns+0xf/0x20
[  321.034684]  [<ffffffff8159c5be>] schedule_hrtimeout_range+0xe/0x10
[  321.034698]  [<ffffffff810cf11b>] usleep_range+0x3b/0x40
[  321.034728]  [<ffffffffa002314f>] __i2c_dw_enable+0x2f/0xa0 [i2c_designware_core]
[  321.034756]  [<ffffffffa0023c7c>] i2c_dw_xfer+0x1bc/0x540 [i2c_designware_core]
[  321.034772]  [<ffffffff8144ac6d>] ? __i2c_transfer+0x6d/0x280
[  321.034786]  [<ffffffff8144ac6d>] __i2c_transfer+0x6d/0x280
[  321.034801]  [<ffffffff8144b530>] i2c_transfer+0x50/0xb0
[  321.034815]  [<ffffffff8144bbda>] i2c_master_send+0x3a/0x50
[  321.034831]  [<ffffffff81413635>] regmap_i2c_write+0x15/0x40
[  321.034845]  [<ffffffff8140f2cd>] _regmap_raw_write+0x76d/0x7a0
[  321.034860]  [<ffffffff8140f4a1>] _regmap_bus_raw_write+0x61/0x90
[  321.034874]  [<ffffffff8140e39a>] _regmap_write+0x7a/0x150
[  321.034888]  [<ffffffff8141013a>] regmap_write+0x4a/0x70
[  321.034904]  [<ffffffff814255f1>] intel_soc_pmic_writeb+0x21/0x30
[  321.034999]  [<ffffffffa042d296>] vlv_pmic_disable_backlight+0x26/0x30 [i915]
[  321.035089]  [<ffffffffa042e1c6>] intel_panel_disable_backlight+0x96/0xc0 [i915]
[  321.035176]  [<ffffffffa0423b17>] intel_dsi_post_disable+0x57/0x500 [i915]
[  321.035260]  [<ffffffffa03f7789>] ? intel_disable_pipe+0x129/0x2f0 [i915]
[  321.035343]  [<ffffffffa0401bf4>] i9xx_crtc_disable+0xf4/0x6f0 [i915]
[  321.035428]  [<ffffffffa03fbc61>] intel_crtc_control+0x51/0x120 [i915]
[  321.035519]  [<ffffffffa03fbd97>] intel_crtc_update_dpms+0x67/0x80 [i915]
[  321.035603]  [<ffffffffa0406791>] intel_connector_dpms+0x41/0x70 [i915]
[  321.035657]  [<ffffffffa02f4edc>] drm_mode_obj_set_property_ioctl+0x35c/0x370 [drm]
[  321.035711]  [<ffffffffa02f4f1b>] drm_mode_connector_property_set_ioctl+0x2b/0x30 [drm]
[  321.035754]  [<ffffffffa02e5a56>] drm_ioctl+0x1f6/0x680 [drm]
[  321.035808]  [<ffffffffa02f4ef0>] ? drm_mode_obj_set_property_ioctl+0x370/0x370 [drm]
[  321.035835]  [<ffffffff81021ad9>] ? init_fpu+0x59/0xc0
[  321.035851]  [<ffffffff811cfe4e>] do_vfs_ioctl+0x7e/0x520
[  321.035865]  [<ffffffff811da044>] ? __fget+0x74/0xb0
[  321.035880]  [<ffffffff811d0381>] SyS_ioctl+0x91/0xb0
[  321.035895]  [<ffffffff8159ddac>] ? int_check_syscall_exit_work+0x34/0x3d
[  321.035910]  [<ffffffff8159db29>] system_call_fastpath+0x12/0x17
Comment 11 Adam Williamson 2015-01-24 23:26:58 UTC
For those playing along at home, I see on intel-gfx that Shobhit does seem to be working on this stuff. This is the latest version of his RFC patch set:

http://lists.freedesktop.org/archives/intel-gfx/2015-January/058756.html

Doesn't look like it actually does backlight control yet, only enable/disable.

Thanks for the work Shobhit!
Comment 12 Adam Williamson 2015-01-24 23:33:21 UTC
Also note that Jan-Michael's hack needs a very minor rediff for 3.19, change this line in the patch:

 int intel_panel_setup_backlight(struct drm_connector *connector)

to this:

 int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)

and it'll apply (HAND RE-DIFFING FOR TEH WIN)
Comment 13 Adam Williamson 2015-01-25 02:54:07 UTC
Created attachment 112786 [details] [review]
parameter-modifiable version of JMB's hack, rediffed for 3.19rc5

OK, so the patch actually needs a bit more rediffing for 3.19rc5. here's the one I'm using; it's a combination of this one and a follow-up JMB sent me via email which allows it to be toggled with a parameter. Set 'i915.force_backlight_pmic=1' to use the hack, otherwise the driver will act as normal.
Comment 14 Adam Williamson 2015-01-25 02:57:21 UTC
damnit, that version doesn't work either, i'll attach another later.
Comment 15 Michael Shigorin 2015-01-26 12:52:16 UTC
Did you look at 3.19rc6, Adam? (my -rc4 attempts have been undermined by brcmfmac-related lockups so far, trying to come up with another bugreport)
Comment 16 Adam Williamson 2015-01-26 16:04:58 UTC
I got it to compile but it hangs on KMS activation, I suspect my hacks to get pipe were wrong. I was planning to build without the patch today.

I don't think anything's changed between rc5 and rc6, I've been looking directly at Linus' tree.
Comment 17 Shobhit 2015-02-04 11:45:13 UTC
(In reply to Adam Williamson from comment #11)
> For those playing along at home, I see on intel-gfx that Shobhit does seem
> to be working on this stuff. This is the latest version of his RFC patch set:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-January/058756.html
> 
> Doesn't look like it actually does backlight control yet, only
> enable/disable.
> 
> Thanks for the work Shobhit!

Backlight control will need a backlight class driver and I am just waiting for the current approach to get accepted before implementing that. Implemented in current patch set enable/disable to atleast save power during suspend/resume.
Comment 18 Adam Williamson 2015-02-20 21:22:20 UTC
http://lists.freedesktop.org/archives/intel-gfx/2015-February/060160.html seems to be Shobhit's latest upstream state of the art.

J-M sent me a version of his hack re-diffed against 3.19...which again fails to apply against current 3.20-pre. I re-diffed it again for 3.20-pre and it applies and boots, but doesn't actually seem to control anything.

So, I'm gonna do a fedlet kernel build with Shobhit's latest patch instead and see how that goes; that might be something other folks could use to test, if they're running Fedlet or a variant of it or something.
Comment 19 Michael Shigorin 2015-02-21 14:12:27 UTC
(In reply to Adam Williamson from comment #18)
> I re-diffed it again for 3.20-pre and it applies and boots
Care to attach? ;-)

> but doesn't actually seem to control anything.
...that's my experience with code in 3.19 as well.  There was some hope that it might eliminate slowly whitening screen (see comment 1) but it's still around.
Comment 20 Adam Williamson 2015-02-21 15:21:30 UTC
Created attachment 113722 [details] [review]
J-M B's hack, re-diffed for 3.19 final (by him)
Comment 21 Adam Williamson 2015-02-21 15:22:02 UTC
Created attachment 113723 [details] [review]
J-M B's hack, further re-diffed for 3.20 current (by me) (doesn't appear to work)
Comment 22 Michael Shigorin 2015-02-25 00:10:53 UTC
(In reply to Adam Williamson from comment #20)
> J-M B's hack, re-diffed for 3.19 final (by him)
This one does work for me, thanks.
Comment 23 Shobhit 2015-03-13 14:07:31 UTC
Latest patch sets for trying out. Both panel enable and back-light enable/disable is supported. Last patch(9) is more of quick test. Once I fix that properly, back-light brightness control should also work - 

http://lists.freedesktop.org/archives/intel-gfx/2015-March/061908.html
Comment 24 Adam Williamson 2015-03-16 23:04:28 UTC
Thanks Shobhit! Should CONFIG_PWM_CRC depend on CONFIG_PWM ? I don't think it does at present; I seemed to manage to get a build to run with CONFIG_PWM_CRC=y but CONFIG_PWM unset, and it failed because pwm_config_alternate() was not defined (because its definition in pwm.h is inside a #if IS_ENABLED(CONFIG_PWM) block).
Comment 25 Shobhit 2015-03-17 06:32:25 UTC
Yes CONFIG_PWM_CRC depends on CONFIG_PWM and additionally on CONFIG_INTEL_SOC_PMIC. CONFIG_PWM_CRC is in drivers/pwm/Kconfig which needs CONFIG_PWM enabled first to be able to select CONFIG_PWM_CRC.
Comment 26 Kit 2015-04-13 20:50:11 UTC
(In reply to Shobhit from comment #23)
> Latest patch sets for trying out. Both panel enable and back-light
> enable/disable is supported. Last patch(9) is more of quick test. Once I fix
> that properly, back-light brightness control should also work - 
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-March/061908.html

Shobhit, I tried to apply your patch against 4.0 kernel. Unfortunately, I get
[    3.205922] crystal_cove_pwm: probe of crystal_cove_pwm failed with error -22
on Acer Aspire Switch 10 with Intel Atom (BayTrail CR?) Z3735F. J-M B's hack never worked for me no matter what kernel version. Is there any debug info I can provide?
Comment 27 Shobhit 2015-04-14 04:19:35 UTC
Not sure if it is BYT-CR. But if it is then LPSS PWM is used on those. I think the LPSS PWM driver is already upstreamed(drivers/pwm/pwm-lpss.c. We would have to use that PWM chip in this case instead of PMIC based one.

if(dev_priv->vbt.dsi.config->pwm_blc == 1)
then it is confirmed to be using LPSS instead of PMIC
Comment 28 Kit 2015-04-14 12:51:49 UTC
(In reply to Shobhit from comment #27)
> Not sure if it is BYT-CR. But if it is then LPSS PWM is used on those. I
> think the LPSS PWM driver is already upstreamed(drivers/pwm/pwm-lpss.c. We
> would have to use that PWM chip in this case instead of PMIC based one.
> 
> if(dev_priv->vbt.dsi.config->pwm_blc == 1)
> then it is confirmed to be using LPSS instead of PMIC

Yes, it's using LPSS then.
Comment 29 Shobhit 2015-04-24 15:53:54 UTC
Created attachment 115306 [details]
Kernel config fie tested on AsusT100

Latest series - 
http://lists.freedesktop.org/archives/intel-gfx/2015-April/065313.html

I have also attached my config file for testing. Using this AsusT100 works fine using the /sys/class/backlight/intel_backlight interface
Comment 30 Luka Karinja 2015-06-16 18:33:55 UTC
(In reply to Shobhit from comment #29)
> Created attachment 115306 [details]
> Kernel config fie tested on AsusT100
> 
> Latest series - 
> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065313.html
> 
> I have also attached my config file for testing. Using this AsusT100 works
> fine using the /sys/class/backlight/intel_backlight interface

Hello. I have a Asus T100TAF wich has Z3735F.
Tried the previously mentioned patch but brightness control is not working.

Furter examination got me to the realisation that it needs LPSS instead of PMIC for the backlight brightness control

Is there a patch aviabile for the Z3735F?
Comment 31 Shobhit 2015-06-17 02:46:45 UTC
(In reply to Luka Karinja from comment #30)
> (In reply to Shobhit from comment #29)
> > Created attachment 115306 [details]
> > Kernel config fie tested on AsusT100
> > 
> > Latest series - 
> > http://lists.freedesktop.org/archives/intel-gfx/2015-April/065313.html
> > 
> > I have also attached my config file for testing. Using this AsusT100 works
> > fine using the /sys/class/backlight/intel_backlight interface
> 
> Hello. I have a Asus T100TAF wich has Z3735F.
> Tried the previously mentioned patch but brightness control is not working.
> 
> Furter examination got me to the realisation that it needs LPSS instead of
> PMIC for the backlight brightness control
> 
> Is there a patch aviabile for the Z3735F?

Not yet. I need to first get the PMIC based patches merged and that's when I am coming back for this LPSS. This *is* in my bin list.
Comment 32 Jan-Michael Brummer 2015-08-15 13:25:38 UTC
@Shobhit: I've tested your patches on Dell Venue 8 Pro but it is not working. Log shows:

[    4.369291] [drm:intel_dsi_init [i915]] *ERROR* Failed to own gpio for panel control
[    4.369889] [drm:pwm_setup_backlight [i915]] *ERROR* Failed to own the pwm chip

This is caused by the soc pmic driver loaded *after* i915, and that's why the pwm list is empty when requesting it. I've added it directly to the kernel and tested i915 as build-in and module, but it doesn't change the effect. Do you have a helping hand?
Comment 33 Adam Williamson 2015-08-17 05:27:18 UTC
Confirming I see the same messages with my latest Fedlet kernel build (to which I applied the latest available patch series from the mailing list).
Comment 34 Shobhit 2015-08-18 06:25:03 UTC
This will happen because this device as I remember from previous discussions uses LPSS for backlight and not PMIC and the PMIC device also might be different and the ACPI enumeration for the same might not be available. Hence it is likely that the we will not get the gpio and pwn chips. Today the PWM supported is only for CRC PMIC which is not there on this platform. There needs to be some work done to use LPSS based PWM. That driver is already there in the kernel. Plumbing in i915 PWM support is pending.
Comment 35 Jan-Michael Brummer 2015-08-18 06:29:50 UTC
At least my variant of the DV8P uses the PMIC for backlight control. After modifying the driver to always call setup_backlight it allows me to disable the screen. Your logic withinn PMIC module also matches my first patch (added as first message in this bug issue).
Comment 36 Shobhit 2015-08-18 06:55:10 UTC
(In reply to Jan-Michael Brummer from comment #35)
> At least my variant of the DV8P uses the PMIC for backlight control. After
> modifying the driver to always call setup_backlight it allows me to disable
> the screen. Your logic withinn PMIC module also matches my first patch
> (added as first message in this bug issue).

Ok, I got confused in the previous replies. two devices are being mixed in this bug discussion. Can you try with Linux-next where all the patches are merged and one of the config file already attached as reference. I have tested with this config file and it works.
Comment 37 Jan-Michael Brummer 2015-08-19 21:02:40 UTC
I've tried linux-next with your latest config file (+ y to emmc) but it produces the same error. PWMs are started after i915. That's understandable as there is no real dependency within i915 to the pwm crc module.
Comment 38 Jani Nikula 2016-04-19 18:07:14 UTC
This bug is about PMIC backlight control. The code to support this, both on BYT and CHV, has been upstream for a while now, so I'm closing the bug. Thanks for the report.

There still remains the issue with probe ordering: if i915 gets probed before all the related pmic/i2c drivers, pwm_get() fails as described in comment #32. If you want to file a new bug for that, I don't mind.

As a workaround, these config options worked for me on Surface 3 PMIC backlight:

CONFIG_PWM=y
CONFIG_PWM_CRC=y
CONFIG_I2C_DESIGNWARE_PLATFORM=y
CONFIG_I2C_DESIGNWARE_PCI=y
CONFIG_INTEL_SOC_PMIC=y
CONFIG_DRM_I915=m
Comment 39 Adam Williamson 2016-04-19 18:18:13 UTC
Thanks, Jani. I haven't had time / motivation to play with my fedlet for a while unfortunately.
Comment 40 Jani Nikula 2016-06-29 14:03:12 UTC
(In reply to Jani Nikula from comment #38)
> There still remains the issue with probe ordering: if i915 gets probed
> before all the related pmic/i2c drivers, pwm_get() fails as described in
> comment #32. If you want to file a new bug for that, I don't mind.

That has been filed as bug 96571.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.