Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
Hi Dmitry, On 5/7/24 3:32 PM, Dmitry Baryshkov wrote: > On Mon, May 06, 2024 at 01:49:17PM +0200, Hans de Goede wrote: >> Hi dma-buf maintainers, et.al., >> >> Various people have been working on making complex/MIPI cameras work OOTB >> with mainline Linux kernels and an opensource userspace stack. >> >> The generic solution adds a software ISP (for Debayering and 3A) to >> libcamera. Libcamera's API guarantees that buffers handed to applications >> using it are dma-bufs so that these can be passed to e.g. a video encoder. >> >> In order to meet this API guarantee the libcamera software ISP allocates >> dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For >> the Fedora COPR repo for the PoC of this: >> https://hansdegoede.dreamwidth.org/28153.html > > Is there any reason for allocating DMA buffers for libcamera through > /dev/dma_heap/ rather than allocating them via corresponding media > device and then giving them away to DRM / VPU / etc? > > At least this should solve the permission usecase: if the app can open > camera device, it can allocate a buffer. This is with a software ISP, the input buffers with raw bayer data come from a v4l2 device, but the output buffers with the processed data are purely userspace managed in this case. Regards, Hans
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
Hi Sima, On 5/6/24 3:38 PM, Daniel Vetter wrote: > On Mon, May 06, 2024 at 02:05:12PM +0200, Maxime Ripard wrote: >> Hi, >> >> On Mon, May 06, 2024 at 01:49:17PM GMT, Hans de Goede wrote: >>> Hi dma-buf maintainers, et.al., >>> >>> Various people have been working on making complex/MIPI cameras work OOTB >>> with mainline Linux kernels and an opensource userspace stack. >>> >>> The generic solution adds a software ISP (for Debayering and 3A) to >>> libcamera. Libcamera's API guarantees that buffers handed to applications >>> using it are dma-bufs so that these can be passed to e.g. a video encoder. >>> >>> In order to meet this API guarantee the libcamera software ISP allocates >>> dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For >>> the Fedora COPR repo for the PoC of this: >>> https://hansdegoede.dreamwidth.org/28153.html >> >> For the record, we're also considering using them for ARM KMS devices, >> so it would be better if the solution wasn't only considering v4l2 >> devices. >> >>> I have added a simple udev rule to give physically present users access >>> to the dma_heap-s: >>> >>> KERNEL=="system", SUBSYSTEM=="dma_heap", TAG+="uaccess" >>> >>> (and on Rasperry Pi devices any users in the video group get access) >>> >>> This was just a quick fix for the PoC. Now that we are ready to move out >>> of the PoC phase and start actually integrating this into distributions >>> the question becomes if this is an acceptable solution; or if we need some >>> other way to deal with this ? >>> >>> Specifically the question is if this will have any negative security >>> implications? I can certainly see this being used to do some sort of >>> denial of service attack on the system (1). This is especially true for >>> the cma heap which generally speaking is a limited resource. >> >> There's plenty of other ways to exhaust CMA, like allocating too much >> KMS or v4l2 buffers. I'm not sure we should consider dma-heaps >> differently than those if it's part of our threat model. > > So generally for an arm soc where your display needs cma, your render node > doesn't. And user applications only have access to the later, while only > the compositor gets a kms fd through logind. At least in drm aside from > vc4 there's really no render driver that just gives you access to cma and > allows you to exhaust that, you need to be a compositor with drm master > access to the display. > > Which means we're mostly protected against bad applications, and that's > not a threat the "user physically sits in front of the machine accounts > for", and which giving cma access to everyone would open up. And with > flathub/snaps/... this is very much an issue. I agree that bad applications are an issue, but not for the flathub / snaps case. Flatpacks / snaps run sandboxed and don't have access to a full /dev so those should not be able to open /dev/dma_heap/* independent of the ACLs on /dev/dma_heap/*. The plan is for cameras using the libcamera software ISP to always be accessed through pipewire and the camera portal, so in this case pipewere is taking the place of the compositor in your kms vs render node example. So this reduces the problem to bad apps packaged by regular distributions and if any of those misbehave the distros should fix that. So I think that for the denial of service side allowing physical present users (but not sandboxed apps running as those users) to access /dev/dma_heap/* should be ok. My bigger worry is if dma_heap (u)dma-bufs can be abused in other ways then causing a denial of service. I guess that the answer there is that causing other security issues should not be possible ? Regards, Hans
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
Hi Maxime, On 5/6/24 2:05 PM, Maxime Ripard wrote: > Hi, > > On Mon, May 06, 2024 at 01:49:17PM GMT, Hans de Goede wrote: >> Hi dma-buf maintainers, et.al., >> >> Various people have been working on making complex/MIPI cameras work OOTB >> with mainline Linux kernels and an opensource userspace stack. >> >> The generic solution adds a software ISP (for Debayering and 3A) to >> libcamera. Libcamera's API guarantees that buffers handed to applications >> using it are dma-bufs so that these can be passed to e.g. a video encoder. >> >> In order to meet this API guarantee the libcamera software ISP allocates >> dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For >> the Fedora COPR repo for the PoC of this: >> https://hansdegoede.dreamwidth.org/28153.html > > For the record, we're also considering using them for ARM KMS devices, > so it would be better if the solution wasn't only considering v4l2 > devices. > >> I have added a simple udev rule to give physically present users access >> to the dma_heap-s: >> >> KERNEL=="system", SUBSYSTEM=="dma_heap", TAG+="uaccess" >> >> (and on Rasperry Pi devices any users in the video group get access) >> >> This was just a quick fix for the PoC. Now that we are ready to move out >> of the PoC phase and start actually integrating this into distributions >> the question becomes if this is an acceptable solution; or if we need some >> other way to deal with this ? >> >> Specifically the question is if this will have any negative security >> implications? I can certainly see this being used to do some sort of >> denial of service attack on the system (1). This is especially true for >> the cma heap which generally speaking is a limited resource. > > There's plenty of other ways to exhaust CMA, like allocating too much > KMS or v4l2 buffers. I'm not sure we should consider dma-heaps > differently than those if it's part of our threat model. Ack. >> But devices tagged for uaccess are only opened up to users who are >> physcially present behind the machine and those can just hit >> the powerbutton, so I don't believe that any *on purpose* DOS is part of >> the thread model. > > How would that work for headless devices? The uaccess tag solution does not work for headless devices, but it also should not hurt any headless scenarios. Headless devices could use something like the video group solution (dma_heap group?) which Raspberry Pi is using and them make sure that any services which need access run as a user in that group. This can co-exist with uaccess tags since those use ACLs not classic Unix permissions. Regards, Hans
Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
Hi dma-buf maintainers, et.al., Various people have been working on making complex/MIPI cameras work OOTB with mainline Linux kernels and an opensource userspace stack. The generic solution adds a software ISP (for Debayering and 3A) to libcamera. Libcamera's API guarantees that buffers handed to applications using it are dma-bufs so that these can be passed to e.g. a video encoder. In order to meet this API guarantee the libcamera software ISP allocates dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For the Fedora COPR repo for the PoC of this: https://hansdegoede.dreamwidth.org/28153.html I have added a simple udev rule to give physically present users access to the dma_heap-s: KERNEL=="system", SUBSYSTEM=="dma_heap", TAG+="uaccess" (and on Rasperry Pi devices any users in the video group get access) This was just a quick fix for the PoC. Now that we are ready to move out of the PoC phase and start actually integrating this into distributions the question becomes if this is an acceptable solution; or if we need some other way to deal with this ? Specifically the question is if this will have any negative security implications? I can certainly see this being used to do some sort of denial of service attack on the system (1). This is especially true for the cma heap which generally speaking is a limited resource. But devices tagged for uaccess are only opened up to users who are physcially present behind the machine and those can just hit the powerbutton, so I don't believe that any *on purpose* DOS is part of the thread model. Any accidental DOS would be a userspace stack bug. Do you foresee any other negative security implications from allowing physically present non root users to create (u)dma-bufs ? Regards, Hans 1) There are some limits in drivers/dma-buf/udmabuf.c and distributions could narrow these.
Re: [PATCH v2 15/43] drm/tiny/gm12u320: Use fbdev-shmem
Hi, On 4/10/24 3:02 PM, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Hans de Goede Thanks, patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans > --- > drivers/gpu/drm/tiny/gm12u320.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > index 0187539ff5eaa..8b4efd39d7c41 100644 > --- a/drivers/gpu/drm/tiny/gm12u320.c > +++ b/drivers/gpu/drm/tiny/gm12u320.c > @@ -13,7 +13,7 @@ > #include > #include > #include > -#include > +#include > #include > #include > #include > @@ -699,7 +699,7 @@ static int gm12u320_usb_probe(struct usb_interface > *interface, > if (ret) > goto err_put_device; > > - drm_fbdev_generic_setup(dev, 0); > + drm_fbdev_shmem_setup(dev, 0); > > return 0; >
[PATCH] drm/vboxvideo: Replace fake VLA at end of vbva_mouse_pointer_shape with real VLA
Replace the fake VLA at end of the vbva_mouse_pointer_shape shape with
a real VLA to fix a "memcpy: detected field-spanning write error" warning:
[ 13.319813] memcpy: detected field-spanning write (size 16896) of single
field "p->data" at drivers/gpu/drm/vboxvideo/hgsmi_base.c:154 (size 4)
[ 13.319841] WARNING: CPU: 0 PID: 1105 at
drivers/gpu/drm/vboxvideo/hgsmi_base.c:154
hgsmi_update_pointer_shape+0x192/0x1c0 [vboxvideo]
[ 13.320038] Call Trace:
[ 13.320173] hgsmi_update_pointer_shape [vboxvideo]
[ 13.320184] vbox_cursor_atomic_update [vboxvideo]
Note as mentioned in the added comment it seems the original length
calculation for the allocated and send hgsmi buffer is 4 bytes too large.
Changing this is not the goal of this patch, so this behavior is kept.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/vboxvideo/hgsmi_base.c | 10 +-
drivers/gpu/drm/vboxvideo/vboxvideo.h | 4 +---
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vboxvideo/hgsmi_base.c
b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
index 8c041d7ce4f1..87dccaecc3e5 100644
--- a/drivers/gpu/drm/vboxvideo/hgsmi_base.c
+++ b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
@@ -139,7 +139,15 @@ int hgsmi_update_pointer_shape(struct gen_pool *ctx, u32
flags,
flags |= VBOX_MOUSE_POINTER_VISIBLE;
}
- p = hgsmi_buffer_alloc(ctx, sizeof(*p) + pixel_len, HGSMI_CH_VBVA,
+ /*
+* The 4 extra bytes come from switching struct vbva_mouse_pointer_shape
+* from having a 4 bytes fixed array at the end to using a proper VLA
+* at the end. These 4 extra bytes were not subtracted from sizeof(*p)
+* before the switch to the VLA, so this way the behavior is unchanged.
+* Chances are these 4 extra bytes are not necessary but they are kept
+* to avoid regressions.
+*/
+ p = hgsmi_buffer_alloc(ctx, sizeof(*p) + pixel_len + 4, HGSMI_CH_VBVA,
VBVA_MOUSE_POINTER_SHAPE);
if (!p)
return -ENOMEM;
diff --git a/drivers/gpu/drm/vboxvideo/vboxvideo.h
b/drivers/gpu/drm/vboxvideo/vboxvideo.h
index f60d82504da0..79ec8481de0e 100644
--- a/drivers/gpu/drm/vboxvideo/vboxvideo.h
+++ b/drivers/gpu/drm/vboxvideo/vboxvideo.h
@@ -351,10 +351,8 @@ struct vbva_mouse_pointer_shape {
* Bytes in the gap between the AND and the XOR mask are undefined.
* XOR mask scanlines have no gap between them and size of XOR mask is:
* xor_len = width * 4 * height.
-*
-* Preallocate 4 bytes for accessing actual data as p->data.
*/
- u8 data[4];
+ u8 data[];
} __packed;
/* pointer is visible */
--
2.44.0
Re: [PATCH v4 10/10] drm/vboxvideo: fix mapping leaks
Hi,
On 4/2/24 3:50 PM, Philipp Stanner wrote:
> On Thu, 2024-03-28 at 12:55 -0500, Bjorn Helgaas wrote:
>> On Fri, Mar 01, 2024 at 12:29:58PM +0100, Philipp Stanner wrote:
>>> When the PCI devres API was introduced to this driver, it was
>>> wrongly
>>> assumed that initializing the device with pcim_enable_device()
>>> instead
>>> of pci_enable_device() will make all PCI functions managed.
>>>
>>> This is wrong and was caused by the quite confusing PCI devres API
>>> in
>>> which some, but not all, functions become managed that way.
>>>
>>> The function pci_iomap_range() is never managed.
>>>
>>> Replace pci_iomap_range() with the actually managed function
>>> pcim_iomap_range().
>>>
>>> CC: # v5.10+
>>
>> This is marked for stable but depends on the preceding patches in
>> this
>> series, which are not marked for stable.
>>
>> The rest of this series might be picked up automatically for stable,
>> but I personally wouldn't suggest backporting it because it's quite a
>> lot of change and I don't think it fits per
>> Documentation/process/stable-kernel-rules.rst.
>
> I agree, if I were a stable maintainer I wouldn't apply it.
> I just put them in CC so that they can make this decision themselves.
>
>> So I think the best way to fix the vboxvideo leaks would be to fix
>> them independently of this series, then include as a separate patch a
>> conversion to the new pcim_iomap_range() in this series (or possibly
>> for the next merge window to avoid merge conflicts).
>
> It is hard to fix independently of our new devres utility.
> Reason being that it's _impossible_ to have partial BAR mappings *with*
> the current PCI devres API.
>
> Consequently, a portable vboxvideo would have to revert the entire
> commit 8558de401b5f and become an unmanaged driver again.
>
> I guess you could do a hacky fix where the regions are handled by
> devres and the mappings are created and destroyed manually with
> pci_iomap_range() – but do we really want that...?
>
> The leak only occurs when driver and device detach, so how often does
> that happen... and as far as I can tell it's also not an exploitable
> leak, so one could make the decision to just leave it in the stable
> kernels...
>
> @Hans:
> What do you say?
In practice this has never been a problem, so I suggest we just drop
the Cc: stable .
Regards,
Hans
>>> Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
>>> Signed-off-by: Philipp Stanner
>>> ---
>>> drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +---
>>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
>>> b/drivers/gpu/drm/vboxvideo/vbox_main.c
>>> index 42c2d8a99509..d4ade9325401 100644
>>> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
>>> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
>>> @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private
>>> *vbox)
>>> /* Take a command buffer for each screen from the end of
>>> usable VRAM. */
>>> vbox->available_vram_size -= vbox->num_crtcs *
>>> VBVA_MIN_BUFFER_SIZE;
>>>
>>> - vbox->vbva_buffers = pci_iomap_range(pdev, 0,
>>> - vbox-
available_vram_size,
>>> - vbox->num_crtcs *
>>> - VBVA_MIN_BUFFER_SIZE);
>>> - if (!vbox->vbva_buffers)
>>> - return -ENOMEM;
>>> + vbox->vbva_buffers = pcim_iomap_range(
>>> + pdev, 0, vbox->available_vram_size,
>>> + vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
>>> + if (IS_ERR(vbox->vbva_buffers))
>>> + return PTR_ERR(vbox->vbva_buffers);
>>>
>>> for (i = 0; i < vbox->num_crtcs; ++i) {
>>> vbva_setup_buffer_context(>vbva_info[i],
>>> @@ -116,11 +115,10 @@ int vbox_hw_init(struct vbox_private *vbox)
>>> DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
>>>
>>> /* Map guest-heap at end of vram */
>>> - vbox->guest_heap =
>>> - pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
>>> - GUEST_HEAP_SIZE);
>>> - if (!vbox->guest_heap)
>>> - return -ENOMEM;
>>> + vbox->guest_heap = pcim_iomap_range(pdev, 0,
>>> + GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
>>> + if (IS_ERR(vbox->guest_heap))
>>> + return PTR_ERR(vbox->guest_heap);
>>>
>>> /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
>>> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4,
>>> -1,
>>> --
>>> 2.43.0
>>>
>>
>
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
Hi, On 3/26/24 4:39 PM, Benjamin Tissoires wrote: > On Mar 26 2024, Werner Sembach wrote: >> Hi all, >> >> Am 25.03.24 um 19:30 schrieb Hans de Goede: >> >> [snip] >>>>> If the kernel already handles the custom protocol into generic HID, the >>>>> work for userspace is not too hard because they can deal with a known >>>>> protocol and can be cross-platform in their implementation. >>>>> >>>>> I'm mentioning that cross-platform because SDL used to rely on the >>>>> input, LEDs, and other Linux peculiarities and eventually fell back on >>>>> using hidraw only because it's way more easier that way. >>>>> >>>>> The other advantage of LampArray is that according to Microsoft's >>>>> document, new devices are going to support it out of the box, so they'll >>>>> be supported out of the box directly. >>>>> >>>>> Most of the time my stance is "do not add new kernel API, you'll regret >>>>> it later". So in that case, given that we have a formally approved >>>>> standard, I would suggest to use it, and consider it your API. >>>> The only new UAPI would be the use_leds_uapi switch to turn on/off the >>>> backwards compatibility. > > I have my reserves with such a kill switch (see below). > >>> Actually we don't even need that. Typically there is a single HID >>> driver handling both keys and the backlight, so userspace cannot >>> just unbind the HID driver since then the keys stop working. > > I don't think Werner meant unbinding the HID driver, just a toggle to > enable/disable the basic HID core processing of LampArray. Right, what I was trying to say is that unbinding the driver might be an alternative. I know things like the G15 keyboard userspace daemons used to do this in the past. But Werner is right that this won't be an option if the actual keyboard presses and the LampArray parts are part of a single HID device. > >>> >>> But with a virtual LampArray HID device the only functionality >>> for an in kernel HID driver would be to export a basic keyboard >>> backlight control interface for simple non per key backlight control >>> to integrate nicely with e.g. GNOME's backlight control. >> >> Don't forget that in the future there will be devices that natively support >> LampArray in their firmware, so for them it is the same device. > > Yeah, the generic LampArray support will not be able to differentiate > "emulated" devices from native ones. > >> >> Regards, >> >> Werner >> >>> And then when OpenRGB wants to take over it can just unbind the HID >>> driver from the HID device using existing mechanisms for that. > > Again no, it'll be too unpredicted. > >>> >>> Hmm, I wonder if that will not also kill hidraw support though ... >>> I guess getting hidraw support back might require then also manually >>> binding the default HID input driver. Bentiss any input on this? > > To be able to talk over hidraw you need a driver to be bound, yes. But I > had the impression that LampArray would be supported by default in > hid-input.c, thus making this hard to remove. Having a separate driver > will work, but as soon as the LampArray device will also export a > multitouch touchpad, we are screwed and will have to make a choice > between LampArray and touch... The idea is to have the actual RGB support in userspace through hidraw, I believe we all agreed on that higher up in the thread. Werner would like for there to also be a simpler in kernel support which treats the per key lighting as if it is a more standard (e.g. thinkpad x1) style keyboard backlight so that existing desktop environment integration for that will work for users who do not install say openrgb. The question is how do we disable the in kernel basic kbd_backlight support when openrgb wants to take over and fully control the LEDs ? Given that driver unbinding is out of the question I think that we are back to having a sysfs attribute to disable / re-enable the in kernel basic kbd_backlight support. Note that the basic kbd_backlight support also allows e.g. GNOME to set the brightness (not only monitor it) so at a minimum we need to disable the "write" support when e.g. openrgb has control. Regards, Hans
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
Hi Werner, On 3/25/24 5:48 PM, Werner Sembach wrote: > Hi Benjamin, > > Am 25.03.24 um 16:56 schrieb Benjamin Tissoires: >> On Mar 25 2024, Hans de Goede wrote: >>> +Cc: Bentiss, Jiri >>> >>> Hi Werner, >>> >>> On 3/20/24 12:16 PM, Werner Sembach wrote: >>>> Hi Hans and the others, >>>> >>>> Am 22.02.24 um 14:14 schrieb Werner Sembach: >>>>> Hi, >>>>> >>>>> Thanks everyone for the exhaustive feedback. And at least this thread is >>>>> a good comprehesive reference for the future ^^. >>>>> >>>>> To recap the hopefully final UAPI for complex RGB lighting devices: >>>>> >>>>> - By default there is a singular /sys/class/leds/* entry that treats the >>>>> device as if it was a single zone RGB keyboard backlight with no special >>>>> effects. >>>>> >>>>> - There is an accompanying misc device with the sysfs attributes "name", >>>>> "device_type", "firmware_version_string", "serial_number" for device >>>>> identification and "use_leds_uapi" that defaults to 1. >>>>> >>>>> - If set to 0 the /sys/class/leds/* entry disappears. The driver >>>>> should keep the last state the backlight was in active if possible. >>>>> >>>>> - If set 1 it appears again. The driver should bring it back to a >>>>> static 1 zone setting while avoiding flicker if possible. >>>>> >>>>> - If the device is not controllable by for example hidraw, the misc >>>>> device might also implement additional ioctls or sysfs attributes to >>>>> allow a more complex low level control for the keyboard backlight. This >>>>> is will be a highly vendor specific UAPI. >>>> So in the OpenRGB issue thread >>>> https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices >>>> aka HID LampArray was mentioned. I did dismiss it because I thought that >>>> is only relevant for firmware, but I now stumbled upon the Virtual HID >>>> Framework (VHF) >>>> https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- >>>> and now I wonder if an equivalent exists for Linux? A quick search did >>>> not yield any results for me. >>> Oh, interesting. I did not know about the HID LampArray API. >>> >>> About your question about virtual HID devices, there is uHID, >>> but as the name suggests this allows userspace to emulate a HID >>> device. >>> >>> In your case you want to do the emulation in kernel so that you >>> can translate the proprietary WMI calls to something HID LampArray >>> compatible. >>> >>> I guess you could do this by defining your own HID transport driver, >>> like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID >>> "client" for each device which talks HID over i2c in the machine. >>> >>> Bentiss, Jiri, do you have any input on this. Would something like >>> that be acceptable to you (just based on the concept at least) ? >> I just read the thread, and I think I now understand a little bit what >> this request is :) >> >> IMO working with the HID LampArray is the way forward. So I would >> suggest to convert any non-HID RGB "LED display" that we are talking >> about as a HID LampArray device through `hid_allocate_device()` in the >> kernel. Basically what you are suggesting Hans. It's just that you don't >> need a formal transport layer, just a child device that happens to be >> HID. >> >> The next question IMO is: do we want the kernel to handle such >> machinery? Wouldn't it be simpler to just export the HID device and let >> userspace talk to it through hidraw, like what OpenRGB does? > > That's already part of my plan: The kernels main goal is to give devices a > LampArray interface that don't have one already (e.g. because they are no HID > devices to begin with). > > The actual handling of LampArray will happen in userspace. > > Exception is that maybe it could be useful to implement a small subset of > LampArray in a generic leds-subsystem driver for backwards compatibility to > userspace applications that only implement that (e.g. UPower). It would treat > the whole keyboard as a single led. > >> >> If the kernel already handles the cust
In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
+Cc: Bentiss, Jiri Hi Werner, On 3/20/24 12:16 PM, Werner Sembach wrote: > Hi Hans and the others, > > Am 22.02.24 um 14:14 schrieb Werner Sembach: >> Hi, >> >> Thanks everyone for the exhaustive feedback. And at least this thread is a >> good comprehesive reference for the future ^^. >> >> To recap the hopefully final UAPI for complex RGB lighting devices: >> >> - By default there is a singular /sys/class/leds/* entry that treats the >> device as if it was a single zone RGB keyboard backlight with no special >> effects. >> >> - There is an accompanying misc device with the sysfs attributes "name", >> "device_type", "firmware_version_string", "serial_number" for device >> identification and "use_leds_uapi" that defaults to 1. >> >> - If set to 0 the /sys/class/leds/* entry disappears. The driver should >> keep the last state the backlight was in active if possible. >> >> - If set 1 it appears again. The driver should bring it back to a static >> 1 zone setting while avoiding flicker if possible. >> >> - If the device is not controllable by for example hidraw, the misc device >> might also implement additional ioctls or sysfs attributes to allow a more >> complex low level control for the keyboard backlight. This is will be a >> highly vendor specific UAPI. > So in the OpenRGB issue thread > https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices > aka HID LampArray was mentioned. I did dismiss it because I thought that is > only relevant for firmware, but I now stumbled upon the Virtual HID Framework > (VHF) > https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- > and now I wonder if an equivalent exists for Linux? A quick search did not > yield any results for me. Oh, interesting. I did not know about the HID LampArray API. About your question about virtual HID devices, there is uHID, but as the name suggests this allows userspace to emulate a HID device. In your case you want to do the emulation in kernel so that you can translate the proprietary WMI calls to something HID LampArray compatible. I guess you could do this by defining your own HID transport driver, like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID "client" for each device which talks HID over i2c in the machine. Bentiss, Jiri, do you have any input on this. Would something like that be acceptable to you (just based on the concept at least) ? Regards, Hans
Re: Future handling of complex RGB devices on Linux v3
Hi Werner, On 3/19/24 4:18 PM, Werner Sembach wrote: > Hi Hans, > > Am 18.03.24 um 12:11 schrieb Hans de Goede: >> Hi Werner, >> >> Sorry for the late reply. > np >> >> On 2/22/24 2:14 PM, Werner Sembach wrote: >>> Hi, >>> >>> Thanks everyone for the exhaustive feedback. And at least this thread is a >>> good comprehesive reference for the future ^^. >>> >>> To recap the hopefully final UAPI for complex RGB lighting devices: >>> >>> - By default there is a singular /sys/class/leds/* entry that treats the >>> device as if it was a single zone RGB keyboard backlight with no special >>> effects. >> Ack this sounds good. >> >>> - There is an accompanying misc device with the sysfs attributes "name", >>> "device_type", "firmware_version_string", "serial_number" for device >>> identification and "use_leds_uapi" that defaults to 1. >> You don't need a char misc device here, you can just make this sysfs >> attributes on the LED class device's parent device by using >> device_driver.dev_groups. Please don't use a char misc device just to attach >> sysfs attributes to it. >> >> Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was >> discussed before >> and makes sense. But at least for HID devices the rest of this info is >> already available >> in sysfs attributes on the HID devices (things like vendor and product id) >> and since the >> userspace code needs per device code to drive the kbd anyways it can also >> have device >> specific code to retrieve all this info, so the other sysfs attributes just >> feel like >> duplicating information. Also there already are a ton of existing hidraw >> userspace rgbkbd >> drivers which already get this info from other places. > The parent device can be a hid device, a wmi device, a platform device etc. > so I thought it would be nice to have some unified way for identification. I think that some shared ioctl for identifying devices which need a misc-device makes sense. But for existing hidraw supported keyboards there already is existing userspace support, which already retreives this in a hidraw specific manner. And I doubt that the existing userspace projects will add support for a new method which is only available on new kernels, while they will also need to keep the old method around to keep things working with older kernels. So at least for the hidraw case I see little value in exporting the same info in another way. >> >>> - If set to 0 the /sys/class/leds/* entry disappears. The driver >>> should keep the last state the backlight was in active if possible. >>> >>> - If set 1 it appears again. The driver should bring it back to a >>> static 1 zone setting while avoiding flicker if possible. >> Ack, if this finds it way into some documentation (which it should) please >> make it >> clear that this is about the "use_leds_uapi" sysfs attribute. > Ack >> >>> - If the device is not controllable by for example hidraw, the misc device >>> might also implement additional ioctls or sysfs attributes to allow a more >>> complex low level control for the keyboard backlight. This is will be a >>> highly vendor specific UAPI. >> IMHO this is the only case where actually using a misc device makes sense, >> so that >> you have a chardev to do the ioctls on. misc-device-s should really only be >> used >> when you need a chardev under /dev . Since you don't need the chardev for >> the e.g. >> hidraw case you should not use a miscdev there IMHO. > > My train of thought was that it would be nice to have the use_leds_uapi at a > somewhat unified location in sysfs. The parent device can be of any kind, > like I mentioned above, and while for deactivating it can easily be found via > /sys/class/leds/*/device/. For reactivating, the leds entry doesn't exist. That is an interesting point. But I would expect any process/daemon which wants to reactivate the in kernel LED class support to be the same process as which deactivated it. So that daemon can simply canonicalize the /sys/class/leds/*/device symlink or canonicalize the entire /sys/class/leds/*/device/use_leds_uapi path and store the canonicalized version for when the daemon wants to reactivate things. So I still think that having the miscdevice just for enumeration and use_leds_uapi purposes is overkill. Regards, Hans
Re: Future handling of complex RGB devices on Linux v3
Hi Werner, Sorry for the late reply. On 2/22/24 2:14 PM, Werner Sembach wrote: > Hi, > > Thanks everyone for the exhaustive feedback. And at least this thread is a > good comprehesive reference for the future ^^. > > To recap the hopefully final UAPI for complex RGB lighting devices: > > - By default there is a singular /sys/class/leds/* entry that treats the > device as if it was a single zone RGB keyboard backlight with no special > effects. Ack this sounds good. > > - There is an accompanying misc device with the sysfs attributes "name", > "device_type", "firmware_version_string", "serial_number" for device > identification and "use_leds_uapi" that defaults to 1. You don't need a char misc device here, you can just make this sysfs attributes on the LED class device's parent device by using device_driver.dev_groups. Please don't use a char misc device just to attach sysfs attributes to it. Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was discussed before and makes sense. But at least for HID devices the rest of this info is already available in sysfs attributes on the HID devices (things like vendor and product id) and since the userspace code needs per device code to drive the kbd anyways it can also have device specific code to retrieve all this info, so the other sysfs attributes just feel like duplicating information. Also there already are a ton of existing hidraw userspace rgbkbd drivers which already get this info from other places. > - If set to 0 the /sys/class/leds/* entry disappears. The driver should > keep the last state the backlight was in active if possible. > > - If set 1 it appears again. The driver should bring it back to a static > 1 zone setting while avoiding flicker if possible. Ack, if this finds it way into some documentation (which it should) please make it clear that this is about the "use_leds_uapi" sysfs attribute. > - If the device is not controllable by for example hidraw, the misc device > might also implement additional ioctls or sysfs attributes to allow a more > complex low level control for the keyboard backlight. This is will be a > highly vendor specific UAPI. IMHO this is the only case where actually using a misc device makes sense, so that you have a chardev to do the ioctls on. misc-device-s should really only be used when you need a chardev under /dev . Since you don't need the chardev for the e.g. hidraw case you should not use a miscdev there IMHO. > > - The actual logic interacting with this low level UAPI is implemented by > a userspace driver > > Implementation wise: For the creation of the misc device with the > use_leds_uapi switch a helper function/macro might be useful? Wonder if it > should go into leds.h, led-class-multicolor.h, or a new header file? See above, I don't think we want the misc device for the hidraw case, at which point I think the helper becomes unnecessary since just a single sysfs write callback is necessary. Also for adding new sysfs attributes it is strongly encouraged to use device_driver.dev_groups so that the device core handled registering / unregistering the sysfs attributes which fixes a bunch of races; and using device_driver.dev_groups does not mix well with a helper as you've suggested. > > - Out of my head it would look something like this: > > led_classdev_add_optional_misc_control( > struct led_classdev *led_cdev, > char* name, > char* device_type, > char* firmware_version_string, > char* serial_number, > void (*deregister_led)(struct led_classdev *led_cdev), > void (*reregister_led)(struct led_classdev *led_cdev)) > > Let me know your thoughts and hopefully I can start implementing it soon for > one of our devices. I think overall the plan sounds good, with my main suggested change being to not use an unnecessary misc device for the hid-raw case. Regards, Hans
Re: [PATCH v4 10/10] drm/vboxvideo: fix mapping leaks
Hi,
On 3/1/24 12:29, Philipp Stanner wrote:
> When the PCI devres API was introduced to this driver, it was wrongly
> assumed that initializing the device with pcim_enable_device() instead
> of pci_enable_device() will make all PCI functions managed.
>
> This is wrong and was caused by the quite confusing PCI devres API in
> which some, but not all, functions become managed that way.
>
> The function pci_iomap_range() is never managed.
>
> Replace pci_iomap_range() with the actually managed function
> pcim_iomap_range().
>
> CC: # v5.10+
> Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
> Signed-off-by: Philipp Stanner
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede
Since this depends on the pcim_iomap_range() function which is new
in this series and since the vboxvideo code does not see a lot
of changes I think that it would be best for this patch to be
merged through the PCI tree together with the rest of the series.
Regards,
Hans
> ---
> drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
> b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index 42c2d8a99509..d4ade9325401 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private *vbox)
> /* Take a command buffer for each screen from the end of usable VRAM. */
> vbox->available_vram_size -= vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE;
>
> - vbox->vbva_buffers = pci_iomap_range(pdev, 0,
> - vbox->available_vram_size,
> - vbox->num_crtcs *
> - VBVA_MIN_BUFFER_SIZE);
> - if (!vbox->vbva_buffers)
> - return -ENOMEM;
> + vbox->vbva_buffers = pcim_iomap_range(
> + pdev, 0, vbox->available_vram_size,
> + vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
> + if (IS_ERR(vbox->vbva_buffers))
> + return PTR_ERR(vbox->vbva_buffers);
>
> for (i = 0; i < vbox->num_crtcs; ++i) {
> vbva_setup_buffer_context(>vbva_info[i],
> @@ -116,11 +115,10 @@ int vbox_hw_init(struct vbox_private *vbox)
> DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
>
> /* Map guest-heap at end of vram */
> - vbox->guest_heap =
> - pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
> - GUEST_HEAP_SIZE);
> - if (!vbox->guest_heap)
> - return -ENOMEM;
> + vbox->guest_heap = pcim_iomap_range(pdev, 0,
> + GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
> + if (IS_ERR(vbox->guest_heap))
> + return PTR_ERR(vbox->guest_heap);
>
> /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
Re: [PATCH 2/2] fbcon: Defer console takeover for splash screens to first switch
Hi Daniel, On 2/28/24 03:00, Daniel van Vugt wrote: > On 27/2/24 21:47, Hans de Goede wrote: > I think some boot failures also take you to the grub menu automatically next > time? In Fedora all boot failures will unhide the grub menu on the next boot. This unfortunately relies on downstream changes so I don't know what Ubuntu does here. >>>> The kernel itself will be quiet as long as you set >>>> CONFIG_CONSOLE_LOGLEVEL_QUIET=3 Ubuntu atm has set this >>>> to 4 which means any kernel pr_err() or dev_err() >>>> messages will get through and since there are quite >>>> a few false positives of those Ubuntu really needs >>>> to set CONFIG_CONSOLE_LOGLEVEL_QUIET=3 to fix part of: >>>> https://bugs.launchpad.net/bugs/1970069 >>> >>> Incorrect. In my testing some laptops needed log level as low as 2 to go >>> quiet. >>> And the Ubuntu kernel team is never going to fix all those for non-sponsored >>> devices. >> >> Notice that atm Ubuntu's kernel is using the too high >> CONFIG_CONSOLE_LOGLEVEL_QUIET=4 with >> CONFIG_CONSOLE_LOGLEVEL_QUIET=3 getting any errors logged >> to the console should be very very rare. >> >> The only thing I can think of is if the kernel oopses >> / WARN()s early on but the cause is innocent enough >> that the boot happily continues. >> >> In that case actually showing the oops/WARN() is a good >> thing. >> >> For all the years Fedora has had flickerfree boot I have >> seen zero bug reports about this. If you have examples >> of this actually being a problem please file bugs for >> them (launchpad or bugzilla.kernel.org is fine) and >> then lets take a look at those bugs and fix them. >> >> These should be so rare that I'm not worried about this >> becoming a never ending list of bugs (unlike pr_err() / >> dev_err() messages of which there are simply too many). > > I personally own many laptops with so many different boot messages that it's > overwhelming for me already to report bugs for each of them. Hence this patch. > > Also I don't own all the laptops in the world, so fixing all the errors just > for my collection wouldn't solve all cases. Whereas this patch does. Almost all of those boot messages are because Ubuntu has set CONFIG_CONSOLE_LOGLEVEL_QUIET too high. Once that is fixed there should be very little of not no messages left. I too own many laptops and I'm not seeing this issue on any of them. You claim you are still seeing errors with CONFIG_CONSOLE_LOGLEVEL_QUIET=3 yet you have not provided a single example! >> Sorry, but your real problem here seems to be your >> noisy downstream systemd patch. I'm not going to ack >> a kernel patch which I consider a bad idea because >> Ubuntu has a non standard systemd patch which is >> to trigger happy with spamming the console. > > Indeed the systemd patch is a big problem. We seem to have had it for 9 years > or so. I only just discovered it recently and would love to drop it, but was > told we can't. Its main problem is that it uses the console as a communication > pipe to plymouth. So simply making it less noisy isn't possible without > disabling its functionality. It was seemingly intended to run behind the > splash, but since it does fsck it tends to run before the splash (because DRM > startup takes a few more seconds). This does indeed sound like it is a non trivial problem to fix, but that is still not a good reason to add this (IMHO) hack to the kernel. The issue deferred fbcon takeover was designed to fix is that the fbcon would mess up the framebuffer contents even if nothing was ever logged to the console. The whole idea being that to still have the fbcon come up as soon as there are any messages. Actively hiding messages was never part of the design, so this is still a NACK from me. Also note that this matches how things work in grub and shim when I first implemented flickerfree boot I also had to patch shim and grub to not make EFI text output protocol calls (including init()) until they actually had some text to show. So the whole design here for shim, grub and the kernel has always been to not mess with the framebuffer until there is some text (any text) to output and then show that text immediately. I do not think that deviating from this design is a good idea. Regards, Hans >>>>> Closes: https://bugs.launchpad.net/bugs/1970069 >>>>> Cc: Mario Limonciello >>>>> Signed-off-by: Daniel van Vugt >>>>> --- >>>>> drivers/video/fbdev/core/fbcon.c | 32 +--- >>>>> 1 file changed, 29 insertions(+), 3 deletions(-) >>>>> >>&g
Re: [PATCH 2/2] fbcon: Defer console takeover for splash screens to first switch
Hi, On 2/27/24 02:06, Daniel van Vugt wrote: > On 27/2/24 02:23, Hans de Goede wrote: >> Hi All, >> >> On 2/2/24 09:53, Daniel van Vugt wrote: >>> Until now, deferred console takeover only meant defer until there is >>> output. But that risks stepping on the toes of userspace splash screens, >>> as console messages may appear before the splash screen. So check for the >>> "splash" parameter (as used by Plymouth) and if present then extend the >>> deferral until the first switch. >> >> Daniel, thank you for your patch but I do not believe that this >> is the right solution. Deferring fbcon takeover further then >> after the first text is output means that any errors about e.g. >> a corrupt initrd or the kernel erroring out / crashing will not >> be visible. > > That's not really correct. If a boot failure has occurred after the splash > then > pressing escape shows the log. Hmm, I guess this is with the latest plymouth which has a builtin terminal emulator for kernels without VT support ? Pressing ESC does not to a VC switch and AFAICT that is what you are triggering on to allow fbcon takeover after this patches. > If a boot failure has occurred before the splash > then it can be debugged visually by rebooting without the "splash" parameter. Which requires the user to know this and requires the user to know how to edit kernel cmdline parameters in e.g. grub. This is not a good user experience. We want inexperienced users to just be able to point a phone camera at the screen and take a picture of the errors. >> When the kernel e.g. oopses or panics because of not finding >> its rootfs (I tested the latter option when writing the original >> deferred fbcon takeover code) then fbcon must takeover and >> print the messages from the dying kernel so that the user has >> some notion of what is going wrong. > > Indeed, just reboot without the "splash" parameter to do that. Again not something beginning Linux users will be able to do, what happened to "Ubuntu: Linux for Human Beings" ? >> And since your patch seems to delay switching till the first >> vc-switch this means that e.g. even after say gdm refusing >> to start because of issues there still will be no text >> output. This makes debugging various issues much harder. > > I've debugged many gdm failures and it is never useful to use the console for > those. Reboot and get the system journal instead. But users will not see any errors now, meaning they don't even know where to begin with troubleshooting ... >> Moreover Fedora has been doing flickerfree boot for many >> years without needing this. > > I believe Fedora has a mostly working solution, but not totally reliable, as > mentioned in the commit message: > > "even systems whose splash exists in initrd may not be not immune because they > still rely on racing against all possible kernel messages that might > trigger the fbcon takeover" Only very serious kernel errors like oopses or panics will trigger the takeover and that is *exactly* what we want. There is a race where plymouth may hide such vary serious messages, if plymouth does manage to start before the errors, but that is actually an existing issue which we don't want to make bigger by *always* hiding such errors. >> The kernel itself will be quiet as long as you set >> CONFIG_CONSOLE_LOGLEVEL_QUIET=3 Ubuntu atm has set this >> to 4 which means any kernel pr_err() or dev_err() >> messages will get through and since there are quite >> a few false positives of those Ubuntu really needs >> to set CONFIG_CONSOLE_LOGLEVEL_QUIET=3 to fix part of: >> https://bugs.launchpad.net/bugs/1970069 > > Incorrect. In my testing some laptops needed log level as low as 2 to go > quiet. > And the Ubuntu kernel team is never going to fix all those for non-sponsored > devices. Notice that atm Ubuntu's kernel is using the too high CONFIG_CONSOLE_LOGLEVEL_QUIET=4 with CONFIG_CONSOLE_LOGLEVEL_QUIET=3 getting any errors logged to the console should be very very rare. The only thing I can think of is if the kernel oopses / WARN()s early on but the cause is innocent enough that the boot happily continues. In that case actually showing the oops/WARN() is a good thing. For all the years Fedora has had flickerfree boot I have seen zero bug reports about this. If you have examples of this actually being a problem please file bugs for them (launchpad or bugzilla.kernel.org is fine) and then lets take a look at those bugs and fix them. These should be so rare that I'm not worried about this becoming a never ending list of bugs (unlike pr_err() / dev_err() messages of which there are simply too many). >> After that it
Re: [PATCH 2/2] fbcon: Defer console takeover for splash screens to first switch
Hi All,
On 2/2/24 09:53, Daniel van Vugt wrote:
> Until now, deferred console takeover only meant defer until there is
> output. But that risks stepping on the toes of userspace splash screens,
> as console messages may appear before the splash screen. So check for the
> "splash" parameter (as used by Plymouth) and if present then extend the
> deferral until the first switch.
Daniel, thank you for your patch but I do not believe that this
is the right solution. Deferring fbcon takeover further then
after the first text is output means that any errors about e.g.
a corrupt initrd or the kernel erroring out / crashing will not
be visible.
When the kernel e.g. oopses or panics because of not finding
its rootfs (I tested the latter option when writing the original
deferred fbcon takeover code) then fbcon must takeover and
print the messages from the dying kernel so that the user has
some notion of what is going wrong.
And since your patch seems to delay switching till the first
vc-switch this means that e.g. even after say gdm refusing
to start because of issues there still will be no text
output. This makes debugging various issues much harder.
Moreover Fedora has been doing flickerfree boot for many
years without needing this.
The kernel itself will be quiet as long as you set
CONFIG_CONSOLE_LOGLEVEL_QUIET=3 Ubuntu atm has set this
to 4 which means any kernel pr_err() or dev_err()
messages will get through and since there are quite
a few false positives of those Ubuntu really needs
to set CONFIG_CONSOLE_LOGLEVEL_QUIET=3 to fix part of:
https://bugs.launchpad.net/bugs/1970069
After that it is "just" a matter of not making userspace
output anything unless it has errors to report.
systemd already is quiet by default (only logging
errors) when quiet is on the kernel commandline.
So any remaining issues are Ubuntu specific boot
process bits and Ubuntu really should be able to
make those by silent unless they have important
info (errors or other unexpected things) to report.
Given that this will make debugging boot issues
much harder and that there are other IMHO better
alternatives I'm nacking this patch: NACK.
FWIW I believe that I'm actually saving Ubuntu
from shooting themselves in the foot here,
hiding all sort of boot errors (like the initrd
not finding /) until the user does a magic
alt+f2 followed by alt+f1 incantation really is
not doing yourself any favors wrt debugging any
sort of boot failures.
Regards,
Hans
> Closes: https://bugs.launchpad.net/bugs/1970069
> Cc: Mario Limonciello
> Signed-off-by: Daniel van Vugt
> ---
> drivers/video/fbdev/core/fbcon.c | 32 +---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c
> b/drivers/video/fbdev/core/fbcon.c
> index 63af6ab034..5b9f7635f7 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -76,6 +76,7 @@
> #include /* For counting font checksums */
> #include
> #include
> +#include
>
> #include "fbcon.h"
> #include "fb_internal.h"
> @@ -146,6 +147,7 @@ static inline void fbcon_map_override(void)
>
> #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> static bool deferred_takeover = true;
> +static int initial_console = -1;
> #else
> #define deferred_takeover false
> #endif
> @@ -3341,7 +3343,7 @@ static void fbcon_register_existing_fbs(struct
> work_struct *work)
> console_unlock();
> }
>
> -static struct notifier_block fbcon_output_nb;
> +static struct notifier_block fbcon_output_nb, fbcon_switch_nb;
> static DECLARE_WORK(fbcon_deferred_takeover_work,
> fbcon_register_existing_fbs);
>
> static int fbcon_output_notifier(struct notifier_block *nb,
> @@ -3358,6 +3360,21 @@ static int fbcon_output_notifier(struct notifier_block
> *nb,
>
> return NOTIFY_OK;
> }
> +
> +static int fbcon_switch_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct vc_data *vc = data;
> +
> + WARN_CONSOLE_UNLOCKED();
> +
> + if (vc->vc_num != initial_console) {
> + dummycon_unregister_switch_notifier(_switch_nb);
> + dummycon_register_output_notifier(_output_nb);
> + }
> +
> + return NOTIFY_OK;
> +}
> #endif
>
> static void fbcon_start(void)
> @@ -3370,7 +3387,14 @@ static void fbcon_start(void)
>
> if (deferred_takeover) {
> fbcon_output_nb.notifier_call = fbcon_output_notifier;
> - dummycon_register_output_notifier(_output_nb);
> + fbcon_switch_nb.notifier_call = fbcon_switch_notifier;
> + initial_console = fg_console;
> +
> + if (cmdline_find_option_bool(boot_command_line, "splash"))
> + dummycon_register_switch_notifier(_switch_nb);
> + else
> + dummycon_register_output_notifier(_output_nb);
> +
> return;
> }
> #endif
> @@ -3417,8 +3441,10 @@
Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
Hi,
On 2/22/24 17:44, Matthew Auld wrote:
> On 22/02/2024 14:58, Marek Behún wrote:
>> A few drivers are doing resource-managed mutex initialization by
>> implementing ad-hoc one-liner mutex dropping functions and using them
>> with devm_add_action_or_reset(). Help drivers avoid these repeated
>> one-liners by adding managed version of mutex initialization.
>> index 74891802200d..70640fb96117 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -24,6 +24,8 @@
>> */
>> #include
>> +#include
>> +#include
>> #include
>> static inline void devm_delayed_work_drop(void *res)
>> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
>> return devm_add_action(dev, devm_work_drop, w);
>> }
>> +static inline void devm_mutex_drop(void *res)
>> +{
>> + mutex_destroy(res);
>> +}
>> +
>> +/**
>> + * devm_mutex_init - Resource managed mutex initialization
>> + * @dev: Device which lifetime mutex is bound to
>> + * @lock: Mutex to be initialized (and automatically destroyed)
>> + *
>> + * Initialize mutex which is automatically destroyed when driver is
>> detached.
>> + * A few drivers initialize mutexes which they want destroyed before driver
>> is
>> + * detached, for debugging purposes.
>> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call
>> when
>> + * driver is detached.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> + mutex_init(lock);
>
> Do you know if this this needs __always_inline? The static lockdep key in
> mutex_init() should be
> different for each caller class. See c21f11d182c2 ("drm: fix
> drmm_mutex_init()").
That is a very good point. I believe that this should mirror mutex_init() and
the actual static inline function should be __devm_mutex_init() which takes
the key as extra argument (and calls __mutex_init()) and then make
devm_mutex_init() itself a macro mirroring the mutex_init() macro.
Regards,
Hans
>
>> +
>> + /*
>> + * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
>> + * disabled. No need to allocate an action in that case.
>> + */
>> + if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
>> + return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
>> + else
>> + return 0;
>> +}
>> +
>> #endif
>
Re: Future handling of complex RGB devices on Linux v2
Hi, On 2/22/24 12:38, Gregor Riepl wrote: >> This certainly is the most KISS approach. This proposal >> in essence is just an arbitrary command multiplexer / >> demultiplexer and ioctls already are exactly that. >> >> With the added advantage of being able to directly use >> pass the vendor-cmd-specific struct to the ioctl instead >> of having to first embed it in some other struct. > > There's also the question of how much complexity needs to remain in the > kernel, if vendor-specific ioctls are made available. > > Does every vendor driver implement a complex mapping to hardware registers? > What about drivers that basically implement no mapping at all and simply > forward all data to the hardware without any checking? The latter case would > match Pavel's concerns, although I don't see how this is any different from > the situation today, where userspace talks directly to the hardware via > libusb etc. This whole discussion got started by embedded-controller driven keyboards in laptops with per key RGB lighting. We cannot just allow userspace raw-access to the embedded-controller. So these per vendor ioctl commands will need to do the minimum to make sure userspace cannot do bad things. But yes complex stuff like figuring out which LED(s) maps to say the enter key should be left to userspace. Especially since this can differ per keyboardlayout. > To be honest, I think the kernel shouldn't include too much high-level > complexity. If there is a desire to implement a generic display device on top > of the RGB device, this should be a configurable service running in user > space. The kernel should provide an interface to expose this emulated display > as a "real" display to applications - unless this can also be done entirely > in user space in a generic way. We really need to stop seeing per key addressable RGB keyboards as displays: 1. Some "pixels" are non square 2. Not all "pixels" have the same width-height ratio 3. Not all rows have the same amount of pixels 4. There are holes in the rows like between the enter key and then numpad 5. Some "pixels" have multiple LEDs beneath them. These might be addressable per LEDs are the sub-pixels ? What about a 2 key wide backspace key vs the 1 key wide + another key (some non US layouts) in place of the backspace? This will be "2 pixels" in some layout and 1 pixel with maybe / maybe-not 2 subpixels where the sub-pixels may/may not be individually addressable ? For all these reasons the display analogy really is a bit fit for these keyboards we tried to come up with a universal coordinate system for these at the beginning of the thread and we failed ... Regards, Hans
Re: Future handling of complex RGB devices on Linux v2
Hi,
On 2/21/24 23:17, Pavel Machek wrote:
> Hi!
>
>> so after more feedback from the OpenRGB maintainers I came up with an even
>> more generic proposal:
>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>
>>> evaluate-set-command ioctl taking:
>>> {
>>> enum command /* one of supported_commands */
>>> union data
>>> {
>>> char raw[3072],
>>> {
>>>
>>> },
>
> Yeah, so ... this is not a interface. This is a backdoor to pass
> arbitrary data. That's not going to fly.
Pavel, Note the data will be interpreted by a kernel driver and
not passed directly to the hw.
With that said I tend to agree that this seems to be a bit too
generic.
Werner, it seems you are basically re-inventing ioctls here.
If you are going to use per vendor specific data structs for various
commands and have those defined in the kernel userspace API headers,
then this means that userspace will already need updated versions
of those headers to support new vendors / new laptop models if
the commands change for a new model.
So what you are basically providing here is a generic interface
to pass a cmd number + a cmd-bumber-specific data struct and
we already have that it is called an ioctl.
So I think that the conclusion of this whole discussion is that
with the exception of a get-dev-info ioctl, we simply want
vendor specific ioctls, using 1 ioctl per command.
Given that these devices are all different in various ways
and that we only want this for devices which cannot be accessed
from userspace directly (so a limit set of devices) I really
think that just doing custom ioctls per vendor is best.
This certainly is the most KISS approach. This proposal
in essence is just an arbitrary command multiplexer /
demultiplexer and ioctls already are exactly that.
With the added advantage of being able to directly use
pass the vendor-cmd-specific struct to the ioctl instead
of having to first embed it in some other struct.
Regards,
Hans
Re: Implement per-key keyboard backlight as auxdisplay?
Hi, On 1/30/24 20:08, Werner Sembach wrote: > Hi, > > Am 30.01.24 um 19:35 schrieb Hans de Goede: >> Hi, >> >> On 1/30/24 19:09, Werner Sembach wrote: >>> Hi Hans, >>> >>> resend because Thunderbird htmlified the mail :/ >> I use thunderbird too. If you right click on the server name >> and then go to "Settings" -> "Composition & Addressing" >> and then uncheck "Compose messages in HTML format" >> I think that should do the trick. > Can't set this globally or other people will complain that my replies delete > company logos in signatures xD. But usually the auto detection of Thunderbird > works. >> >>> Am 30.01.24 um 18:10 schrieb Hans de Goede: >>>> Hi Werner, >>>> >>>> On 1/30/24 12:12, Werner Sembach wrote: >>>>> Hi Hans, >>>>> >>>>> Am 29.01.24 um 14:24 schrieb Hans de Goede: >>> >>>>> I think that are mostly external keyboards, so in theory a possible cut >>>>> could also between built-in and external devices. >>>> IMHO it would be better to limit /dev/rgbledstring use to only >>>> cases where direct userspace control is not possible and thus >>>> have the cut be based on whether direct userspace control >>>> (e.g. /dev/hidraw access) is possible or not. >>> Ack >>> >>> >>> >>>>> So also no basic driver? Or still the concept from before with a basic 1 >>>>> zone only driver via leds subsystem to have something working, but it is >>>>> unregistered by userspace, if open rgb wants to take over for fine >>>>> granular support? >>>> Ah good point, no I think that a basic driver just for kbd backlight >>>> brightness support which works with the standard desktop environment >>>> controls for this makes sense. >>>> >>>> Combined with some mechanism for e.g. openrgb to fully take over >>>> control as discussed. It is probably a good idea to file a separate >>>> issue with the openrgb project to discuss the takeover API. >>> I think the OpenRGB maintainers are pretty flexible at that point, after >>> all it's similar to enable commands a lot of rgb devices need anyway. I >>> would include it in a full api proposal. >> Ack. >> >>> On this note: Any particular reason you suggested an ioctl interface >>> instead of a sysfs one? (Open question as, for example, I have no idea what >>> performance implications both have) >> sysfs APIs typically have a one file per setting approach, >> so for effects with speed and multiple-color settings you >> would need a whole bunch of different files and then you >> would either need to immediately apply every setting, >> needing multiple writes to the hw for a single effect >> update, or have some sort of "commit" sysfs attribute. >> >> With ioctls you can simply provide all the settings >> in one call, which is why I suggested using ioctls. > > Ack > > If the static mode update is fast enough to have userspace controlled > animations, OpenRGB is calling that direct mode. Is it feasible to send 30 or > more ioctls per second for such an direct mode? Or should this spawn a > special purpose sysfs file that is kept open by userspace to continuously > update the keyboard? ioctls are quite fast and another advantage of ioctls is you open the /dev/rgbledstring# device only once and then re-use the fd for as many ioctls as you want. Regards, Hans
Re: Implement per-key keyboard backlight as auxdisplay?
Hi, On 1/30/24 19:09, Werner Sembach wrote: > Hi Hans, > > resend because Thunderbird htmlified the mail :/ I use thunderbird too. If you right click on the server name and then go to "Settings" -> "Composition & Addressing" and then uncheck "Compose messages in HTML format" I think that should do the trick. > Am 30.01.24 um 18:10 schrieb Hans de Goede: >> Hi Werner, >> >> On 1/30/24 12:12, Werner Sembach wrote: >>> Hi Hans, >>> >>> Am 29.01.24 um 14:24 schrieb Hans de Goede: > >>> I think that are mostly external keyboards, so in theory a possible cut >>> could also between built-in and external devices. >> IMHO it would be better to limit /dev/rgbledstring use to only >> cases where direct userspace control is not possible and thus >> have the cut be based on whether direct userspace control >> (e.g. /dev/hidraw access) is possible or not. > > Ack > > > >>> So also no basic driver? Or still the concept from before with a basic 1 >>> zone only driver via leds subsystem to have something working, but it is >>> unregistered by userspace, if open rgb wants to take over for fine granular >>> support? >> Ah good point, no I think that a basic driver just for kbd backlight >> brightness support which works with the standard desktop environment >> controls for this makes sense. >> >> Combined with some mechanism for e.g. openrgb to fully take over >> control as discussed. It is probably a good idea to file a separate >> issue with the openrgb project to discuss the takeover API. > > I think the OpenRGB maintainers are pretty flexible at that point, after all > it's similar to enable commands a lot of rgb devices need anyway. I would > include it in a full api proposal. Ack. > On this note: Any particular reason you suggested an ioctl interface instead > of a sysfs one? (Open question as, for example, I have no idea what > performance implications both have) sysfs APIs typically have a one file per setting approach, so for effects with speed and multiple-color settings you would need a whole bunch of different files and then you would either need to immediately apply every setting, needing multiple writes to the hw for a single effect update, or have some sort of "commit" sysfs attribute. With ioctls you can simply provide all the settings in one call, which is why I suggested using ioctls. Regards, Hans
Re: Implement per-key keyboard backlight as auxdisplay?
Hi Werner,
On 1/30/24 12:12, Werner Sembach wrote:
> Hi Hans,
>
> Am 29.01.24 um 14:24 schrieb Hans de Goede:
>> That sounds workable OTOH combined with your remarks about also supporting
>> lightbars. I'm starting to think that we need to just punt this to userspace.
>>
>> So basically change things from trying to present a standardized address
>> space where say the 'Q' key is always in the same place just model
>> a keyboard as a string of LEDs (1 dimensional / so an array) and leave
>> mapping which address in the array is which key to userspace, then userspace
>> can have json or whatever files for this per keyboard.
>>
>> This keeps the kernel interface much more KISS which I think is what
>> we need to strive for.
>>
>> So instead of having /dev/rgbkbd we get a /dev/rgbledstring and then
>> that can be used for rbb-kbds and also your lightbar example as well
>> as actual RGB LED strings, which depending on the controller may
>> also have zones / effects, etc. just like the keyboards.
>> Right, so see above I think we need to push all these complications
>> into userspace. And simple come up for a kernel interface
>> for RGB LED strings with zones / effects / possibly individual
>> addressable LEDs.
>>
>> Also we should really only use whatever kernel interface we come up
>> with for devices which cannot be supported directly from userspace
>> through e.g. hidraw access. Looking at keyboards then the openrgb project:
>>
>> https://openrgb.org/devices_0.9.html
>>
>> Currently already supports 398 keyboard modes, we really do not want
>> to be adding support for all those to the kernel.
> I think that are mostly external keyboards, so in theory a possible cut could
> also between built-in and external devices.
IMHO it would be better to limit /dev/rgbledstring use to only
cases where direct userspace control is not possible and thus
have the cut be based on whether direct userspace control
(e.g. /dev/hidraw access) is possible or not.
>> Further down in the thread you mention Mice with RGB LEDs,
>> Mice are almost always HID devices and already have extensive support,
>> including for their LEDs in userspace through libratbag and the piper UI,
>> see the screenshots here (click on the camera icon):
>> https://linux.softpedia.com/get/Utilities/Piper-libratbag-104168.shtml
>>
>> Again we really don't want to be re-doing all this work in the kernel
>> only to end up conflicting with the existing userspace support.
>>
>>
>>
>>>> 5. A set_effect ioctl which takes a struct with the following members:
>>>>
>>>> {
>>>> long size; /* Size of passed in struct including the size member itself */
>>>> int effect_nr; /* enum value of the effect to enable, 0 for disable effect
>>>> */
>>>> int zone; /* zone to apply the effect to */
>>> Don't know if this is necessary, the keyboards I have seen so far apply
>>> firmware effects globally.
>>>> int speed; /* cycle speed of the effect in milli-hz */
>>> I would split this into speed and speed_max and don't specify an actual
>>> unit. The firmwares effects I have seen so far: If they have a speed value,
>>> it's some low number interpreted as a proportional x/n * the max speed of
>>> this effect, with n being some low number like 8 or 10.
>>>
>>> But i don't know if such clearly named properties are even sensefull, see
>>> below.
>>>
>>>> char color1[3]; /* effect dependend may be unused. */
>>>> char color2[3]; /* effect dependend may be unused. */
>>>> }
>>> We can not predetermine how many colors we might need in the future.
>>>
>>> Firmware effects can vary vastly in complexity, e.g. breathing can be a
>>> single bit switch that just varies the brightness of whatever color setting
>>> is currently applied. It could have an optional speed argument. It could
>>> have nth additional color arguments to cycle through, it could have an
>>> optional randomize bit that either randomizes the order of the defined
>>> colors or means that it is picking completely random color ignoring the
>>> color settings if set.
>>>
>>> Like this we could have a very fast explosion of the effects enum e.g.:
>>> breathing, breathing_2_colors, breathing_3_colors, ... breathing_n_colors,
>>> breathing_speed_controlled, breathing_speed_controlled_2_colors, ...
>>> breathing_speed_controlled_n_colors_random_bit, etc.
>>>
>>> Or we give up on generic n
Re: Implement per-key keyboard backlight as auxdisplay?
Hi Werner, On 1/19/24 17:04, Werner Sembach wrote: > Am 19.01.24 um 09:44 schrieb Hans de Goede: >> So my proposal would be an ioctl interface (ioctl only no r/w) >> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev. >> >> For per key controllable rgb LEDs we need to discuss a coordinate >> system. I propose using a fixed size of 16 rows of 64 keys, >> so 64x16 in standard WxH notation. >> >> And then storing RGB in separate bytes, so userspace will then >> always send a buffer of 192 bytes per line (64x3) x 14 rows >> = 3072 bytes. With the kernel driver ignoring parts of >> the buffer where there are no actual keys. > The be sure the "14 rows" is a typo? And should be 16 rows? Yes that should be 16. >> This way we can address all the possible keys in the various >> standard layouts in one standard wat and then the drivers can >> just skip keys which are not there when preparing the buffer >> to send to the hw / fw. > > Some remarks here: > > - Some keyboards might have two or more leds for big keys like (iso-)enter, > shift, capslock, num+, etc. that in theory are individually controllable by > the firmware. In windows drivers this is usually abstracted away, but could > be interesting for effects (e.g. if the top of iso-enter is separate from the > bottom of iso-enter like with one of our devices). > > - In combination with this: The driver might not be able to tell if the > actual physical keyboard is ISO or ANSI, so it might not be able the > correctly assign the leds around enter correctly as being an own key or being > part of ANSI- or ISO-enter. > > - Should the interface have different addresses for the different enter and > num+ styles (or even the different length shifts and spacebars)? > > One idea for this: Actually assign 1 value per line for tall keys per line, 3 > (or maybe even 4, to have one spare) values per line for wide keys and 6 (or > 8) values for space. e.g.: That sounds workable OTOH combined with your remarks about also supporting lightbars. I'm starting to think that we need to just punt this to userspace. So basically change things from trying to present a standardized address space where say the 'Q' key is always in the same place just model a keyboard as a string of LEDs (1 dimensional / so an array) and leave mapping which address in the array is which key to userspace, then userspace can have json or whatever files for this per keyboard. This keeps the kernel interface much more KISS which I think is what we need to strive for. So instead of having /dev/rgbkbd we get a /dev/rgbledstring and then that can be used for rbb-kbds and also your lightbar example as well as actual RGB LED strings, which depending on the controller may also have zones / effects, etc. just like the keyboards. > - Right shift would have 3 values in row 10. The first value might be the > left side of shift or the additional ABNT/JIS key. The 2nd Key might be the > left side or middle of shift and the third key might be the right side of > shift or the only value for the whole key. The additional ABNT/JIS key still > also has a dedicated value which is used by drivers which can differentiate > between physical layouts. > > - Enter would have 3 values in row 8 and 3 values in row 9. With the same > disambiguation as the additional ABNT/JIS but this time for ansii-/ and iso-# > > - Num+ would have 2 values, one row 8 and one in row 9. The one in row 9 > might control the whole key or might just control the lower half. The one in > row 8 might be another key or the upper half > > For the left half if the main block the leftmost value should be the "might > be the only relevant"-value while the right most value should be the "might > be another key"-value. For the right side of the main block this should be > swapped. Unused values should be adjacent to the "might be another > key"-value, e.g.: > > | Left shift value 1 | Left shift value > 2 | Left shift value 3 | Left shift value 4 | 102nd > key value > ISO/ANSI aware | Left shift color | Unused > | Unused | Unused | 102nd > key color > ISO non aware 1 led under shift | Left shift color | Unused > | Unused | 102nd key color | Unused > ANSI non aware 1 led under shift | Left shift color | Unused > | Unused | Unused | Unused > ISO non aware 2 leds under shift | Left shift left color | Left shift right > color | Unused
Re: Implement per-key keyboard backlight as auxdisplay?
Hi, On 1/19/24 21:15, Pavel Machek wrote: > Hi! > 2. Implement per-key keyboards as auxdisplay - Pro: - Already has a concept for led positions - Is conceptually closer to "multiple leds forming a singular entity" - Con: - No preexisting UPower support - No concept for special hardware lightning modes - No support for arbitrary led outlines yet (e.g. ISO style enter-key) >>> >>> Please do this one. >> >> Ok, so based on the discussion so far and Pavel's feedback lets try to >> design a custom userspace API for this. I do not believe that auxdisplay >> is a good fit because: > > Ok, so lets call this a "display". These days, framebuffers and drm > handles displays. My proposal is to use similar API as other displays. > >> So my proposal would be an ioctl interface (ioctl only no r/w) >> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev. >> >> For per key controllable rgb LEDs we need to discuss a coordinate >> system. I propose using a fixed size of 16 rows of 64 keys, >> so 64x16 in standard WxH notation. >> >> And then storing RGB in separate bytes, so userspace will then >> always send a buffer of 192 bytes per line (64x3) x 14 rows >> = 3072 bytes. With the kernel driver ignoring parts of >> the buffer where there are no actual keys. > > That's really really weird interface. If you are doing RGB888 64x14, > lets make it a ... display? :-). > > ioctl always sending 3072 bytes is really a hack. > > Small displays exist and are quite common, surely we'd handle this as > a display: > https://pajenicko.cz/displeje/graficky-oled-displej-0-66-64x48-i2c-bily-wemos-d1-mini > It is 64x48. This is indeed a display and should use display APIs > And then there's this: > https://pajenicko.cz/displeje/maticovy-8x8-led-displej-s-radicem-max7219 > and this: > https://pajenicko.cz/displeje/maticovy-8x32-led-displej-s-radicem-max7219 > > One of them is 8x8. > > Surely those should be displays, too? The 8x8 one not really, the other one could be used to scroll some text one but cannot display images, so not really displays IMHO. Anyways we are talking about keyboards here and those do not have a regular x-y grid like your example above, so they certainly do not count as displays. See the long discussion earlier in the thread. Regards, Hans
Re: [PATCH v2 10/10] drm/vboxvideo: fix mapping leaks
Hi Philipp,
On 1/23/24 10:43, Philipp Stanner wrote:
> When the PCI devres API was introduced to this driver, it was wrongly
> assumed that initializing the device with pcim_enable_device() instead
> of pci_enable_device() will make all PCI functions managed.
>
> This is wrong and was caused by the quite confusing devres API for PCI
> in which some, but not all, functions become managed that way.
>
> The function pci_iomap_range() is never managed.
>
> Replace pci_iomap_range() with the actually managed function
> pcim_iomap_range().
>
> Additionally, add a call to pcim_request_region() to ensure exclusive
> access to BAR 0.
I'm a bit worried about this last change. There might be
issues where the pcim_request_region() fails due to
e.g. a conflict with the simplefb / simpledrm code.
There is a drm_aperture_remove_conflicting_pci_framebuffers()
call done before hw_init() gets called, but still this
has been known to cause issues in the past.
Can you split out the adding of the pcim_request_region()
into a separate patch and *not* mark that separate patch
for stable ?
Regards,
Hans
>
> CC: # v5.10+
> Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
> Signed-off-by: Philipp Stanner
> ---
> drivers/gpu/drm/vboxvideo/vbox_main.c | 24 +---
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
> b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index 42c2d8a99509..7f686a0190e6 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private *vbox)
> /* Take a command buffer for each screen from the end of usable VRAM. */
> vbox->available_vram_size -= vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE;
>
> - vbox->vbva_buffers = pci_iomap_range(pdev, 0,
> - vbox->available_vram_size,
> - vbox->num_crtcs *
> - VBVA_MIN_BUFFER_SIZE);
> - if (!vbox->vbva_buffers)
> - return -ENOMEM;
> + vbox->vbva_buffers = pcim_iomap_range(
> + pdev, 0, vbox->available_vram_size,
> + vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
> + if (IS_ERR(vbox->vbva_buffers))
> + return PTR_ERR(vbox->vbva_buffers);
>
> for (i = 0; i < vbox->num_crtcs; ++i) {
> vbva_setup_buffer_context(>vbva_info[i],
> @@ -115,12 +114,15 @@ int vbox_hw_init(struct vbox_private *vbox)
>
> DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
>
> + ret = pcim_request_region(pdev, 0, "vboxvideo");
> + if (ret)
> + return ret;
> +
> /* Map guest-heap at end of vram */
> - vbox->guest_heap =
> - pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
> - GUEST_HEAP_SIZE);
> - if (!vbox->guest_heap)
> - return -ENOMEM;
> + vbox->guest_heap = pcim_iomap_range(pdev, 0,
> + GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
> + if (IS_ERR(vbox->guest_heap))
> + return PTR_ERR(vbox->guest_heap);
>
> /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
Re: Implement per-key keyboard backlight as auxdisplay?
Hi,
On 1/18/24 18:45, Pavel Machek wrote:
> Hi!
>
>> We have an upcoming device that has a per-key keyboard backlight, but does
>> the control completely via a wmi/acpi interface. So no usable hidraw here
>> for a potential userspace driver implementation ...
>>
>> So a quick summary for the ideas floating in this thread so far:
>>
>> 1. Expand leds interface allowing arbitrary modes with semi arbitrary
>> optional attributes:
>
>> - Con:
>>
>> - Violates the simplicity paradigm of the leds interface (e.g. with
>> this one leds entry controls possible multiple leds)
>
> Let's not do this.
>
>> 2. Implement per-key keyboards as auxdisplay
>>
>> - Pro:
>>
>> - Already has a concept for led positions
>>
>> - Is conceptually closer to "multiple leds forming a singular entity"
>>
>> - Con:
>>
>> - No preexisting UPower support
>>
>> - No concept for special hardware lightning modes
>>
>> - No support for arbitrary led outlines yet (e.g. ISO style
>> enter-key)
>
> Please do this one.
Ok, so based on the discussion so far and Pavel's feedback lets try to
design a custom userspace API for this. I do not believe that auxdisplay
is a good fit because:
- auxdisplay is just a directory name, it does not seem to clearly
define an API
- instead the deprecated /dev/fb API is used which is deprecated
- auxdisplays are very much displays (hence /dev/fb) they are typically
small LCD displays with a straight widthxheight grid of square pixels
- /dev/fb does gives us nothing for effects, zoned keyboard, etc.
So my proposal would be an ioctl interface (ioctl only no r/w)
using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
For per key controllable rgb LEDs we need to discuss a coordinate
system. I propose using a fixed size of 16 rows of 64 keys,
so 64x16 in standard WxH notation.
And then storing RGB in separate bytes, so userspace will then
always send a buffer of 192 bytes per line (64x3) x 14 rows
= 3072 bytes. With the kernel driver ignoring parts of
the buffer where there are no actual keys.
I would then like the map the standard 105 key layout onto this,
starting at x.y (column.row) coordinates of 16.6 (with 0.0 being
the top left). Leaving plenty of space on the left top and right
(and some on the bottom) for extra media key rows, macro keys, etc.
The idea to have the standard layout at a fixed place is to allow
userspace to have a database of preset patterns which will work
everywhere.
Note I say standard 105 key layout, but in reality for
defining the standardized part of the buffer we should
use the maximum amount of keys per row of all the standard layouts,
so for row 6 (the ESC row) and for extra keys on the right outside
the main block we use the standard layout as shown here:
http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
For the main area of the keyboard looking at:
http://bopqehorizon.weebly.com/uploads/1/3/4/3/134337299/913246919_orig.png
We want to max rows per key, so this means that per row we use
(from the above image) :
row 7: 106/109 - JIS
row 8: 101/104 - ANSI
row 9: 102/105 - ISO
row 10: 104/107 - ABNT
row 11: 106/109 - JIS
(with row 7 being the main area top row)
This way we can address all the possible keys in the various
standard layouts in one standard wat and then the drivers can
just skip keys which are not there when preparing the buffer
to send to the hw / fw.
One open question is if we should add padding after the main
area so that the printscreen / ins / del / leftarrow of the
"middle" block of
http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
all start at the same x (say 32) or we just pack these directly
after the main area.
And the same question for the numlock block, do we align
this to an x of say 36, or pack it ?
As for the actual IOCTL API I think there should be
the following ioctls:
1. A get-info ioctl returning a struct with the following members:
{
char name[64] /* Keyboard model name / identifier */
int row_begin[16]; /* The x address of the first available key per row. On a
std 105key kbd this will be 16 for rows 6-11, 0 for other rows */
int row_end[16]; /* x+1 for the address of the last available key per row,
end - begin gives number of keys in a row */
int rgb_zones; /* number of rgb zones for zoned keyboards. Note both
zones and per key addressing may be available if
effects are applied per zone. */
?
}
2. A set-leds ioctl which takes the earlier discussed 3092 bytes buffer
to set all the LEDs at once, only valid if at least one row has a non 0 lenght.
3. A set-zones ioctl which takes an array of bytes sized 3 * number-of-zones
containing RGB values for each zone
4. A enum_effects ioctl which takes a struct with the following members:
{
long size; /* Size of passed in struct including the size member itself */
long effects_mask[]
}
the idea being that there is an enum
Re: Userspace API for per key backlight for non HID (no hidraw) keyboards
Hi All, On 11/27/23 11:59, Werner Sembach wrote: > I also stumbled across a new Problem: > > We have an upcoming device that has a per-key keyboard backlight, but does > the control completely via a wmi/acpi interface. So no usable hidraw here for > a potential userspace driver implementation ... > > So a quick summary for the ideas floating in this thread so far: > > 1. Expand leds interface allowing arbitrary modes with semi arbitrary > optional attributes: > > - Pro: > > - Still offers all default attributes for use with UPower > > - Fairly simple to implement from the preexisting codebase > > - Could be implemented for all (to me) known internal keyboard > backlights > > - Con: > > - Violates the simplicity paradigm of the leds interface (e.g. with > this one leds entry controls possible multiple leds) So what you are suggesting here is having some way (a-z + other sysfs attr?) to use a single LED class device and then extend that to allow setting all keys ? This does not seem like a good idea to me and this will also cause issues when doing animations in software, since this API will likely be slow. And if the API is not slow, then it will likely involve some sort of binary sysfs file for setting multiple keys rather then 1 file per key which would break the normal 1 file per setting sysfs paradigm. > 2. Implement per-key keyboards as auxdisplay > > - Pro: > > - Already has a concept for led positions With a "concept" you mean simple x,y positioning or is there something more advanced here that I'm aware of ? > - Is conceptually closer to "multiple leds forming a singular entity" > > - Con: > > - No preexisting UPower support > > - No concept for special hardware lightning modes > > - No support for arbitrary led outlines yet (e.g. ISO style enter-key) Hmm, so there is very little documentation on this and what docs there is: Documentation/admin-guide/auxdisplay/cfag12864b.rst as well as the example program how to uses this suggests that this is using the old /dev/fb# interface which we are sorta trying to retire. > 3. Implement in input subsystem > > - Pro: > > - Preexisting concept for keys and key purpose > > - Con: > > - Not in scope for subsystem > > - No other preexisting light infrastructure Dmitry actually recently nacked the addition of a LED_MIC_MUTE define to include/uapi/linux/input-event-codes.h which was intended to be able to allow the input LED support with standard HID mic-mute leds (spk-mute is already supported this way). Dmitry was very clear that no new LEDs must be added and that any new LED support should be done through the LED subsytem, so I do not think that something like this is going to fly. > 4. Implement a simple leds driver only supporting a small subset of the > capabilities and make it disable-able for a userspace driver to take over > > - Pro: > > - Most simple to implement basic support > > - In scope for led subsystem simplicity paradigm > > - Con: > > - Not all built in keyboard backlights can be implemented in a > userspace only driver Right, so this is basically what we have been discussing in the other part of the thread with the: /sys/bus/hid/devices/0003::./disable_kbd_backlight_support proposal to unregister the kernel's LED class device and then allow userspace to do whatever it wants through /dev/hidraw without the kernel also trying to access the backlight functionality at the same time. AFAIK there already is a bunch of userspace support for per key addressable kbd RGB backlights using hidraw support, so this way we can use the momentum / code of these existing projects, at least for existing hidraw keyboards and adding support for: /sys/bus/hid/devices/0003::./disable_kbd_backlight_support to these existing projects should be simple. Yet this will not work for your mentioned "control completely via a wmi/acpi interface". Still I think we should go the same route for those adding a misc-char device or something like that to allow making WMI calls from userspace (like Windows can do). Maybe with an allow list per GUID to only allow specific calls, so that we can avoid possible dangerous calls. Armin Wolf recently became the WMI bus maintainer. Armin, we are discussing how to deal with (laptop) keyboards which have a separate RGB LED per key and how to control those LEDs. So far per key addressable keyboard backlighting has always been HID based, so any existing support is just userspace based using /dev/hidraw. In my experience the problem with supporting gaming peripherals is that there is interest in it, but not really enough interest to keep a sustained momentum behind projects, especially not when it comes to taking code from a fun weekend hack to upstreaming them into bigger projects like the kernel. So I would like
Re: Implement per-key keyboard backlight as auxdisplay?
Hi Werner, Once again, sorry for the very slow response here. On 11/27/23 11:59, Werner Sembach wrote: > Hi Hans, > > Am 22.11.23 um 19:34 schrieb Hans de Goede: >> Hi Werner, > [snip] >>>>> Another idea I want to throw in the mix: >>>>> >>>>> Maybe the kernel is not the right place to implement this at all. RGB >>>>> stuff is not at all standardized and every vendor is doing completely >>>>> different interfaces, which does not fit the kernel userpsace apis desire >>>>> to be uniformal and fixed. e.g. Auxdisplay might fit static setting of >>>>> RGB values, but it does not fit the snake-effect mode, or the raindrops >>>>> mode, or the 4-different-colors-in-the-edges-breathing-and-color-cycling >>>>> mode. >>>>> >>>>> So my current idea: Implement these keyboards as a single zone RGB >>>>> kbd_backlight in the leds interface to have something functional out of >>>>> the box, but make it runtime disable-able if something like >>>>> https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine >>>>> granular control from userspace via hidraw. >>>> That sounds like a good approach to me. We are seeing the same with game >>>> controllers where steam and wine/proton also sometimes use hidraw mode to >>>> get access to all the crazy^W interesting features. >>>> >>>> That would mean that all we need to standardize and the kernel <-> >>>> userspace API level is adding a standard way to disable the single zone >>>> RGB kbd_backlight support in the kernel. >>> I would suggest a simple "enable" entry. Default is 1. When set to 0 the >>> kernel driver no longer does anything. >> I'm not in favor of using "enable" as sysfs attribute for this, >> I would like to see a more descriptive name, how about: >> >> "disable_kernel_kbd_backlight_support" >> >> And then maybe also have the driver actually unregister >> the LED class device ? >> >> Or just make the support inactive when writing 1 to >> this and allow re-enabling it by writing 0? > > Unregistering would mean that it can't be reenabled without module > reload/reboot? Yes. > I would prefer that the userspace driver could easily give back control to > the leds interface. Hmm, the problem here is that leaving a non-working LED class device behind may confuse things like upower. So maybe the disable_kbd_backlight_support sysfs attr should not sit under /sys/class/leds/foobar::kbd_backlight, but rather it should sit under the sysfs dir of the parent-device ? So if we are talking [USB|I2C]-HID keyboards and userspace using hidraw to takeover kbd_backlight control through, then have "disable_kbd_backlight_support" sit under /sys/bus/hid/devices/0003::./disable_kbd_backlight_support and then re-register the LED class device for the keyboard when 0 gets written to disable_kbd_backlight_support ? That seems better to me then leaving a non-working LED class device behind and this will not require any changes to the LED subsystem. >>> Questions: >>> >>> - Should the driver try to reset the settings to boot default? Or just >>> leave the device in the current state? With the former I could see issues >>> that they keyboard is flashing when changing from kernelspace control to >>> userspace control. With the later the burden on bringing the device to a >>> know state lies with the userspace driver. >> My vote would go to leave the state as is. Even if the hw >> does not support state readback, then the userspace code >> can readback the state before writing 1 to >> "disable_kernel_kbd_backlight_support" > ack >> >>> - Should this be a optional entry that only shows up on drivers supporting >>> it, or could this implemented in a generic way affecting all current led >>> entries? >> IMHO this should be optional. If we go with the variant >> where writing 1 to "disable_kernel_kbd_backlight_support" >> just disables support and 0 re-enables it then I guess >> we could have support for this in the LED-core, enabled >> by a flag set by the driver. >> >> If we go with unregistering the led class device, >> then this needs to be mostly handled in the driver. >> >> Either way the kernel driver should know about this even >> if it is mostly handled in the LED core so that e.g. >> it does not try to restore settings on resume from suspend. > > So a
Re: [PATCH] fbdev/simplefb: change loglevel when the power domains cannot be parsed
Hi,
On 12/12/23 20:57, Brian Masney wrote:
> When the power domains cannot be parsed, the message is incorrectly
> logged as an info message. Let's change this to an error since an error
> is returned.
>
> Fixes: 92a511a568e4 ("fbdev/simplefb: Add support for generic power-domains")
> Signed-off-by: Brian Masney
Thank you for your patch. I have pushed this to drm-misc-next now.
Regards,
Hans
> ---
> drivers/video/fbdev/simplefb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 6f58ee276ad1..028a56525047 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -470,7 +470,7 @@ static int simplefb_attach_genpds(struct simplefb_par
> *par,
> if (err == -ENOENT)
> return 0;
>
> - dev_info(dev, "failed to parse power-domains: %d\n", err);
> + dev_err(dev, "failed to parse power-domains: %d\n", err);
> return err;
> }
>
[PATCH] drm/i915/dsi: Use devm_gpiod_get() for all GPIOs
soc_gpio_set_value() already uses devm_gpiod_get(), lets be consistent
and use devm_gpiod_get() for all GPIOs.
This allows removing the intel_dsi_vbt_gpio_cleanup() function,
which only function was to put the GPIO-descriptors.
Signed-off-by: Hans de Goede
---
Note this applies on top of Andy's recent GPIO rework series which
has just landed in drm-intel-next
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 17 ++---
drivers/gpu/drm/i915/display/intel_dsi_vbt.h | 1 -
drivers/gpu/drm/i915/display/vlv_dsi.c | 10 +-
3 files changed, 3 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 275d0218394c..a5d7fc8418c9 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -922,7 +922,7 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi,
bool panel_is_on)
gpiod_add_lookup_table(gpiod_lookup_table);
if (want_panel_gpio) {
- intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
+ intel_dsi->gpio_panel = devm_gpiod_get(dev->dev, "panel",
flags);
if (IS_ERR(intel_dsi->gpio_panel)) {
drm_err(_priv->drm,
"Failed to own gpio for panel control\n");
@@ -932,7 +932,7 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi,
bool panel_is_on)
if (want_backlight_gpio) {
intel_dsi->gpio_backlight =
- gpiod_get(dev->dev, "backlight", flags);
+ devm_gpiod_get(dev->dev, "backlight", flags);
if (IS_ERR(intel_dsi->gpio_backlight)) {
drm_err(_priv->drm,
"Failed to own gpio for backlight control\n");
@@ -943,16 +943,3 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi,
bool panel_is_on)
if (gpiod_lookup_table)
gpiod_remove_lookup_table(gpiod_lookup_table);
}
-
-void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
-{
- if (intel_dsi->gpio_panel) {
- gpiod_put(intel_dsi->gpio_panel);
- intel_dsi->gpio_panel = NULL;
- }
-
- if (intel_dsi->gpio_backlight) {
- gpiod_put(intel_dsi->gpio_backlight);
- intel_dsi->gpio_backlight = NULL;
- }
-}
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.h
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.h
index 468d873fab1a..3462fcc760e6 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.h
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.h
@@ -13,7 +13,6 @@ struct intel_dsi;
bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on);
-void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi);
void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
enum mipi_seq seq_id);
void intel_dsi_log_params(struct intel_dsi *intel_dsi);
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index 5bda97c96810..9b33b8a74d64 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1532,16 +1532,8 @@ static void intel_dsi_unprepare(struct intel_encoder
*encoder)
}
}
-static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
-{
- struct intel_dsi *intel_dsi =
enc_to_intel_dsi(to_intel_encoder(encoder));
-
- intel_dsi_vbt_gpio_cleanup(intel_dsi);
- intel_encoder_destroy(encoder);
-}
-
static const struct drm_encoder_funcs intel_dsi_funcs = {
- .destroy = intel_dsi_encoder_destroy,
+ .destroy = intel_encoder_destroy,
};
static enum drm_mode_status vlv_dsi_mode_valid(struct drm_connector *connector,
--
2.41.0
Re: Implement per-key keyboard backlight as auxdisplay?
Hi Werner,
On 11/21/23 14:29, Werner Sembach wrote:
>
> Am 21.11.23 um 13:20 schrieb Hans de Goede:
>> Hi Werner,
>>
>> On 11/21/23 12:33, Werner Sembach wrote:
>>> Hi,
>>>
>>> Am 20.11.23 um 21:52 schrieb Pavel Machek:
>>>> Hi!
>>>>
>>>>>>> So... a bit of rationale. The keyboard does not really fit into the
>>>>>>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>>>>>>> a matrix of them.
>>>>>> Makes sense.
>>>>>>
>>>>>>> We do see various strange displays these days -- they commonly have
>>>>>>> rounded corners and holes in them. I'm not sure how that's currently
>>>>>>> supported, but I believe it is reasonable to view keyboard as a
>>>>>>> display with slightly weird placing of pixels.
>>>>>>>
>>>>>>> Plus, I'd really like to play tetris on one of those :-).
>>>>>>>
>>>>>>> So, would presenting them as auxdisplay be acceptable? Or are there
>>>>>>> better options?
>>>>>> It sounds like a fair use case -- auxdisplay are typically simple
>>>>>> character-based or small graphical displays, e.g. 128x64, that may not
>>>>>> be a "main" / usual screen as typically understood, but the concept is
>>>>>> a bit fuzzy and we are a bit of a catch-all.
>>>>>>
>>>>>> And "keyboard backlight display with a pixel/color per-key" does not
>>>>>> sound like a "main" screen, and having some cute effects displayed
>>>>>> there are the kind of thing that one could do in the usual small
>>>>>> graphical ones too. :)
>>>>>>
>>>>>> But if somebody prefers to create new categories (or subcategories
>>>>>> within auxdisplay) to hold these, that could be nice too (in the
>>>>>> latter case, I would perhaps suggest reorganizing all of the existing
>>>>>> ones while at it).
>>>>> One could also reasonably make the argument that controlling the
>>>>> individual keyboard key backlights should be part of the input
>>>>> subsystem. It's not a display per se. (Unless you actually have small
>>>>> displays on the keycaps, and I think that's a thing too.)
>>>> While it would not be completely crazy to do that... I believe the
>>>> backlight is more of a display and less of a keyboard. Plus input
>>>> subystem is very far away from supporting this, and we had no input
>>>> from input people here.
>>>>
>>>> I don't think LED subsystem is right place for this, and I believe
>>>> auxdisplay makes slightly more sense than input.
>>>>
>>>> Unless someone steps up, I'd suggest Werner tries to implement this as
>>>> an auxdisplay. [And yes, this will not be simple task. RGB on LED is
>>>> different from RGB on display. But there are other LED displays, so
>>>> auxdisplay should handle this. Plus pixels are really funnily
>>>> shaped. But displays with missing pixels -- aka holes for camera --
>>>> are common in phones, and I believe we'll get variable pixel densities
>>>> -- less dense over camera -- too. So displays will have to deal with
>>>> these in the end.]
>>> Another idea I want to throw in the mix:
>>>
>>> Maybe the kernel is not the right place to implement this at all. RGB stuff
>>> is not at all standardized and every vendor is doing completely different
>>> interfaces, which does not fit the kernel userpsace apis desire to be
>>> uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB
>>> values, but it does not fit the snake-effect mode, or the raindrops mode,
>>> or the 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>>>
>>> So my current idea: Implement these keyboards as a single zone RGB
>>> kbd_backlight in the leds interface to have something functional out of the
>>> box, but make it runtime disable-able if something like
>>> https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine
>>> granular control from userspace via hidraw.
>> That sounds like a good approach to me. We are seeing the same with game
>> controllers where steam and wine/proton also sometimes use hidraw mode to
>> get access to all the crazy^W in
Re: [PATCH] fbdev/simplefb: Suppress error on missing power domains
Hi,
On 11/22/23 01:54, Richard Acayan wrote:
> When the power domains are missing, the call to of_count_phandle_with_args
> fails with -ENOENT. The power domains are not required and there are
> some device trees that do not specify them. Suppress this error to fix
> devices without power domains attached to simplefb.
>
> Fixes: 92a511a568e4 ("fbdev/simplefb: Add support for generic power-domains")
> Closes: https://lore.kernel.org/linux-fbdev/ZVwFNfkqjrvhFHM0@radian
> Signed-off-by: Richard Acayan
Thank you for the patch.
I've pushed this to drm-misc-next now.
Regards,
Hans
> ---
> drivers/video/fbdev/simplefb.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index fe682af63827..6f58ee276ad1 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -466,6 +466,10 @@ static int simplefb_attach_genpds(struct simplefb_par
> *par,
> err = of_count_phandle_with_args(dev->of_node, "power-domains",
>"#power-domain-cells");
> if (err < 0) {
> + /* Nothing wrong if optional PDs are missing */
> + if (err == -ENOENT)
> + return 0;
> +
> dev_info(dev, "failed to parse power-domains: %d\n", err);
> return err;
> }
Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains
Hi,
On 11/22/23 01:01, Richard Acayan wrote:
> On Tue, Nov 21, 2023 at 10:01:18AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/21/23 02:17, Richard Acayan wrote:
>>> Hello,
>>>
>>> On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote:
>>>> From: Thierry Reding
>>>>
>>>> The simple-framebuffer device tree bindings document the power-domains
>>>> property, so make sure that simplefb supports it. This ensures that the
>>>> power domains remain enabled as long as simplefb is active.
>>>>
>>>> v2: - remove unnecessary call to simplefb_detach_genpds() since that's
>>>> already done automatically by devres
>>>> - fix crash if power-domains property is missing in DT
>>>>
>>>> Signed-off-by: Thierry Reding
>>>> ---
>>>> drivers/video/fbdev/simplefb.c | 93 ++
>>>> 1 file changed, 93 insertions(+)
>>>>
>>>> diff --git a/drivers/video/fbdev/simplefb.c
>>>> b/drivers/video/fbdev/simplefb.c
>>>> index 18025f34fde7..fe682af63827 100644
>>>> --- a/drivers/video/fbdev/simplefb.c
>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>> @@ -25,6 +25,7 @@
>>>> #include
>>>> #include
>>>> #include
>>>> +#include
>>>> #include
>>>>
>>>> static const struct fb_fix_screeninfo simplefb_fix = {
>>>> @@ -78,6 +79,11 @@ struct simplefb_par {
>>>>unsigned int clk_count;
>>>>struct clk **clks;
>>>> #endif
>>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>>> + unsigned int num_genpds;
>>>
>>> This is the cause of the crash that occurred on the older patch series.
>>> The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c.
>>>
>>> Instead of making it signed, this version emits an error whenever the
>>> count is negative.
>>
>> I'm not sure what you are trying to say here ?
>
> In v1 of the patch, there was no error propagation from
> of_count_phandle_with_args
> and this field was directly assigned to the return value. This was a
> problem (the "crash" as mentioned in this patch's changelog) when the
> return value is negative, since unsigned integers cannot hold negative
> values. On mainstream architectures, the driver would believe that there
> is an absurd amount of power domains.
>
> I compared the versions of this patch and figured that the fix to the
> crash was more error handling.
>
> Basically, if "unsigned" was removed, the error handling for the call to
> of_count_phandle_with_args could be dropped with few consequences.
I see, thank you for explaining.
I believe that actually handling the error is better then storing
a negative value, so I believe that the fix in v2 is correct.
>>>> + struct device **genpds;
>>>> + struct device_link **genpd_links;
>>>> +#endif
>>>> #if defined CONFIG_OF && defined CONFIG_REGULATOR
>>>>bool regulators_enabled;
>>>>u32 regulator_count;
>>>> @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct
>>>> simplefb_par *par,
>>>> static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>>>> #endif
>>>>
>>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>>> +static void simplefb_detach_genpds(void *res)
>>>> +{
>>>> + struct simplefb_par *par = res;
>>>> + unsigned int i = par->num_genpds;
>>>> +
>>>> + if (par->num_genpds <= 1)
>>>> + return;
>>>> +
>>>> + while (i--) {
>>>> + if (par->genpd_links[i])
>>>> + device_link_del(par->genpd_links[i]);
>>>> +
>>>> + if (!IS_ERR_OR_NULL(par->genpds[i]))
>>>> + dev_pm_domain_detach(par->genpds[i], true);
>>>> + }
>>>> +}
>>>> +
>>>> +static int simplefb_attach_genpds(struct simplefb_par *par,
>>>> +struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = >dev;
>>>> + unsigned int i;
>>>> + int err;
>>>> +
>>>> + err = of_count_phandle_with_args(dev->of_node, "power-domains",
>>>> +
Re: Implement per-key keyboard backlight as auxdisplay?
Hi Werner,
On 11/21/23 12:33, Werner Sembach wrote:
> Hi,
>
> Am 20.11.23 um 21:52 schrieb Pavel Machek:
>> Hi!
>>
> So... a bit of rationale. The keyboard does not really fit into the
> LED subsystem; LEDs are expected to be independent ("hdd led") and not
> a matrix of them.
Makes sense.
> We do see various strange displays these days -- they commonly have
> rounded corners and holes in them. I'm not sure how that's currently
> supported, but I believe it is reasonable to view keyboard as a
> display with slightly weird placing of pixels.
>
> Plus, I'd really like to play tetris on one of those :-).
>
> So, would presenting them as auxdisplay be acceptable? Or are there
> better options?
It sounds like a fair use case -- auxdisplay are typically simple
character-based or small graphical displays, e.g. 128x64, that may not
be a "main" / usual screen as typically understood, but the concept is
a bit fuzzy and we are a bit of a catch-all.
And "keyboard backlight display with a pixel/color per-key" does not
sound like a "main" screen, and having some cute effects displayed
there are the kind of thing that one could do in the usual small
graphical ones too. :)
But if somebody prefers to create new categories (or subcategories
within auxdisplay) to hold these, that could be nice too (in the
latter case, I would perhaps suggest reorganizing all of the existing
ones while at it).
>>> One could also reasonably make the argument that controlling the
>>> individual keyboard key backlights should be part of the input
>>> subsystem. It's not a display per se. (Unless you actually have small
>>> displays on the keycaps, and I think that's a thing too.)
>> While it would not be completely crazy to do that... I believe the
>> backlight is more of a display and less of a keyboard. Plus input
>> subystem is very far away from supporting this, and we had no input
>> from input people here.
>>
>> I don't think LED subsystem is right place for this, and I believe
>> auxdisplay makes slightly more sense than input.
>>
>> Unless someone steps up, I'd suggest Werner tries to implement this as
>> an auxdisplay. [And yes, this will not be simple task. RGB on LED is
>> different from RGB on display. But there are other LED displays, so
>> auxdisplay should handle this. Plus pixels are really funnily
>> shaped. But displays with missing pixels -- aka holes for camera --
>> are common in phones, and I believe we'll get variable pixel densities
>> -- less dense over camera -- too. So displays will have to deal with
>> these in the end.]
>
> Another idea I want to throw in the mix:
>
> Maybe the kernel is not the right place to implement this at all. RGB stuff
> is not at all standardized and every vendor is doing completely different
> interfaces, which does not fit the kernel userpsace apis desire to be
> uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB values,
> but it does not fit the snake-effect mode, or the raindrops mode, or the
> 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>
> So my current idea: Implement these keyboards as a single zone RGB
> kbd_backlight in the leds interface to have something functional out of the
> box, but make it runtime disable-able if something like
> https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine
> granular control from userspace via hidraw.
That sounds like a good approach to me. We are seeing the same with game
controllers where steam and wine/proton also sometimes use hidraw mode to get
access to all the crazy^W interesting features.
That would mean that all we need to standardize and the kernel <-> userspace
API level is adding a standard way to disable the single zone RGB kbd_backlight
support in the kernel.
Regards,
Hans
Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains
Hi,
On 11/21/23 02:17, Richard Acayan wrote:
> Hello,
>
> On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote:
>> From: Thierry Reding
>>
>> The simple-framebuffer device tree bindings document the power-domains
>> property, so make sure that simplefb supports it. This ensures that the
>> power domains remain enabled as long as simplefb is active.
>>
>> v2: - remove unnecessary call to simplefb_detach_genpds() since that's
>> already done automatically by devres
>> - fix crash if power-domains property is missing in DT
>>
>> Signed-off-by: Thierry Reding
>> ---
>> drivers/video/fbdev/simplefb.c | 93 ++
>> 1 file changed, 93 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index 18025f34fde7..fe682af63827 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -25,6 +25,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>>
>> static const struct fb_fix_screeninfo simplefb_fix = {
>> @@ -78,6 +79,11 @@ struct simplefb_par {
>> unsigned int clk_count;
>> struct clk **clks;
>> #endif
>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> +unsigned int num_genpds;
>
> This is the cause of the crash that occurred on the older patch series.
> The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c.
>
> Instead of making it signed, this version emits an error whenever the
> count is negative.
I'm not sure what you are trying to say here ?
>> +struct device **genpds;
>> +struct device_link **genpd_links;
>> +#endif
>> #if defined CONFIG_OF && defined CONFIG_REGULATOR
>> bool regulators_enabled;
>> u32 regulator_count;
>> @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct
>> simplefb_par *par,
>> static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>> #endif
>>
>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> +static void simplefb_detach_genpds(void *res)
>> +{
>> +struct simplefb_par *par = res;
>> +unsigned int i = par->num_genpds;
>> +
>> +if (par->num_genpds <= 1)
>> +return;
>> +
>> +while (i--) {
>> +if (par->genpd_links[i])
>> +device_link_del(par->genpd_links[i]);
>> +
>> +if (!IS_ERR_OR_NULL(par->genpds[i]))
>> +dev_pm_domain_detach(par->genpds[i], true);
>> +}
>> +}
>> +
>> +static int simplefb_attach_genpds(struct simplefb_par *par,
>> + struct platform_device *pdev)
>> +{
>> +struct device *dev = >dev;
>> +unsigned int i;
>> +int err;
>> +
>> +err = of_count_phandle_with_args(dev->of_node, "power-domains",
>> + "#power-domain-cells");
>> +if (err < 0) {
>> +dev_info(dev, "failed to parse power-domains: %d\n", err);
>> +return err;
>
> This error path is taken when there is no power-domains property in the
> device tree with err = -ENOENT.
>
> Strangely, this does not suppress the error like the next if statement,
> even though it is possible that nothing is wrong.
>
>> +}
>> +
>> +par->num_genpds = err;
>> +
>> +/*
>> + * Single power-domain devices are handled by the driver core, so
>> + * nothing to do here.
>> + */
>> +if (par->num_genpds <= 1)
>> +return 0;
>> +
>> +par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
>> + GFP_KERNEL);
>
>> @@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
>> if (ret < 0)
>> goto error_clocks;
>>
>> +ret = simplefb_attach_genpds(par, pdev);
>> +if (ret < 0)
>> +goto error_regulators;
>
> With the error case specified above, not specifying power-domains (which
> is valid according to dtschema) causes the entire driver to fail
> whenever there are no power domains in the device tree.
>
> On google-sargo, this causes a bug where the framebuffer fails to probe:
>
> [0.409290] simple-framebuffer 9c00.framebuffer: failed to parse
> power-domains: -2
> [0.409340] simple-framebuffer: probe of 9c00.framebuffer failed
> with error -2
Ok so this is a problem, sorry for not catching this during review.
I believe that this should be fixed by changing the code to:
err = of_count_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells");
if (err < 0) {
if (err == -ENOENT)
return 0;
dev_info(dev, "failed to parse power-domains: %d\n", err);
return err;
}
Can you submit a (tested) patch fixing this? Then I'll push it
to drm-misc-next right away.
Regards,
Hans
Re: [rft, PATCH v4 00/16] drm/i915/dsi: 4th attempt to get rid of IOSF GPIO
Hi, On 11/3/23 21:18, Andy Shevchenko wrote: > DSI code for VBT has a set of ugly GPIO hacks, one of which is direct > talking to GPIO IP behind the actual driver's back. A second attempt > to fix that is here. > > If I understood correctly, my approach should work in the similar way as > the current IOSF GPIO. > > Hans, I believe you have some devices that use this piece of code, > is it possible to give a test run on (one of) them? Ok, this now has been testen on both a BYT and a CHT device which actually use GPIO controls in their MIPI sequences so this series is: Tested-by: Hans de Goede And the code of the entire series also looks good to me: Reviewed-by: Hans de Goede for the series. Regards, Hans > In v4: > - fixed compile time errors in patch 14 (Hans, LKP) > - fixed cover letter Subject > - added patch 15 (as suggested by Ville) > - added Ack tag (Jani) > > In v3: > - incorporated series by Jani > - incorporated couple of precursor patches by Hans > - added Rb tag for used to be first three patches (Andi) > - rebased on top of the above changes > - fixed indexing for multi-community devices, such as Cherry View > > In v2: > - added a few cleanup patches > - reworked to use dynamic GPIO lookup tables > - converted CHV as well > > Andy Shevchenko (9): > drm/i915/dsi: Replace while(1) with one with clear exit condition > drm/i915/dsi: Get rid of redundant 'else' > drm/i915/dsi: Replace check with a (missing) MIPI sequence name > drm/i915/dsi: Extract common soc_gpio_set_value() helper > drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back > drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO > communities > drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back > drm/i915/dsi: Combine checks in mipi_exec_gpio() > drm/i915/iosf: Drop unused APIs > > Hans de Goede (2): > drm/i915/dsi: Remove GPIO lookup table at the end of > intel_dsi_vbt_gpio_init() > drm/i915/dsi: Fix wrong initial value for GPIOs in > bxt_gpio_set_value() > > Jani Nikula (5): > drm/i915/dsi: assume BXT gpio works for non-native GPIO > drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915 > drm/i915/dsi: clarify GPIO exec sequence > drm/i915/dsi: rename platform specific *_exec_gpio() to > *_gpio_set_value() > drm/i915/dsi: bxt/icl GPIO set value do not need gpio source > > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 355 +++ > drivers/gpu/drm/i915/vlv_sideband.c | 17 - > drivers/gpu/drm/i915/vlv_sideband.h | 3 - > 3 files changed, 136 insertions(+), 239 deletions(-) >
Re: [PATCH] drm/panel-orientation-quirks: add Lenovo Legion Go
Hi Brenton, On 11/15/23 16:52, Brenton Simpson wrote: > Yes, thanks! > > That's the email attached to my public git work, so it should be the > one here as well. Ok, I've pushed this to drm-misc-fixes now, thank you for the patch. > Sorry for the hassle. Very new to sending PRs over email, and still > working through the kinks. Your initial submission was almost there. Just the weird thing with the + in the From: email-address. What also threw me off is that the From was: Normally the '+' + the part after it gets thrown away to get the canonical email address, so that would make your canonical email: where I now know it should be: . So next time you use a + address please do something like: I also guess you never got any of the replies addressed to since the mail server presumably has tried to deliver those to , but because your normal email was in the Cc things still worked out. Either way for future patches note that using "git send-email" is the advised way to submit kernel patches. Regards, Hans
Re: [PATCH] drm/panel-orientation-quirks: add Lenovo Legion Go
Hi,
On 11/15/23 16:48, Brenton Simpson wrote:
> Resending from the email address linked to my GitHub account.
Ok, this doesn't really help. I'll just fix-up the author
field of the original patch.
Do understand correctly that both the author and the Signed-off-by
should be set to:
Brenton Simpson
?
Regards,
Hans
>
> -- >8 --
>
> The Legion Go has a 2560x1600 portrait screen, with the native "up" facing =
> the right controller (90=C2=B0 CW from the rest of the device).
>
> Signed-off-by: Brenton Simpson
> ---
> drivers/gpu/drm/drm_panel_orientation_quirks.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c b/drivers/gpu/d=
> rm/drm_panel_orientation_quirks.c
> index d5c1529..3d92f66 100644
> --- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> @@ -336,6 +336,12 @@ static const struct dmi_system_id orientation_data[] =
> =3D {
> =09=09 DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "IdeaPad Duet 3 10IGL5"),
> =09=09},
> =09=09.driver_data =3D (void *)_rightside_up,
> +=09}, {=09/* Lenovo Legion Go 8APU1 */
> +=09=09.matches =3D {
> +=09=09 DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +=09=09 DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "Legion Go 8APU1"),
> +=09=09},
> +=09=09.driver_data =3D (void *)_leftside_up,
> =09}, {=09/* Lenovo Yoga Book X90F / X90L */
> =09=09.matches =3D {
> =09=09 DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
> --=20
> 2.43.0.rc0.421.g78406f8d94-goog
>
Re: [REGRESSION]: acpi/nouveau: Hardware unavailable upon resume or suspend fails
Hi, On 11/10/23 17:58, Owen T. Heisler wrote: > Hi everyone, > > On 11/10/23 06:52, Kai-Heng Feng wrote: >> On Fri, Nov 10, 2023 at 2:19 PM Hans de Goede wrote: >>> On 11/10/23 07:09, Kai-Heng Feng wrote: >>>> On Fri, Nov 10, 2023 at 5:55 AM Owen T. Heisler wrote: >>>>> #regzbot introduced: 89c290ea758911e660878e26270e084d862c03b0 >>>>> #regzbot link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/273 >>>>> #regzbot link: https://bugzilla.kernel.org/show_bug.cgi?id=218124 >>>> >>>> Thanks for the bug report. Do you prefer to continue the discussion >>>> here, on gitlab or on bugzilla? > > Kai-Heng, you're welcome and thank you too. By email is fine with me. > >>> Owen, as Kai-Heng said thank you for reporting this. > > Hans, you're welcome, and thanks for your help too. > >>>>> ## Reproducing >>>>> >>>>> 1. Boot system to framebuffer console. >>>>> 2. Run `systemctl suspend`. If undocked without secondary display, >>>>> suspend fails. If docked with secondary display, suspend succeeds. >>>>> 3. Resume from suspend if applicable. >>>>> 4. System is now in a broken state. >>>> >>>> So I guess we need to put those devices to ACPI D3 for suspend. Let's >>>> discuss this on your preferred platform. >>> >>> Ok, so I was already sort of afraid we might see something like this >>> happening because of: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=89c290ea758911e660878e26270e084d862c03b0 >>> >>> As I mentioned during the review of that, it might be better to >>> not touch the video-card ACPI power-state at all and instead >>> only do acpi_device_fix_up_power() on the child devices. >> >> Or the child devices need to be put to D3 during suspend. >> >>> Owen, attached are 2 patches which implement only >>> calling acpi_device_fix_up_power() on the child devices, >>> can you build a v6.6 kernel with these 2 patches added >>> on top please and see if that fixes things ? > > Yes, with those patches v6.6 suspend works normally. That's great, thanks! > > I tested with v6.6 with the 2 patches at > <https://lore.kernel.org/regressions/a592ce0c-64f0-477d-80fa-8f5a52ba2...@redhat.com/> > using > <https://gitlab.freedesktop.org/drm/nouveau/uploads/788d7faf22ba2884dcc09d7be931e813/v6.6-config1>. > I tested both docked and un-docked, just in case. > > Tested-by: Owen T. Heisler > >>> Kai-Heng can you test that the issue on the HP ZBook Fury 16 G10 >>> is still resolved after applying these patches ? >> >> Yes. Thanks for the patch. >> >> If this patch also fixes Owen's issue, then >> Tested-by: Kai-Heng Feng
Re: [REGRESSION]: acpi/nouveau: Hardware unavailable upon resume or suspend fails
Hi All,
On 11/10/23 07:09, Kai-Heng Feng wrote:
> Hi Owen,
>
> On Fri, Nov 10, 2023 at 5:55 AM Owen T. Heisler wrote:
>>
>> #regzbot introduced: 89c290ea758911e660878e26270e084d862c03b0
>> #regzbot link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/273
>> #regzbot link: https://bugzilla.kernel.org/show_bug.cgi?id=218124
>
> Thanks for the bug report. Do you prefer to continue the discussion
> here, on gitlab or on bugzilla?
Owen, as Kai-Heng said thank you for reporting this.
>> ## Reproducing
>>
>> 1. Boot system to framebuffer console.
>> 2. Run `systemctl suspend`. If undocked without secondary display,
>> suspend fails. If docked with secondary display, suspend succeeds.
>> 3. Resume from suspend if applicable.
>> 4. System is now in a broken state.
>
> So I guess we need to put those devices to ACPI D3 for suspend. Let's
> discuss this on your preferred platform.
Ok, so I was already sort of afraid we might see something like this
happening because of:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=89c290ea758911e660878e26270e084d862c03b0
As I mentioned during the review of that, it might be better to
not touch the video-card ACPI power-state at all and instead
only do acpi_device_fix_up_power() on the child devices.
Owen, attached are 2 patches which implement only
calling acpi_device_fix_up_power() on the child devices,
can you build a v6.6 kernel with these 2 patches added
on top please and see if that fixes things ?
Kai-Heng can you test that the issue on the HP ZBook Fury 16 G10
is still resolved after applying these patches ?
Regards,
Hans
From 68a819101c580bb89f34a31196ace81244ca8eb5 Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Fri, 10 Nov 2023 12:52:39 +0100
Subject: [PATCH 1/2] ACPI: PM: Add acpi_device_fix_up_power_children()
function
In some cases it is necessary to fix-up the power-state of an ACPI
device's children without touching the ACPI device itself add
a new acpi_device_fix_up_power_children() function for this.
Signed-off-by: Hans de Goede
---
drivers/acpi/device_pm.c | 13 +
include/acpi/acpi_bus.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index f007116a8427..3b4d048c4941 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -397,6 +397,19 @@ void acpi_device_fix_up_power_extended(struct acpi_device *adev)
}
EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
+/**
+ * acpi_device_fix_up_power_children - Force a device's children into D0.
+ * @adev: Parent device object whose children's power state is to be fixed up.
+ *
+ * Call acpi_device_fix_up_power() for @adev's children so long as they
+ * are reported as present and enabled.
+ */
+void acpi_device_fix_up_power_children(struct acpi_device *adev)
+{
+ acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_children);
+
int acpi_device_update_power(struct acpi_device *device, int *state_p)
{
int state;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 254685085c82..0b7eab0ef7d7 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -539,6 +539,7 @@ int acpi_device_set_power(struct acpi_device *device, int state);
int acpi_bus_init_power(struct acpi_device *device);
int acpi_device_fix_up_power(struct acpi_device *device);
void acpi_device_fix_up_power_extended(struct acpi_device *adev);
+void acpi_device_fix_up_power_children(struct acpi_device *adev);
int acpi_bus_update_power(acpi_handle handle, int *state_p);
int acpi_device_update_power(struct acpi_device *device, int *state_p);
bool acpi_bus_power_manageable(acpi_handle handle);
--
2.41.0
From 33e2d55917ac7082e8d98dc2a678a856f8d48352 Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Fri, 10 Nov 2023 13:10:02 +0100
Subject: [PATCH 2/2] ACPI: video: Use acpi_device_fix_up_power_children()
Commit 89c290ea7589 ("ACPI: video: Put ACPI video and its child devices
into D0 on boot") introduced calling acpi_device_fix_up_power_extended()
on the video card for which the ACPI video bus is the companion device.
This unnecessarily touches the power-state of the GPU itself, while
the issue it tries to address only requires calling _PS0 on the child
devices.
Touching the power-state of the GPU itself is causing suspend / resume
issues on e.g. a Lenovo ThinkPad W530.
Instead use acpi_device_fix_up_power_children(), which only touches
the child devices, to fix this.
Fixes: 89c290ea7589 ("ACPI: video: Put ACPI video and its child devices into D0 on boot")
Reported-by: Owen T. Heisler
Closes: https://gitlab.freedesktop.org/drm/nouveau/-/issues/273
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218124
Signed-off-by: Hans de Goede
---
drivers/acpi/acpi_video.c | 2 +-
1 file changed, 1 insertion(+), 1
Re: [PATCH v2 6/9] PCI: Rename is_thunderbolt to is_tunneled
Hi,
On 11/3/23 20:07, Mario Limonciello wrote:
> The `is_thunderbolt` bit has been used to indicate that a PCIe device
> contained the Intel VSEC which is used by various parts of the kernel
> to change behavior. To later allow usage with USB4 controllers as well,
> rename this to `is_tunneled`.
>
> Signed-off-by: Mario Limonciello
Here is my ack for the trivial drivers/platform/x86/apple-gmux.c change:
Acked-by: Hans de Goede
Bjorn, feel free to route this through the PCI tree.
Regards,
Hans
> ---
> drivers/pci/pci.c | 2 +-
> drivers/pci/probe.c | 2 +-
> drivers/platform/x86/apple-gmux.c | 2 +-
> include/linux/pci.h | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 59c01d68c6d5..d9aa5a39f585 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3032,7 +3032,7 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> return true;
>
> /* Even the oldest 2010 Thunderbolt controller supports D3. */
> - if (bridge->is_thunderbolt)
> + if (bridge->is_tunneled)
> return true;
>
> /* Platform might know better if the bridge supports D3 */
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 795534589b98..518413d15402 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1597,7 +1597,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> /* Is the device part of a Thunderbolt controller? */
> vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
> PCI_VSEC_ID_INTEL_TBT);
> if (vsec)
> - dev->is_thunderbolt = 1;
> + dev->is_tunneled = 1;
> }
>
> static void set_pcie_untrusted(struct pci_dev *dev)
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 1417e230edbd..20315aa4463a 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -774,7 +774,7 @@ static int gmux_resume(struct device *dev)
>
> static int is_thunderbolt(struct device *dev, void *data)
> {
> - return to_pci_dev(dev)->is_thunderbolt;
> + return to_pci_dev(dev)->is_tunneled;
> }
>
> static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 439c2dac8a3e..b1724f25fb02 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -440,7 +440,7 @@ struct pci_dev {
> unsigned intis_virtfn:1;
> unsigned intis_hotplug_bridge:1;
> unsigned intshpc_managed:1; /* SHPC owned by shpchp */
> - unsigned intis_thunderbolt:1; /* Thunderbolt controller */
> + unsigned intis_tunneled:1; /* Tunneled TBT or USB4 link */
> unsigned intno_command_complete:1; /* No command completion */
> /*
>* Devices marked being untrusted are the ones that can potentially
Re: [PATCH v3 14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi,
On 11/2/23 16:12, Andy Shevchenko wrote:
> It's a dirty hack in the driver that pokes GPIO registers behind
> the driver's back. Moreoever it might be problematic as simultaneous
> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> cherryview: prevent concurrent access to GPIO controllers") for
> the details. Taking all this into consideration replace the hack
> with proper GPIO APIs being used.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +---
> 1 file changed, 10 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index b1736c1301ea..ffc65c943b11 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
> #define CHV_GPIO_IDX_START_SW100
> #define CHV_GPIO_IDX_START_SE198
>
> -#define CHV_VBT_MAX_PINS_PER_FMLY15
> -
> -#define CHV_GPIO_PAD_CFG0(f, i) (0x4400 + (f) * 0x400 + (i) * 8)
> -#define CHV_GPIO_GPIOEN (1 << 15)
> -#define CHV_GPIO_GPIOCFG_GPIO (0 << 8)
> -#define CHV_GPIO_GPIOCFG_GPO(1 << 8)
> -#define CHV_GPIO_GPIOCFG_GPI(2 << 8)
> -#define CHV_GPIO_GPIOCFG_HIZ(3 << 8)
> -#define CHV_GPIO_GPIOTXSTATE(state) ((!!(state)) << 1)
> -
> -#define CHV_GPIO_PAD_CFG1(f, i) (0x4400 + (f) * 0x400 + (i) * 8
> + 4)
> -#define CHV_GPIO_CFGLOCK(1 << 31)
> -
> /* ICL DSI Display GPIO Pins */
> #define ICL_GPIO_DDSP_HPD_A 0
> #define ICL_GPIO_L_VDDEN_1 1
> @@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector
> *connector,
> u8 gpio_source, u8 gpio_index, bool value)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> - u16 cfg0, cfg1;
> - u16 family_num;
> - u8 port;
>
> if (connector->panel.vbt.dsi.seq_version >= 3) {
> if (gpio_index >= CHV_GPIO_IDX_START_SE) {
> /* XXX: it's unclear whether 255->57 is part of SE. */
> - gpio_index -= CHV_GPIO_IDX_START_SE;
> - port = CHV_IOSF_PORT_GPIO_SE;
> + soc_exec_opaque_gpio(connector, gpio_index,
> "INT33FF:03", "Panel SE",
> + gpio_index -
> CHV_GPIO_IDX_START_SW, value);
The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index -
CHV_GPIO_IDX_START_SE".
Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to
compile ...
Regards,
Hans
> } else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
> - gpio_index -= CHV_GPIO_IDX_START_SW;
> - port = CHV_IOSF_PORT_GPIO_SW;
> + soc_exec_opaque_gpio(connector, gpio_index,
> "INT33FF:00", "Panel SW",
> + gpio_index -
> CHV_GPIO_IDX_START_SW, value);
> } else if (gpio_index >= CHV_GPIO_IDX_START_E) {
> - gpio_index -= CHV_GPIO_IDX_START_E;
> - port = CHV_IOSF_PORT_GPIO_E;
> + soc_exec_opaque_gpio(connector, gpio_index,
> "INT33FF:02", "Panel E",
> + gpio_index - CHV_GPIO_IDX_START_E,
> value);
> } else {
> - port = CHV_IOSF_PORT_GPIO_N;
> + soc_exec_opaque_gpio(connector, gpio_index,
> "INT33FF:01", "Panel N",
> + gpio_index - CHV_GPIO_IDX_START_N,
> value);
> }
> } else {
> /* XXX: The spec is unclear about CHV GPIO on seq v2 */
> @@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector
> *connector,
> return;
> }
>
> - port = CHV_IOSF_PORT_GPIO_N;
> + soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01",
> "Panel N",
> + gpio_index - CHV_GPIO_IDX_START_N, value);
> }
> -
> - family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
> - gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
> -
> - cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
> - cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
> -
> - vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
> - vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
> - vlv_iosf_sb_write(dev_priv, port, cfg0,
> - CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
> - CHV_GPIO_GPIOTXSTATE(value));
> - vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
> }
>
> static void bxt_gpio_set_value(struct intel_connector *connector,
Re: [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi,
On 11/2/23 15:19, Andy Shevchenko wrote:
> On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote:
>> On 11/1/23 10:32, Andy Shevchenko wrote:
>>> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>>>> On 10/31/23 17:07, Hans de Goede wrote:
>>>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>
> ...
>
>> Note you still need the first part of my patch which is
>> an unrelated bugfix:
>>
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector
>> *connector, const char *con_id,
>> } else {
>> gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
>> con_id, gpio_index,
>> - value ? GPIOD_OUT_LOW :
>> - GPIOD_OUT_HIGH);
>> + value ? GPIOD_OUT_HIGH :
>> GPIOD_OUT_LOW);
>> if (IS_ERR(gpio_desc)) {
>> drm_err(_priv->drm,
>> "GPIO index %u request failed (%pe)\n",
>
> Can you attach or send a formal submission, so I can incorporate it into one
> (v3) series among other changes?
I thought this fixed new code in your series and it is a trivial fix,
so my idea was that you would just fold the fix into the patch
introducing the code.
But I see now that this is existing code from bxt_exec_gpio().
A formal fix to use as a prep patch for your series is now attached,
this is based on top of drm-misc-next (I guess drm-intel-next
would be better but I had an up2date drm-misc-next handy).
Regards,
Hans
From c300ed0e09d1fd14bf966dc172c6db54b888faf3 Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Wed, 1 Nov 2023 11:49:01 +0100
Subject: [PATCH] drm/i915/dsi: Fix wrong initial value for GPIOs in
bxt_exec_gpio()
Fix wrong initial value for GPIOs in bxt_exec_gpio().
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index e56ec3f2d84a..0587cbc2e584 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -356,9 +356,7 @@ static void bxt_exec_gpio(struct intel_connector *connector,
if (!gpio_desc) {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
NULL, gpio_index,
- value ? GPIOD_OUT_LOW :
- GPIOD_OUT_HIGH);
-
+ value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
if (IS_ERR_OR_NULL(gpio_desc)) {
drm_err(_priv->drm,
"GPIO index %u request failed (%ld)\n",
--
2.41.0
Re: [PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features
Hi, On 11/1/23 18:54, Hans de Goede wrote: > Hi, > > On 11/1/23 18:20, Thierry Reding wrote: >> From: Thierry Reding >> >> Hi, >> >> This contains two patches that bring simplefb up to feature parity with >> simpledrm. The patches add support for the "memory-region" property that >> provides an alternative to the "reg" property to describe the memory >> used for the framebuffer and allow attaching the simple-framebuffer >> device to one or more generic power domains to make sure they aren't >> turned off during the boot process and take down the display >> configuration. >> >> Changes in v2: >> - remove unnecessary call to simplefb_detach_genpds() since that's >> already done automatically by devres >> - fix crash if power-domains property is missing in DT > > Thanks, the new version looks good to me: > > Reviewed-by: Hans de Goede > > for the series. > > Helge, will you pick these 2 up, or shall I push them to drm-misc-fixes? I have pushed this to drm-misc-next now. I now I said drm-misc-fixes at first, but on a second look these really are not fixes, so getting them in mainline will have to wait to the next merge-window. Regards, Hans
Re: [PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features
Hi, On 11/1/23 18:20, Thierry Reding wrote: > From: Thierry Reding > > Hi, > > This contains two patches that bring simplefb up to feature parity with > simpledrm. The patches add support for the "memory-region" property that > provides an alternative to the "reg" property to describe the memory > used for the framebuffer and allow attaching the simple-framebuffer > device to one or more generic power domains to make sure they aren't > turned off during the boot process and take down the display > configuration. > > Changes in v2: > - remove unnecessary call to simplefb_detach_genpds() since that's > already done automatically by devres > - fix crash if power-domains property is missing in DT Thanks, the new version looks good to me: Reviewed-by: Hans de Goede for the series. Helge, will you pick these 2 up, or shall I push them to drm-misc-fixes? Regards, Hans > Thierry Reding (2): > fbdev/simplefb: Support memory-region property > fbdev/simplefb: Add support for generic power-domains > > drivers/video/fbdev/simplefb.c | 128 +++-- > 1 file changed, 123 insertions(+), 5 deletions(-) >
Re: [PATCH 2/2] fbdev/simplefb: Add support for generic power-domains
Hi,
On 11/1/23 17:50, Thierry Reding wrote:
> On Thu, Oct 26, 2023 at 02:50:27PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> Thank you for your patches.
>>
>> On 10/11/23 16:38, Thierry Reding wrote:
>>> From: Thierry Reding
>>>
>>> The simple-framebuffer device tree bindings document the power-domains
>>> property, so make sure that simplefb supports it. This ensures that the
>>> power domains remain enabled as long as simplefb is active.
>>>
>>> Signed-off-by: Thierry Reding
>>> ---
>>> drivers/video/fbdev/simplefb.c | 93 +-
>>> 1 file changed, 91 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>> index 18025f34fde7..e69fb0ad2d54 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -25,6 +25,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>>
>>> static const struct fb_fix_screeninfo simplefb_fix = {
>>> @@ -78,6 +79,11 @@ struct simplefb_par {
>>> unsigned int clk_count;
>>> struct clk **clks;
>>> #endif
>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>> + unsigned int num_genpds;
>>> + struct device **genpds;
>>> + struct device_link **genpd_links;
>>> +#endif
>>> #if defined CONFIG_OF && defined CONFIG_REGULATOR
>>> bool regulators_enabled;
>>> u32 regulator_count;
>>> @@ -432,6 +438,83 @@ static void simplefb_regulators_enable(struct
>>> simplefb_par *par,
>>> static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>>> #endif
>>>
>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>> +static void simplefb_detach_genpds(void *res)
>>> +{
>>> + struct simplefb_par *par = res;
>>> + unsigned int i = par->num_genpds;
>>> +
>>> + if (par->num_genpds <= 1)
>>> + return;
>>> +
>>> + while (i--) {
>>> + if (par->genpd_links[i])
>>> + device_link_del(par->genpd_links[i]);
>>> +
>>> + if (!IS_ERR_OR_NULL(par->genpds[i]))
>>> + dev_pm_domain_detach(par->genpds[i], true);
>>> + }
>>
>> Using this i-- construct means that genpd at index 0 will
>> not be cleaned up.
>
> This is actually a common variant to clean up in reverse order. You'll
> find this used a lot in core code and so on. It has the advantage that
> you can use it to unwind (not the case here) because i will already be
> set to the right value, typically. It's also nice because it works for
> unsigned integers.
>
> Note that this uses the postfix decrement, so evaluation happens before
> the decrement and therefore the last iteration of the loop will run with
> i == 0. For unsigned integers this also means that after the loop the
> variable will actually have wrapped around, but that's usually not a
> problem since it isn't used after this point anymore.
Ah yes you are right, I messed the post-decrement part.
I got confused when I compaired this to the simpledrm code
which uses the other construct.
>
>>> static int simplefb_probe(struct platform_device *pdev)
>>> {
>>> int ret;
>>> @@ -518,6 +601,10 @@ static int simplefb_probe(struct platform_device *pdev)
>>> if (ret < 0)
>>> goto error_clocks;
>>>
>>> + ret = simplefb_attach_genpd(par, pdev);
>>> + if (ret < 0)
>>> + goto error_regulators;
>>> +
>>> simplefb_clocks_enable(par, pdev);
>>> simplefb_regulators_enable(par, pdev);
>>>
>>> @@ -534,18 +621,20 @@ static int simplefb_probe(struct platform_device
>>> *pdev)
>>> ret = devm_aperture_acquire_for_platform_device(pdev, par->base,
>>> par->size);
>>> if (ret) {
>>> dev_err(>dev, "Unable to acquire aperture: %d\n", ret);
>>> - goto error_regulators;
>>> + goto error_genpds;
>>
>> This is not necessary since simplefb_attach_genpd() ends with:
>>
>> devm_add_action_or_reset(dev, simplefb_detach_genpds, par)
>>
>> Which causes simplefb_detach_genpds() to run when probe() fails.
>
> Yes, you're right. I've removed all these explicit cleanup paths since
> they are not needed.
>
>>
>>> }
>>> ret = register_framebuffer(info);
>>> if (ret < 0) {
>>> dev_err(>dev, "Unable to register simplefb: %d\n", ret);
>>> - goto error_regulators;
>>> + goto error_genpds;
>>> }
>>>
>>> dev_info(>dev, "fb%d: simplefb registered!\n", info->node);
>>>
>>> return 0;
>>>
>>> +error_genpds:
>>> + simplefb_detach_genpds(par);
>>
>> As the kernel test robot (LKP) already pointed out this is causing
>> compile errors when #if defined CONFIG_OF && defined
>> CONFIG_PM_GENERIC_DOMAINS
>> evaluates as false.
>>
>> Since there is no simplefb_detach_genpds() stub in the #else, but as
>> mentioned above this is not necessary so just remove it.
>
> Yep, done.
Great, thank you.
Regards,
Hans
Re: [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi,
On 11/1/23 11:20, Hans de Goede wrote:
> Hi,
>
> On 11/1/23 10:32, Andy Shevchenko wrote:
>> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>>> On 10/31/23 17:07, Hans de Goede wrote:
>>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>>
>> ...
>>
>>>> As for the CHT support, I have not added that to my tree yet, I would
>>>> prefer to directly test the correct/fixed patch.
>>>
>>> And I hit the "jackpot" on the first device I tried and the code needed
>>> some fixing to actually work, so here is something to fold into v3 to
>>> fix things:
>>
>> Thanks!
>>
>> But let me first send current v3 as it quite differs to v2 in the sense
>> of how I do instantiate GPIO lookup tables.
>
> The problem is there already is a GPIO lookup table registered for
> the ":00:02.0" device by intel_dsi_vbt_gpio_init() and there can
> be only be one GPIO lookup table per device. So no matter how you
> instantiate GPIO lookup tables it will not work.
>
> The solution that I chose is to not instantiate a GPIO lookup table
> at all and instead to extend the existing table with an extra entry.
>
> Although thinking more about it I must admit that this is racy.
>
> So a better idea would be to unregister the GPIO lookup
> table registered by intel_dsi_vbt_gpio_init() after getting
> the GPIOs there, that would allow instantiating a new one
> from soc_exec_opaque_gpio() as it currently does and that
> would be race free.
>
>> Meanwhile I will look into the change you sent (and hopefully we can
>> incorporate something in v3 for v4).
>
> Ok, lets go with your v3.
>
> I'll prepare a patch to move the unregistering of the existing
> conflicting GPIO lookup from intel_dsi_vbt_gpio_cleanup()
> to the end of intel_dsi_vbt_gpio_init() to avoid the conflict
> we have there.
Attached is this patch, this should probably be one of
the first patches in the v3 submission.
Note that if you go with Ville's suggestion to preparse
the MIPI sequences, things will change significantly
and then the attached patch will likely be unnecessary.
Regards,
Hans
> Note you still need the first part of my patch which is
> an unrelated bugfix:
>
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector
> *connector, const char *con_id,
> } else {
> gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
>con_id, gpio_index,
> - value ? GPIOD_OUT_LOW :
> - GPIOD_OUT_HIGH);
> + value ? GPIOD_OUT_HIGH :
> GPIOD_OUT_LOW);
> if (IS_ERR(gpio_desc)) {
> drm_err(_priv->drm,
> "GPIO index %u request failed (%pe)\n",
>
> Regards,
>
> Hans
>
>
From cb3cc656ad89d98824d38c07a35019c512227601 Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Wed, 1 Nov 2023 11:54:18 +0100
Subject: [PATCH] drm/i915/dsi: Remove GPIO lookup table at the end of
intel_dsi_vbt_gpio_init()
To properly deal with GPIOs used in MIPI panel sequences a temporary
GPIO lookup will be used. Since there can only be 1 GPIO lookup table
for the ":00:02.0" device this will not work if the GPIO lookup
table used by intel_dsi_vbt_gpio_init() is still registered.
After getting the "backlight" and "panel" GPIOs the lookup table
registered by intel_dsi_vbt_gpio_init() is no longer necessary,
remove it so that another temporary lookup-table for the ":00:02.0"
device can be added.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 25 +++-
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index cb64454932d1..d965ae1d2b08 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -922,6 +922,7 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
struct intel_connector *connector = intel_dsi->attached_connector;
struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+ struct gpiod_lookup_table *gpiod_lookup_table = NULL;
bool want_backlight_gpio = false;
bool want_panel_gpio = false;
struct pinctrl
Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi, On 11/1/23 11:34, Ville Syrjälä wrote: > On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote: >> Hi, >> >> On 11/1/23 10:32, Andy Shevchenko wrote: >>> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote: >>>> On 10/31/23 17:07, Hans de Goede wrote: >>>>> On 10/24/23 18:11, Andy Shevchenko wrote: >>>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote: >>> >>> ... >>> >>>>> As for the CHT support, I have not added that to my tree yet, I would >>>>> prefer to directly test the correct/fixed patch. >>>> >>>> And I hit the "jackpot" on the first device I tried and the code needed >>>> some fixing to actually work, so here is something to fold into v3 to >>>> fix things: >>> >>> Thanks! >>> >>> But let me first send current v3 as it quite differs to v2 in the sense >>> of how I do instantiate GPIO lookup tables. >> >> The problem is there already is a GPIO lookup table registered for >> the ":00:02.0" device by intel_dsi_vbt_gpio_init() and there can >> be only be one GPIO lookup table per device. So no matter how you >> instantiate GPIO lookup tables it will not work. >> >> The solution that I chose is to not instantiate a GPIO lookup table >> at all and instead to extend the existing table with an extra entry. >> >> Although thinking more about it I must admit that this is racy. >> >> So a better idea would be to unregister the GPIO lookup >> table registered by intel_dsi_vbt_gpio_init() after getting >> the GPIOs there, that would allow instantiating a new one >> from soc_exec_opaque_gpio() as it currently does and that >> would be race free. > > The proper solution would likely be be to pre-parse the sequences > to determine which GPIOs are actually needed. That would also get > rid of the bxt_gpio_table[] eyesore. Interesting suggestion. Note that intel_dsi_vbt_gpio_init() arm only runs on byt and cht though, so that is something to keep in mind. Regards, Hans
Re: [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi,
On 11/1/23 10:32, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>> On 10/31/23 17:07, Hans de Goede wrote:
>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>
> ...
>
>>> As for the CHT support, I have not added that to my tree yet, I would
>>> prefer to directly test the correct/fixed patch.
>>
>> And I hit the "jackpot" on the first device I tried and the code needed
>> some fixing to actually work, so here is something to fold into v3 to
>> fix things:
>
> Thanks!
>
> But let me first send current v3 as it quite differs to v2 in the sense
> of how I do instantiate GPIO lookup tables.
The problem is there already is a GPIO lookup table registered for
the ":00:02.0" device by intel_dsi_vbt_gpio_init() and there can
be only be one GPIO lookup table per device. So no matter how you
instantiate GPIO lookup tables it will not work.
The solution that I chose is to not instantiate a GPIO lookup table
at all and instead to extend the existing table with an extra entry.
Although thinking more about it I must admit that this is racy.
So a better idea would be to unregister the GPIO lookup
table registered by intel_dsi_vbt_gpio_init() after getting
the GPIOs there, that would allow instantiating a new one
from soc_exec_opaque_gpio() as it currently does and that
would be race free.
> Meanwhile I will look into the change you sent (and hopefully we can
> incorporate something in v3 for v4).
Ok, lets go with your v3.
I'll prepare a patch to move the unregistering of the existing
conflicting GPIO lookup from intel_dsi_vbt_gpio_cleanup()
to the end of intel_dsi_vbt_gpio_init() to avoid the conflict
we have there.
Note you still need the first part of my patch which is
an unrelated bugfix:
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector
*connector, const char *con_id,
} else {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
con_id, gpio_index,
-value ? GPIOD_OUT_LOW :
-GPIOD_OUT_HIGH);
+value ? GPIOD_OUT_HIGH :
GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(_priv->drm,
"GPIO index %u request failed (%pe)\n",
Regards,
Hans
Re: [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi,
On 10/31/23 17:07, Hans de Goede wrote:
> Hi Andy,
>
> On 10/24/23 18:11, Andy Shevchenko wrote:
>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>>> It's a dirty hack in the driver that pokes GPIO registers behind
>>> the driver's back. Moreoever it might be problematic as simultaneous
>>> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
>>> cherryview: prevent concurrent access to GPIO controllers") for
>>> the details. Taking all this into consideration replace the hack
>>> with proper GPIO APIs being used.
>>
>> Ah, just realised that this won't work if it happens to request to GPIOs with
>> the same index but different communities. I will fix that in v3, but will
>> wait
>> for Hans to test VLV and it might even work in most of the cases on CHV as it
>> seems quite unlikely that the above mentioned assertion is going to happen in
>> real life.
>
> I have added patches 1-5 to my personal tree + a small debug patch on top
> which logs when soc_exec_opaque_gpio() actually gets called.
>
> So these patches will now get run every time I run some tests on
> one my tablets.
>
> I'll get back to you with testing results when I've found a device where
> the new soc_exec_opaque_gpio() actually gets called.
>
> As for the CHT support, I have not added that to my tree yet, I would
> prefer to directly test the correct/fixed patch.
And I hit the "jackpot" on the first device I tried and the code needed
some fixing to actually work, so here is something to fold into v3 to
fix things:
>From 144fae4de91a6b5ed993b1722a07cca679f74cbe Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Tue, 31 Oct 2023 17:04:35 +0100
Subject: [PATCH] drm/i915/dsi: Fix GPIO lookup table used by
soc_exec_opaque_gpio()
There already is a GPIO lookup table for device ":00:02.0" and
there can be only one GPIO lookup per device.
Instead add an extra empty entry to the GPIO lookup table
registered by intel_dsi_vbt_gpio_init() and use that extra entry
in soc_exec_opaque_gpio().
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 60 ++--
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8fc82aceae14..70f1d2c411e8 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector
*connector, const char *con_id,
} else {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
con_id, gpio_index,
-value ? GPIOD_OUT_LOW :
-GPIOD_OUT_HIGH);
+value ? GPIOD_OUT_HIGH :
GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(_priv->drm,
"GPIO index %u request failed (%pe)\n",
@@ -232,26 +231,20 @@ static void soc_exec_gpio(struct intel_connector
*connector, const char *con_id,
}
}
+static struct gpiod_lookup *soc_exec_opaque_gpiod_lookup;
+
static void soc_exec_opaque_gpio(struct intel_connector *connector,
const char *chip, const char *con_id,
u8 gpio_index, bool value)
{
- struct gpiod_lookup_table *lookup;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
- lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
- if (!lookup)
- return;
-
- lookup->dev_id = ":00:02.0";
- lookup->table[0] =
+ *soc_exec_opaque_gpiod_lookup =
GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index,
GPIO_ACTIVE_HIGH);
- gpiod_add_lookup_table(lookup);
-
soc_exec_gpio(connector, con_id, gpio_index, value);
- gpiod_remove_lookup_table(lookup);
- kfree(lookup);
+ soc_exec_opaque_gpiod_lookup->key = NULL;
}
static void vlv_exec_gpio(struct intel_connector *connector,
@@ -898,6 +891,7 @@ static struct gpiod_lookup_table pmic_panel_gpio_table = {
.table = {
/* Panel EN/DISABLE */
GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
{ }
},
};
@@ -907,6 +901,15 @@ static struct gpiod_lookup_table soc_panel_gpio_table = {
.table = {
GPIO_LOOKUP("INT33FC:01", 10, "backlight", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("I
Re: [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Hi Andy,
On 10/24/23 18:11, Andy Shevchenko wrote:
> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>> It's a dirty hack in the driver that pokes GPIO registers behind
>> the driver's back. Moreoever it might be problematic as simultaneous
>> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
>> cherryview: prevent concurrent access to GPIO controllers") for
>> the details. Taking all this into consideration replace the hack
>> with proper GPIO APIs being used.
>
> Ah, just realised that this won't work if it happens to request to GPIOs with
> the same index but different communities. I will fix that in v3, but will wait
> for Hans to test VLV and it might even work in most of the cases on CHV as it
> seems quite unlikely that the above mentioned assertion is going to happen in
> real life.
I have added patches 1-5 to my personal tree + a small debug patch on top
which logs when soc_exec_opaque_gpio() actually gets called.
So these patches will now get run every time I run some tests on
one my tablets.
I'll get back to you with testing results when I've found a device where
the new soc_exec_opaque_gpio() actually gets called.
As for the CHT support, I have not added that to my tree yet, I would
prefer to directly test the correct/fixed patch.
Regards,
Hans
Re: [PATCH 2/2] fbdev/simplefb: Add support for generic power-domains
Hi,
Thank you for your patches.
On 10/11/23 16:38, Thierry Reding wrote:
> From: Thierry Reding
>
> The simple-framebuffer device tree bindings document the power-domains
> property, so make sure that simplefb supports it. This ensures that the
> power domains remain enabled as long as simplefb is active.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/video/fbdev/simplefb.c | 93 +-
> 1 file changed, 91 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 18025f34fde7..e69fb0ad2d54 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> static const struct fb_fix_screeninfo simplefb_fix = {
> @@ -78,6 +79,11 @@ struct simplefb_par {
> unsigned int clk_count;
> struct clk **clks;
> #endif
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + unsigned int num_genpds;
> + struct device **genpds;
> + struct device_link **genpd_links;
> +#endif
> #if defined CONFIG_OF && defined CONFIG_REGULATOR
> bool regulators_enabled;
> u32 regulator_count;
> @@ -432,6 +438,83 @@ static void simplefb_regulators_enable(struct
> simplefb_par *par,
> static void simplefb_regulators_destroy(struct simplefb_par *par) { }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +static void simplefb_detach_genpds(void *res)
> +{
> + struct simplefb_par *par = res;
> + unsigned int i = par->num_genpds;
> +
> + if (par->num_genpds <= 1)
> + return;
> +
> + while (i--) {
> + if (par->genpd_links[i])
> + device_link_del(par->genpd_links[i]);
> +
> + if (!IS_ERR_OR_NULL(par->genpds[i]))
> + dev_pm_domain_detach(par->genpds[i], true);
> + }
Using this i-- construct means that genpd at index 0 will
not be cleaned up.
I think it would be best to instead use the same construct
as the simpledrm version of this which makes i signed
(and does not initialize it) and then does:
for (i = sdev->pwr_dom_count - 1; i >= 0; i--) { .. }
> +}
> +
> +static int simplefb_attach_genpd(struct simplefb_par *par,
> + struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + unsigned int i;
> + int err;
> +
> + par->num_genpds = of_count_phandle_with_args(dev->of_node,
> + "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by the driver core, so
> + * nothing to do here.
> + */
> + if (par->num_genpds <= 1)
> + return 0;
> +
> + par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
> +GFP_KERNEL);
> + if (!par->genpds)
> + return -ENOMEM;
> +
> + par->genpd_links = devm_kcalloc(dev, par->num_genpds,
> + sizeof(*par->genpd_links),
> + GFP_KERNEL);
> + if (!par->genpd_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < par->num_genpds; i++) {
> + par->genpds[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(par->genpds[i])) {
> + err = PTR_ERR(par->genpds[i]);
> + if (err == -EPROBE_DEFER) {
> + simplefb_detach_genpds(par);
> + return err;
> + }
> +
> + dev_warn(dev, "failed to attach domain %u: %d\n", i,
> err);
> + continue;
> + }
> +
> + par->genpd_links[i] = device_link_add(dev, par->genpds[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!par->genpd_links[i])
> + dev_warn(dev, "failed to link power-domain %u\n", i);
> + }
> +
> + return devm_add_action_or_reset(dev, simplefb_detach_genpds, par);
> +}
> +#else
> +static int simplefb_attach_genpd(struct simplefb_par *par,
> + struct platform_device *pdev)
> +{
> + return 0;
> +}
> +#endif
> +
> static int simplefb_probe(struct platform_device *pdev)
> {
> int ret;
> @@ -518,6 +601,10 @@ static int simplefb_probe(struct platform_device *pdev)
> if (ret < 0)
> goto error_clocks;
>
> + ret = simplefb_attach_genpd(par, pdev);
> + if (ret < 0)
> + goto error_regulators;
> +
> simplefb_clocks_enable(par, pdev);
> simplefb_regulators_enable(par, pdev);
>
> @@ -534,18 +621,20 @@ static int
Re: [PATCH 1/2] fbdev/simplefb: Support memory-region property
Hi,
On 10/11/23 16:38, Thierry Reding wrote:
> From: Thierry Reding
>
> The simple-framebuffer bindings specify that the "memory-region"
> property can be used as an alternative to the "reg" property to define
> the framebuffer memory used by the display hardware. Implement support
> for this in the simplefb driver.
>
> Signed-off-by: Thierry Reding
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede
Regards,
Hans
> ---
> drivers/video/fbdev/simplefb.c | 35 +-
> 1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 62f99f6fccd3..18025f34fde7 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -121,12 +122,13 @@ struct simplefb_params {
> u32 height;
> u32 stride;
> struct simplefb_format *format;
> + struct resource memory;
> };
>
> static int simplefb_parse_dt(struct platform_device *pdev,
> struct simplefb_params *params)
> {
> - struct device_node *np = pdev->dev.of_node;
> + struct device_node *np = pdev->dev.of_node, *mem;
> int ret;
> const char *format;
> int i;
> @@ -166,6 +168,23 @@ static int simplefb_parse_dt(struct platform_device
> *pdev,
> return -EINVAL;
> }
>
> + mem = of_parse_phandle(np, "memory-region", 0);
> + if (mem) {
> + ret = of_address_to_resource(mem, 0, >memory);
> + if (ret < 0) {
> + dev_err(>dev, "failed to parse memory-region\n");
> + of_node_put(mem);
> + return ret;
> + }
> +
> + if (of_property_present(np, "reg"))
> + dev_warn(>dev, "preferring \"memory-region\" over
> \"reg\" property\n");
> +
> + of_node_put(mem);
> + } else {
> + memset(>memory, 0, sizeof(params->memory));
> + }
> +
> return 0;
> }
>
> @@ -193,6 +212,8 @@ static int simplefb_parse_pd(struct platform_device *pdev,
> return -EINVAL;
> }
>
> + memset(>memory, 0, sizeof(params->memory));
> +
> return 0;
> }
>
> @@ -431,10 +452,14 @@ static int simplefb_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(>dev, "No memory resource\n");
> - return -EINVAL;
> + if (params.memory.start == 0 && params.memory.end == 0) {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(>dev, "No memory resource\n");
> + return -EINVAL;
> + }
> + } else {
> + res =
> }
>
> mem = request_mem_region(res->start, resource_size(res), "simplefb");
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hi Sean, On 10/22/23 12:46, Sean Young wrote: > Hi Hans, > > On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote: >> On 10/19/23 12:51, Uwe Kleine-König wrote: >>> On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote: >>>> On 10/17/23 11:17, Sean Young wrote: >>>>> Some drivers require sleeping, for example if the pwm device is connected >>>>> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc >>>>> with the generated IR signal when sleeping occurs. >>>>> >>>>> This patch makes it possible to use pwm when the driver does not sleep, >>>>> by introducing the pwm_can_sleep() function. >>>>> >>>>> Signed-off-by: Sean Young >>>> >>>> I have no objection to this patch by itself, but it seems a bit >>>> of unnecessary churn to change all current callers of pwm_apply_state() >>>> to a new API. >>> >>> The idea is to improve the semantic of the function name, see >>> https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rly...@pengutronix.de >>> for more context. >> >> Hmm, so the argument here is that the GPIO API has this, but GPIOs >> generally speaking can be set atomically, so there not being able >> to set it atomically is special. >> >> OTOH we have many many many other kernel functions which may sleep >> and we don't all postfix them with _can_sleep. >> >> And for PWM controllers pwm_apply_state is IMHO sorta expected to >> sleep. Many of these are attached over I2C so things will sleep, >> others have a handshake to wait for the current dutycycle to >> end before you can apply a second change on top of an earlier >> change during the current dutycycle which often also involves >> sleeping. >> >> So the natural/expeected thing for pwm_apply_state() is to sleep >> and thus it does not need a postfix for this IMHO. > > Most pwm drivers look like they can be made to work in atomic context, > I think. Like you say this is not the case for all of them. Whatever > we choose to be the default for pwm_apply_state(), we should have a > clear function name for the alternative. This is essentially why > pam_apply_cansleep() was picked. > > The alternative to pwm_apply_cansleep() is to have a function name > which implies it can be used from atomic context. However, > pwm_apply_atomic() is not great because the "atomic" could be > confused with the PWM atomic API, not the kernel process/atomic > context. Well pwm_apply_state() is the atomic PWM interface right? So to me pwm_apply_state_atomic() would be clearly about running atomically. > So what should the non-sleeping function be called then? > - pwm_apply_cannotsleep() > - pwm_apply_nosleep() > - pwm_apply_nonsleeping() > - pwm_apply_atomic_context() I would just go with: pwm_apply_state_atomic() but if this is disliked by others then lets just rename pwm_apply_state() to pwm_apply_state_cansleep() as is done in this patch and use plain pwm_apply_state() for the new atomic-context version. Regards, Hans > >>> I think it's very subjective if you consider this >>> churn or not. >> >> I consider it churn because I don't think adding a postfix >> for what is the default/expected behavior is a good idea >> (with GPIOs not sleeping is the expected behavior). >> >> I agree that this is very subjective and very much goes >> into the territory of bikeshedding. So please consider >> the above my 2 cents on this and lets leave it at that. > > You have a valid point. Let's focus on having descriptive function names. > >>> While it's nice to have every caller converted in a single >>> step, I'd go for >>> >>> #define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state) >>> >>> , keep that macro for a while and convert all users step by step. This >>> way we don't needlessly break oot code and the changes to convert to the >>> new API can go via their usual trees without time pressure. >> >> I don't think there are enough users of pwm_apply_state() to warrant >> such an exercise. >> >> So if people want to move ahead with the _can_sleep postfix addition >> (still not a fan) here is my acked-by for the drivers/platform/x86 >> changes, for merging this through the PWM tree in a single commit: >> >> Acked-by: Hans de Goede > > Thanks, > > Sean >
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hi Uwe, On 10/19/23 12:51, Uwe Kleine-König wrote: > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote: >> Hi Sean, >> >> On 10/17/23 11:17, Sean Young wrote: >>> Some drivers require sleeping, for example if the pwm device is connected >>> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc >>> with the generated IR signal when sleeping occurs. >>> >>> This patch makes it possible to use pwm when the driver does not sleep, >>> by introducing the pwm_can_sleep() function. >>> >>> Signed-off-by: Sean Young >> >> I have no objection to this patch by itself, but it seems a bit >> of unnecessary churn to change all current callers of pwm_apply_state() >> to a new API. > > The idea is to improve the semantic of the function name, see > https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rly...@pengutronix.de > for more context. Hmm, so the argument here is that the GPIO API has this, but GPIOs generally speaking can be set atomically, so there not being able to set it atomically is special. OTOH we have many many many other kernel functions which may sleep and we don't all postfix them with _can_sleep. And for PWM controllers pwm_apply_state is IMHO sorta expected to sleep. Many of these are attached over I2C so things will sleep, others have a handshake to wait for the current dutycycle to end before you can apply a second change on top of an earlier change during the current dutycycle which often also involves sleeping. So the natural/expeected thing for pwm_apply_state() is to sleep and thus it does not need a postfix for this IMHO. > I think it's very subjective if you consider this > churn or not. I consider it churn because I don't think adding a postfix for what is the default/expected behavior is a good idea (with GPIOs not sleeping is the expected behavior). I agree that this is very subjective and very much goes into the territory of bikeshedding. So please consider the above my 2 cents on this and lets leave it at that. > While it's nice to have every caller converted in a single > step, I'd go for > > #define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state) > > , keep that macro for a while and convert all users step by step. This > way we don't needlessly break oot code and the changes to convert to the > new API can go via their usual trees without time pressure. I don't think there are enough users of pwm_apply_state() to warrant such an exercise. So if people want to move ahead with the _can_sleep postfix addition (still not a fan) here is my acked-by for the drivers/platform/x86 changes, for merging this through the PWM tree in a single commit: Acked-by: Hans de Goede Regards, Hans
Re: [PATCH v4 13/17] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
Hi,
I was not following this at first, so my apologies for
jumping in in the middle of the thread:
> +static int amd_pmf_gpu_get_cur_state(struct thermal_cooling_device
> *cooling_dev,
> + unsigned long *state)
> +{
> + struct backlight_device *bd;
> +
> + if (!acpi_video_backlight_use_native())
> + return -ENODEV;
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
> +
> + *state = backlight_get_brightness(bd);
> +
> + return 0;
> +}
> +
> +static int amd_pmf_gpu_get_max_state(struct thermal_cooling_device
> *cooling_dev,
> + unsigned long *state)
> +{
> + struct backlight_device *bd;
> +
> + if (!acpi_video_backlight_use_native())
> + return -ENODEV;
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
> +
> + if (backlight_is_blank(bd))
> + *state = 0;
> + else
> + *state = bd->props.max_brightness;
> +
> + return 0;
> +}
> +
> +static const struct thermal_cooling_device_ops bd_cooling_ops = {
> + .get_max_state = amd_pmf_gpu_get_max_state,
> + .get_cur_state = amd_pmf_gpu_get_cur_state,
> +};
So first of all, good to see that this is using the
thermal_cooling_device APIs now, that is great thank you.
But the whole idea behind using the thermal_cooling_device APIs
is that amdgpu exports the cooling_device itself, rather then have
the AMD PMF code export it. Now the AMD PMF code is still poking
at the backlight_device itself, while the idea was to delegate
this to the GPU driver.
Actually seeing all the acpi_video_backlight_use_native()
checks here, I wonder why only have this work with native backlight
control. One step better would be to add thermal_cooling_device
support to the backlight core in:
drivers/video/backlight/backlight.c
Then it will work with any backlight control provider!
Last but not least this code MUST not call
acpi_video_backlight_use_native()
No code other then native GPU drivers must ever call
acpi_video_backlight_use_native(). This special function
not only checks if the native backlight control is the
one which the detection code in drivers/acpi/video_detect.c
has selected, it also signals to video_detect.c that
native GPU backlight control is available.
So by calling this in the AMD PMF code you are now
telling video_detect.c that native GPU backlight control
is available on all systems where AMD PMF runs.
As I already said I really believe the whole cooling
device should be registered somewhere else. But if you
do end up sticking with this then you MUST replace
the acpi_video_backlight_use_native() calls with:
if (acpi_video_get_backlight_type() == acpi_backlight_native) {...}
Regards,
Hans
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hi Sean,
On 10/17/23 11:17, Sean Young wrote:
> Some drivers require sleeping, for example if the pwm device is connected
> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
> with the generated IR signal when sleeping occurs.
>
> This patch makes it possible to use pwm when the driver does not sleep,
> by introducing the pwm_can_sleep() function.
>
> Signed-off-by: Sean Young
I have no objection to this patch by itself, but it seems a bit
of unnecessary churn to change all current callers of pwm_apply_state()
to a new API.
Why not just keep pwm_apply_state() as is and introduce a new
pwm_apply_state_atomic() for callers which want to apply state
in a case where sleeping is not allowed ?
Regards,
Hans
> ---
> Documentation/driver-api/pwm.rst | 16 +++-
> .../gpu/drm/i915/display/intel_backlight.c| 6 +-
> drivers/gpu/drm/solomon/ssd130x.c | 2 +-
> drivers/hwmon/pwm-fan.c | 8 +-
> drivers/input/misc/da7280.c | 4 +-
> drivers/input/misc/pwm-beeper.c | 4 +-
> drivers/input/misc/pwm-vibra.c| 8 +-
> drivers/leds/leds-pwm.c | 2 +-
> drivers/leds/rgb/leds-pwm-multicolor.c| 4 +-
> drivers/media/rc/pwm-ir-tx.c | 4 +-
> drivers/platform/x86/lenovo-yogabook.c| 2 +-
> drivers/pwm/core.c| 75 ++-
> drivers/pwm/pwm-renesas-tpu.c | 1 -
> drivers/pwm/pwm-twl-led.c | 2 +-
> drivers/pwm/pwm-vt8500.c | 2 +-
> drivers/pwm/sysfs.c | 10 +--
> drivers/regulator/pwm-regulator.c | 4 +-
> drivers/video/backlight/lm3630a_bl.c | 2 +-
> drivers/video/backlight/lp855x_bl.c | 2 +-
> drivers/video/backlight/pwm_bl.c | 6 +-
> drivers/video/fbdev/ssd1307fb.c | 2 +-
> include/linux/pwm.h | 57 ++
> 22 files changed, 147 insertions(+), 76 deletions(-)
>
> diff --git a/Documentation/driver-api/pwm.rst
> b/Documentation/driver-api/pwm.rst
> index 3fdc95f7a1d15..a2fb5f8f6e1f8 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -41,7 +41,15 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also
> exist.
>
> After being requested, a PWM has to be configured using::
>
> - int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
> + int pwm_apply_cansleep(struct pwm_device *pwm, struct pwm_state *state);
> +
> +If the PWM support atomic mode, which can be determined with::
> +
> +bool pwm_is_atomic(struct pwm_device *pwm);
> +
> +Then the PWM can be configured with::
> +
> + int pwm_apply(struct pwm_device *pwm, struct pwm_state *state);
>
> This API controls both the PWM period/duty_cycle config and the
> enable/disable state.
> @@ -57,13 +65,13 @@ If supported by the driver, the signal can be optimized,
> for example to improve
> EMI by phase shifting the individual channels of a chip.
>
> The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
> -around pwm_apply_state() and should not be used if the user wants to change
> +around pwm_apply_cansleep() and should not be used if the user wants to
> change
> several parameter at once. For example, if you see pwm_config() and
> pwm_{enable,disable}() calls in the same function, this probably means you
> -should switch to pwm_apply_state().
> +should switch to pwm_apply_cansleep().
>
> The PWM user API also allows one to query the PWM state that was passed to
> the
> -last invocation of pwm_apply_state() using pwm_get_state(). Note this is
> +last invocation of pwm_apply_cansleep() using pwm_get_state(). Note this is
> different to what the driver has actually implemented if the request cannot
> be
> satisfied exactly with the hardware in use. There is currently no way for
> consumers to get the actually implemented settings.
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
> b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 2e8f17c045222..cf516190cde8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct
> drm_connector_state *conn_state,
> struct intel_panel *panel =
> _intel_connector(conn_state->connector)->panel;
>
> pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100);
> - pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
> + pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state);
> }
>
> static void
> @@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct
> drm_connector_state *old_conn
> intel_backlight_set_pwm_level(old_conn_state, level);
>
> panel->backlight.pwm_state.enabled =
Re: [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV
Hi Andy, On 10/18/23 07:10, Andy Shevchenko wrote: > DSI code for VBT has a set of ugly GPIO hacks, one of which is direct > talking to GPIO IP behind the actual driver's back. An attempt to fix > that is here. > > If I understood correctly, my approach should work in the similar way as > the current IOSF GPIO. > > Hans, I believe you have some devices that use this piece of code, > is it possible to give a test run on (one of) them? Yes I should be able to find a device or 2 which poke GPIOs from the VBT MIPI sequences. Unfortunately I don't know from the top of my head which devices actually use this, so I may need to try quite a few devices before finding one which actually uses this. I'll try to get this series tested sometime the coming weeks, depending on when I can schedule some time for this. Regards, Hans > > Andy Shevchenko (2): > drm/i915/dsi: Extract common soc_gpio_exec() helper > drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back > > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 150 +++ > 1 file changed, 58 insertions(+), 92 deletions(-) >
Re: [PATCH 0/4] drm/i915/vlv_dsi: Add quirks for x86 android tablets (v3)
Hi, On 10/11/23 15:05, Jani Nikula wrote: > On Sun, 08 Oct 2023, Hans de Goede wrote: >> Hi All, >> >> Ping what is the status of this now? This v3 addresses all review >> remarks from previous versions (specifically the request to file >> + link gitlab issues). >> >> So AFAICT this is ready for merging ? >> >> But I'm waiting for an ack for this before pushing it >> do drm-intel-next myself ... > > There are maybe one or two things I could nitpick about, such as casting > away the const in there, but then I just don't have the time to look > into this much deeper, it's all fairly isolated, and, let's be honest, > you of all people probably have the best idea how well the vlv dsi code > works out in the real world. > > Thanks for fixing all this stuff, and please just merge. > > Acked-by: Jani Nikula Thank you, I've pushed this to drm-intel-next now. Regards, Hans
Re: [PATCH 0/4] drm/i915/vlv_dsi: Add quirks for x86 android tablets (v3)
Hi All, Ping what is the status of this now? This v3 addresses all review remarks from previous versions (specifically the request to file + link gitlab issues). So AFAICT this is ready for merging ? But I'm waiting for an ack for this before pushing it do drm-intel-next myself ... Regards, Hans On 9/20/23 21:56, Hans de Goede wrote: > Hi All, > > Some vlv/chv tablets ship with Android as factory OS. The factory OS > BSP style kernel on these tablets does not use the normal x86 hw > autodetection instead it hardcodes a whole bunch of things including > using panel drivers instead of relying on VBT MIPI sequences to > turn the panel/backlight on/off. > > The normal i915 driver (which does not use panel drivers) mostly works > since the VBT still needs to contain valid info for the GOP, but because > of the Android kernel relying on panel drivers with various things > hardcoded some DMI quirks are necessary to fix some issues on these > devices. > > Some of these issues also are related to which I2C bus to use for > MIPI sequence elements which do I2C transfers. This series also > includes a patch adding some extra debugging to mipi_exec_i2c() to > help with debugging similar issues in the future. > > These patches have been posted before but back then I did not get around > to follow up on the series: > https://lore.kernel.org/intel-gfx/20220225214934.383168-1-hdego...@redhat.com/ > > v2: > - Drop the changes how the I2C bus number is found, instead just have > the quirks set the right number directly where necessary. This should > avoid any chances of causing regressions on devices where the quirks > do not apply. > - New quirk for backlight control issues on Lenovo Yoga Tab 3 > - Address Jani Nikula's remark about __func__ being redundant when using > drm_dbg_kms() > > v3: > - File 3 gitlab issues with drm.debug=0xe dmesg output, VBT dump for all > 3 affected models. Add Closes: tags with links to gitlab issues > > Regards, > > Hans > > > Hans de Goede (4): > drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on > Asus TF103C (v3) > drm/i915/vlv_dsi: Add DMI quirk for wrong I2C bus and panel size on > Lenovo Yoga Tablet 2 series (v3) > drm/i915/vlv_dsi: Add DMI quirk for backlight control issues on Lenovo > Yoga Tab 3 (v2) > drm/i915/dsi: Add some debug logging to mipi_exec_i2c (v2) > > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 + > drivers/gpu/drm/i915/display/vlv_dsi.c | 124 +++ > 2 files changed, 127 insertions(+) >
Re: [PATCH v2] drm: panel-orientation-quirks: Add quirk for One Mix 2S
Hi,
On 10/1/23 13:47, Kai Uwe Broulik wrote:
> The One Mix 2S is a mini laptop with a 1200x1920 portrait screen
> mounted in a landscape oriented clamshell case. Because of the too
> generic DMI strings this entry is also doing bios-date matching.
>
> Signed-off-by: Kai Uwe Broulik
> ---
> Changes since v1:
> * Got two more BIOS dates reported
Thanks, patch still looks good to me:
Reviewed-by: Hans de Goede
drm-misc maintainers, I'm currently traveling can
one of you push this to drm-misc-fixes please?
Regards,
Hans
>
> drivers/gpu/drm/drm_panel_orientation_quirks.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> index 0cb646cb04ee..d5c15292ae93 100644
> --- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> @@ -38,6 +38,14 @@ static const struct drm_dmi_panel_orientation_data
> gpd_micropc = {
> .orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> };
>
> +static const struct drm_dmi_panel_orientation_data gpd_onemix2s = {
> + .width = 1200,
> + .height = 1920,
> + .bios_dates = (const char * const []){ "05/21/2018", "10/26/2018",
> + "03/04/2019", NULL },
> + .orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> +};
> +
> static const struct drm_dmi_panel_orientation_data gpd_pocket = {
> .width = 1200,
> .height = 1920,
> @@ -401,6 +409,14 @@ static const struct dmi_system_id orientation_data[] = {
> DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "LTH17"),
> },
> .driver_data = (void *)_rightside_up,
> + }, {/* One Mix 2S (generic strings, also match on bios date) */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Default string"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Default string"),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
> + },
> + .driver_data = (void *)_onemix2s,
> },
> {}
> };
Re: [PATCH] drm: panel-orientation-quirks: Add quirk for One Mix 2S
Hi,
On 9/28/23 21:35, Kai Uwe Broulik wrote:
> The One Mix 2S is a mini laptop with a 1200x1920 portrait screen
> mounted in a landscape oriented clamshell case. Because of the too
> generic DMI strings this entry is also doing bios-date matching.
>
> Signed-off-by: Kai Uwe Broulik
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede
drm-misc maintainers, I'm currently traveling can
one of you push this to drm-misc-fixes please?
Regards,
Hans
> ---
> drivers/gpu/drm/drm_panel_orientation_quirks.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> index 0cb646cb04ee..cc9a9099faaf 100644
> --- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> @@ -38,6 +38,14 @@ static const struct drm_dmi_panel_orientation_data
> gpd_micropc = {
> .orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> };
>
> +static const struct drm_dmi_panel_orientation_data gpd_onemix2s = {
> + .width = 1200,
> + .height = 1920,
> + .bios_dates = (const char * const []){ "03/04/2019",
> + NULL },
> + .orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> +};
> +
> static const struct drm_dmi_panel_orientation_data gpd_pocket = {
> .width = 1200,
> .height = 1920,
> @@ -401,6 +409,14 @@ static const struct dmi_system_id orientation_data[] = {
> DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "LTH17"),
> },
> .driver_data = (void *)_rightside_up,
> + }, {/* One Mix 2S (generic strings, also match on bios date) */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Default string"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Default string"),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
> + },
> + .driver_data = (void *)_onemix2s,
> },
> {}
> };
Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
HI,
On 9/26/23 15:17, Christian König wrote:
> Am 26.09.23 um 14:56 schrieb Hans de Goede:
>> Hi,
>>
>> On 9/26/23 13:24, Shyam Sundar S K wrote:
>>> Hi Hans,
>>>
>>> On 9/26/2023 4:05 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 9/22/23 19:50, Shyam Sundar S K wrote:
>>>>> For the Smart PC Solution to fully work, it has to enact to the actions
>>>>> coming from TA. Add the initial code path for set interface to AMDGPU.
>>>>>
>>>>> Co-developed-by: Mario Limonciello
>>>>> Signed-off-by: Mario Limonciello
>>>>> Signed-off-by: Shyam Sundar S K
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +
>>>>> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
>>>>> drivers/platform/x86/amd/pmf/tee-if.c | 19 +--
>>>>> include/linux/amd-pmf-io.h | 1 +
>>>>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> index 232d11833ddc..5c567bff0548 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>> return 0;
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>>>>> +
>>>>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>> +{
>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>>>> + struct backlight_device *bd;
>>>>> +
>>>>> + if (!(adev->flags & AMD_IS_APU)) {
>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>>>>> + return -ENODEV;
>>>>> + }
>>>>> +
>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>> + if (!bd)
>>>>> + return -ENODEV;
>>>> This assumes that the backlight is always controller by the amdgpu's
>>>> native backlight driver, but it might e.g. also be handled by
>>>> eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
>>>> nvidia dgpu).
>>> PMF is meant for AMD APUs(atleast for now) and the _HID will only be
>>> made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
>>> should be safe, right?
>> Users can pass say acpi_backlight=video and use the acpi_video
>> driver for backlight control instead of the native GPU backlight
>> control.
>>
>>>> For now what should be done here is to call acpi_video_get_backlight_type()
>>>> and then translate the return value from this into a backlight-type:
>>>>
>>>> acpi_backlight_video -> BACKLIGHT_FIRMWARE
>>>> acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
>>>> acpi_backlight_native, -> BACKLIGHT_RAW
>>>> acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
>>>> acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>>>>
>>> I can add this change in the v2, do you insist on this?
>> Insist is a strong word, but I think that it is a good idea to have
>> this. Evenutally it looks like this code will need to either integrate with
>> the drm drivers lot more; or the drm core needs to export some special
>> hooks for this which the PMF code can then call.
>>
>> Actually thinking more about this, I think that the right thing to do
>> here is make some code register brightness control as a cooling device
>> (which I think is already done in some cases) and then have the PMF
>> code use the cooling-device APIs for this.
>>
>> IMHO that would be a much cleaner solution then this hack.
>
> Yeah, fully agree with Hans. This looks like a rather extreme hack to me.
Shyam, the cooling device interface is defined in:
include/linux/thermal.h
And then look for cooling_device .
An example of code registering a cooling_device for backlight control is:
drivers/acpi/acpi_video.c
and then specifically the code starting around line 257 with:
video_get_max_state()
until
static const struct thermal_cooling_device_ops video_cooling_ops = {
...
And the code
Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
Hi,
On 9/26/23 13:24, Shyam Sundar S K wrote:
> Hi Hans,
>
> On 9/26/2023 4:05 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 9/22/23 19:50, Shyam Sundar S K wrote:
>>> For the Smart PC Solution to fully work, it has to enact to the actions
>>> coming from TA. Add the initial code path for set interface to AMDGPU.
>>>
>>> Co-developed-by: Mario Limonciello
>>> Signed-off-by: Mario Limonciello
>>> Signed-off-by: Shyam Sundar S K
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +
>>> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
>>> drivers/platform/x86/amd/pmf/tee-if.c | 19 +--
>>> include/linux/amd-pmf-io.h | 1 +
>>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>> index 232d11833ddc..5c567bff0548 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>>> return 0;
>>> }
>>> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>>> +
>>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
>>> +{
>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>> + struct backlight_device *bd;
>>> +
>>> + if (!(adev->flags & AMD_IS_APU)) {
>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>> + if (!bd)
>>> + return -ENODEV;
>>
>> This assumes that the backlight is always controller by the amdgpu's
>> native backlight driver, but it might e.g. also be handled by
>> eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
>> nvidia dgpu).
>
> PMF is meant for AMD APUs(atleast for now) and the _HID will only be
> made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
> should be safe, right?
Users can pass say acpi_backlight=video and use the acpi_video
driver for backlight control instead of the native GPU backlight
control.
>
>>
>> For now what should be done here is to call acpi_video_get_backlight_type()
>> and then translate the return value from this into a backlight-type:
>>
>> acpi_backlight_video -> BACKLIGHT_FIRMWARE
>> acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
>> acpi_backlight_native, -> BACKLIGHT_RAW
>> acpi_backlight_nvidia_wmi_ec,-> BACKLIGHT_FIRMWARE
>> acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>>
>
> I can add this change in the v2, do you insist on this?
Insist is a strong word, but I think that it is a good idea to have
this. Evenutally it looks like this code will need to either integrate with
the drm drivers lot more; or the drm core needs to export some special
hooks for this which the PMF code can then call.
Actually thinking more about this, I think that the right thing to do
here is make some code register brightness control as a cooling device
(which I think is already done in some cases) and then have the PMF
code use the cooling-device APIs for this.
IMHO that would be a much cleaner solution then this hack.
Regards,
Hans
>
> Thanks,
> Shyam
>
>> Also I'm worried about probe order here, this code currently assumes
>> that the GPU or other backlight driver has loaded before this runs,
>> which is not necessarily the case.
>>
>> I think that if the backlight_device_get_by_type() fails this
>> should be retried say every 10 seconds from some delayed workqueue
>> for at least a couple of minutes after boot.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> +
>>> + backlight_device_set_brightness(bd, pmf->brightness);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>> index 9032df4ba48a..ce89cc0daa5a 100644
>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>> @@ -73,6 +73,7 @@
>>> #define PMF_POLICY_STT_SKINTEMP_APU7
>>> #define PMF_POLICY_S
Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
Hi,
On 9/22/23 19:50, Shyam Sundar S K wrote:
> For the Smart PC Solution to fully work, it has to enact to the actions
> coming from TA. Add the initial code path for set interface to AMDGPU.
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +
> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
> drivers/platform/x86/amd/pmf/tee-if.c | 19 +--
> include/linux/amd-pmf-io.h | 1 +
> 4 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> index 232d11833ddc..5c567bff0548 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
> return 0;
> }
> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
> +
> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
> +{
> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
> + struct backlight_device *bd;
> +
> + if (!(adev->flags & AMD_IS_APU)) {
> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
> + return -ENODEV;
> + }
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
This assumes that the backlight is always controller by the amdgpu's
native backlight driver, but it might e.g. also be handled by
eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
nvidia dgpu).
For now what should be done here is to call acpi_video_get_backlight_type()
and then translate the return value from this into a backlight-type:
acpi_backlight_video-> BACKLIGHT_FIRMWARE
acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
acpi_backlight_native, -> BACKLIGHT_RAW
acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
Also I'm worried about probe order here, this code currently assumes
that the GPU or other backlight driver has loaded before this runs,
which is not necessarily the case.
I think that if the backlight_device_get_by_type() fails this
should be retried say every 10 seconds from some delayed workqueue
for at least a couple of minutes after boot.
Regards,
Hans
> +
> + backlight_device_set_brightness(bd, pmf->brightness);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 9032df4ba48a..ce89cc0daa5a 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -73,6 +73,7 @@
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> #define PMF_POLICY_SYSTEM_STATE 9
> +#define PMF_POLICY_DISPLAY_BRIGHTNESS12
> #define PMF_POLICY_P3T 38
>
> /* TA macros */
> @@ -480,6 +481,7 @@ enum ta_pmf_error_type {
> };
>
> struct pmf_action_table {
> + unsigned long display_brightness;
> enum system_state system_state;
> unsigned long spl; /* in mW */
> unsigned long sppt; /* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1608996654e8..eef83a4c 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev
> *dev, u16 event)
> return 0;
> }
>
> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_result *out)
> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_result *out)
> {
> u32 val, event = 0;
> - int idx;
> + int idx, ret;
>
> for (idx = 0; idx < out->actions_count; idx++) {
> val = out->actions_list[idx].value;
> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev
> *dev, struct ta_pmf_enact_
> dev->prev_data->system_state = 0;
> }
> break;
> +
> + case PMF_POLICY_DISPLAY_BRIGHTNESS:
> + ret = amd_pmf_get_gfx_data(>gfx_data);
> + if (ret)
> + return ret;
> +
> + dev->prev_data->display_brightness =
> dev->gfx_data.brightness;
> + if (dev->prev_data->display_brightness != val) {
> + dev->gfx_data.brightness = val;
> + amd_pmf_set_gfx_data(>gfx_data);
> +
[PATCH 4/4] drm/i915/dsi: Add some debug logging to mipi_exec_i2c (v2)
Add some debug logging to mipi_exec_i2c, to make debugging various
issues seen with it easier.
Changes in v2:
- Drop unnecessary __func__ drm_dbg_kms() argument
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index e56ec3f2d84a..24b2cbcfc1ef 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -565,6 +565,9 @@ static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi,
const u8 *data)
u8 payload_size = *(data + 6);
u8 *payload_data;
+ drm_dbg_kms(>drm, "bus %d client-addr 0x%02x reg 0x%02x data
%*ph\n",
+ vbt_i2c_bus_num, slave_addr, reg_offset, payload_size, data
+ 7);
+
if (intel_dsi->i2c_bus_num < 0) {
intel_dsi->i2c_bus_num = vbt_i2c_bus_num;
i2c_acpi_find_adapter(intel_dsi, slave_addr);
--
2.41.0
[PATCH 2/4] drm/i915/vlv_dsi: Add DMI quirk for wrong I2C bus and panel size on Lenovo Yoga Tablet 2 series (v3)
On the Lenovo Yoga Tablet 2 830 / 1050 there are 2 problems:
1. The I2C MIPI sequence elements reference bus 3. ACPI has I2C1 - I2C7
which under Linux become bus 0 - 6. And the MIPI sequence reference
to bus 3 is indented for I2C3 which is bus 2 under Linux.
This leads to errors like these:
[ 178.244049] i2c_designware 80860F41:03: controller timed out
[ 178.245703] i915 :00:02.0: [drm] *ERROR* Failed to xfer payload of
size (1) to reg (169)
There are 3 timeouts when the panel is on, delaying
waking up the screen on a key press by 3 seconds.
Note mipi_exec_i2c() cannot just subtract 1 from the bus
given in the I2C MIPI sequence element. Since on other
devices the I2C bus-numbers used in the MIPI sequences do
actually start at 0.
2. width_/height_mm contain a bogus 192mm x 120mm size. This is
especially a problem on the 8" 830 version which uses a 10:16
portrait screen where as the bogus size is 16:10.
Add a DMI quirk to override the I2C bus and the panel size with
the correct values.
Note both the 10" 1050 models as well as the 8" 830 models use the same
mainboard and thus the same DMI strings. The 10" 1050 uses a 1920x1200
landscape screen, where as the 8" 830 uses a 1200x1920 portrait screen,
so the quirk handling uses the display resolution to detect the model.
v2:
- Also override i2c_bus_num to fix mipi_exec_i2c() timeouts
v3:
- Add Closes tag to gitlab issue with drm.debug=0xe, VBT info
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9379
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/vlv_dsi.c | 52 ++
1 file changed, 52 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index 0d3aabf6a1dd..f69cafe8a17d 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1767,6 +1767,44 @@ static void vlv_dsi_asus_tf103c_mode_fixup(struct
intel_dsi *intel_dsi)
fixed_mode->vtotal -= 4;
}
+/*
+ * On the Lenovo Yoga Tablet 2 830 / 1050 there are 2 problems:
+ * 1. The I2C MIPI sequence elements reference bus 3. ACPI has I2C1 - I2C7
+ *which under Linux become bus 0 - 6. And the MIPI sequence reference
+ *to bus 3 is indented for I2C3 which is bus 2 under Linux.
+ *
+ *Note mipi_exec_i2c() cannot just subtract 1 from the bus
+ *given in the I2C MIPI sequence element. Since on other
+ *devices the I2C bus-numbers used in the MIPI sequences do
+ *actually start at 0.
+ *
+ * 2. width_/height_mm contain a bogus 192mm x 120mm size. This is
+ *especially a problem on the 8" 830 version which uses a 10:16
+ *portrait screen where as the bogus size is 16:10.
+ *
+ * https://gitlab.freedesktop.org/drm/intel/-/issues/9379
+ */
+static void vlv_dsi_lenovo_yoga_tab2_size_fixup(struct intel_dsi *intel_dsi)
+{
+ const struct drm_display_mode *fixed_mode =
+ intel_panel_preferred_fixed_mode(intel_dsi->attached_connector);
+ struct drm_display_info *info =
_dsi->attached_connector->base.display_info;
+
+ intel_dsi->i2c_bus_num = 2;
+
+ /*
+* The 10" 1050 uses a 1920x1200 landscape screen, where as the 8" 830
+* uses a 1200x1920 portrait screen.
+*/
+ if (fixed_mode->hdisplay == 1920) {
+ info->width_mm = 216;
+ info->height_mm = 135;
+ } else {
+ info->width_mm = 107;
+ info->height_mm = 171;
+ }
+}
+
static const struct dmi_system_id vlv_dsi_dmi_quirk_table[] = {
{
/* Asus Transformer Pad TF103C */
@@ -1776,6 +1814,20 @@ static const struct dmi_system_id
vlv_dsi_dmi_quirk_table[] = {
},
.driver_data = (void *)vlv_dsi_asus_tf103c_mode_fixup,
},
+ {
+ /*
+* Lenovo Yoga Tablet 2 830F/L or 1050F/L (The 8" and 10"
+* Lenovo Yoga Tablet 2 use the same mainboard)
+*/
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Intel Corp."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "VALLEYVIEW C0 PLATFORM"),
+ DMI_MATCH(DMI_BOARD_NAME, "BYT-T FFD8"),
+ /* Partial match on beginning of BIOS version */
+ DMI_MATCH(DMI_BIOS_VERSION, "BLADE_21"),
+ },
+ .driver_data = (void *)vlv_dsi_lenovo_yoga_tab2_size_fixup,
+ },
{ }
};
--
2.41.0
[PATCH 3/4] drm/i915/vlv_dsi: Add DMI quirk for backlight control issues on Lenovo Yoga Tab 3 (v2)
On the Lenovo Yoga Tab 3 Pro YT3-X90F there are 2 issues with the backlight
on/off MIPI sequences:
1. The backlight on sequence has an I2C MIPI sequence element which uses
bus 0, but there is a bogus I2cSerialBus resource under the GPU in
the DSDT which causes i2c_acpi_find_adapter() to pick the wrong bus.
2. There is no backlight off sequence, causing the backlight to stay on.
Add a DMI quirk fixing both issues.
v2:
- Add Closes tag to gitlab issue with drm.debug=0xe, VBT info
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9380
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/vlv_dsi.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index f69cafe8a17d..55da627a8b8d 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1805,6 +1805,31 @@ static void vlv_dsi_lenovo_yoga_tab2_size_fixup(struct
intel_dsi *intel_dsi)
}
}
+/*
+ * On the Lenovo Yoga Tab 3 Pro YT3-X90F there are 2 problems:
+ * 1. i2c_acpi_find_adapter() picks the wrong adapter causing mipi_exec_i2c()
+ *to not work. Fix this by setting i2c_bus_num.
+ * 2. There is no backlight off MIPI sequence, causing the backlight to stay
on.
+ *Add a backlight off sequence mirroring the existing backlight on
sequence.
+ *
+ * https://gitlab.freedesktop.org/drm/intel/-/issues/9380
+ */
+static void vlv_dsi_lenovo_yoga_tab3_backlight_fixup(struct intel_dsi
*intel_dsi)
+{
+ static const u8 backlight_off_sequence[16] = {
+ /* Header Seq-id 7, length after header 11 bytes */
+ 0x07, 0x0b, 0x00, 0x00, 0x00,
+ /* MIPI_SEQ_ELEM_I2C bus 0 addr 0x2c reg 0x00 data-len 1 data
0x00 */
+ 0x04, 0x08, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x01, 0x00,
+ /* MIPI_SEQ_ELEM_END */
+ 0x00
+ };
+ struct intel_connector *connector = intel_dsi->attached_connector;
+
+ intel_dsi->i2c_bus_num = 0;
+ connector->panel.vbt.dsi.sequence[MIPI_SEQ_BACKLIGHT_OFF] =
backlight_off_sequence;
+}
+
static const struct dmi_system_id vlv_dsi_dmi_quirk_table[] = {
{
/* Asus Transformer Pad TF103C */
@@ -1828,6 +1853,15 @@ static const struct dmi_system_id
vlv_dsi_dmi_quirk_table[] = {
},
.driver_data = (void *)vlv_dsi_lenovo_yoga_tab2_size_fixup,
},
+ {
+ /* Lenovo Yoga Tab 3 Pro YT3-X90F */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "CHERRYVIEW D1 PLATFORM"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "Blade3-10A-001"),
+ },
+ .driver_data = (void *)vlv_dsi_lenovo_yoga_tab3_backlight_fixup,
+ },
{ }
};
--
2.41.0
[PATCH 1/4] drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on Asus TF103C (v3)
Vtotal is wrong in the BIOS supplied modeline for the DSI panel on
the Asus TF103C leading to the last line of the display being shown
as the first line.
Original: "1280x800": 60 67700 1280 1312 1328 1376 800 808 812 820 0x8 0xa
Fixed:"1280x800": 60 67700 1280 1312 1328 1376 800 808 812 816 0x8 0xa
The factory installed Android has a hardcoded modeline in its kernel,
causing it to not suffer from this BIOS bug;
and the Android boot-splash which uses the EFI FB which does have this bug
has the last line all black causing the bug to not be visible.
This commit introduces a generic DMI based quirk mechanism to vlv_dsi for
doing various fixups, and uses this to correct the modeline.
v2:
- s/mode_fixup/dmi_quirk/ to make the new DMI quirk mechanism more generic
- Add a comment with the old and new modelines to the patch and commit msg
v3:
- Add Closes tag to gitlab issue with drm.debug=0xe, VBT info
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9381
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/vlv_dsi.c | 44 ++
1 file changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index a96e7d028c5c..0d3aabf6a1dd 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -23,6 +23,7 @@
* Author: Jani Nikula
*/
+#include
#include
#include
@@ -1744,6 +1745,40 @@ static void vlv_dphy_param_init(struct intel_dsi
*intel_dsi)
intel_dsi_log_params(intel_dsi);
}
+typedef void (*vlv_dsi_dmi_quirk_func)(struct intel_dsi *intel_dsi);
+
+/*
+ * Vtotal is wrong on the Asus TF103C leading to the last line of the display
+ * being shown as the first line. The factory installed Android has a hardcoded
+ * modeline, causing it to not suffer from this BIOS bug.
+ *
+ * Original mode: "1280x800": 60 67700 1280 1312 1328 1376 800 808 812 820 0x8
0xa
+ * Fixedmode: "1280x800": 60 67700 1280 1312 1328 1376 800 808 812 816 0x8
0xa
+ *
+ * https://gitlab.freedesktop.org/drm/intel/-/issues/9381
+ */
+static void vlv_dsi_asus_tf103c_mode_fixup(struct intel_dsi *intel_dsi)
+{
+ /* Cast away the const as we want to fixup the mode */
+ struct drm_display_mode *fixed_mode = (struct drm_display_mode *)
+ intel_panel_preferred_fixed_mode(intel_dsi->attached_connector);
+
+ if (fixed_mode->vtotal == 820)
+ fixed_mode->vtotal -= 4;
+}
+
+static const struct dmi_system_id vlv_dsi_dmi_quirk_table[] = {
+ {
+ /* Asus Transformer Pad TF103C */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "TF103C"),
+ },
+ .driver_data = (void *)vlv_dsi_asus_tf103c_mode_fixup,
+ },
+ { }
+};
+
void vlv_dsi_init(struct drm_i915_private *dev_priv)
{
struct intel_dsi *intel_dsi;
@@ -1752,6 +1787,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
struct intel_connector *intel_connector;
struct drm_connector *connector;
struct drm_display_mode *current_mode;
+ const struct dmi_system_id *dmi_id;
enum port port;
enum pipe pipe;
@@ -1883,6 +1919,14 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
goto err_cleanup_connector;
}
+ dmi_id = dmi_first_match(vlv_dsi_dmi_quirk_table);
+ if (dmi_id) {
+ vlv_dsi_dmi_quirk_func quirk_func =
+ (vlv_dsi_dmi_quirk_func)dmi_id->driver_data;
+
+ quirk_func(intel_dsi);
+ }
+
intel_panel_init(intel_connector, NULL);
intel_backlight_setup(intel_connector, INVALID_PIPE);
--
2.41.0
[PATCH 0/4] drm/i915/vlv_dsi: Add quirks for x86 android tablets (v3)
Hi All, Some vlv/chv tablets ship with Android as factory OS. The factory OS BSP style kernel on these tablets does not use the normal x86 hw autodetection instead it hardcodes a whole bunch of things including using panel drivers instead of relying on VBT MIPI sequences to turn the panel/backlight on/off. The normal i915 driver (which does not use panel drivers) mostly works since the VBT still needs to contain valid info for the GOP, but because of the Android kernel relying on panel drivers with various things hardcoded some DMI quirks are necessary to fix some issues on these devices. Some of these issues also are related to which I2C bus to use for MIPI sequence elements which do I2C transfers. This series also includes a patch adding some extra debugging to mipi_exec_i2c() to help with debugging similar issues in the future. These patches have been posted before but back then I did not get around to follow up on the series: https://lore.kernel.org/intel-gfx/20220225214934.383168-1-hdego...@redhat.com/ v2: - Drop the changes how the I2C bus number is found, instead just have the quirks set the right number directly where necessary. This should avoid any chances of causing regressions on devices where the quirks do not apply. - New quirk for backlight control issues on Lenovo Yoga Tab 3 - Address Jani Nikula's remark about __func__ being redundant when using drm_dbg_kms() v3: - File 3 gitlab issues with drm.debug=0xe dmesg output, VBT dump for all 3 affected models. Add Closes: tags with links to gitlab issues Regards, Hans Hans de Goede (4): drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on Asus TF103C (v3) drm/i915/vlv_dsi: Add DMI quirk for wrong I2C bus and panel size on Lenovo Yoga Tablet 2 series (v3) drm/i915/vlv_dsi: Add DMI quirk for backlight control issues on Lenovo Yoga Tab 3 (v2) drm/i915/dsi: Add some debug logging to mipi_exec_i2c (v2) drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 + drivers/gpu/drm/i915/display/vlv_dsi.c | 124 +++ 2 files changed, 127 insertions(+) -- 2.41.0
Re: [PATCH 0/4] drm/i915/vlv_dsi: Add quirks for x86 android tablets (v2)
Hi Ville, On 9/18/23 10:00, Ville Syrjälä wrote: > On Sat, Sep 16, 2023 at 02:54:51PM +0200, Hans de Goede wrote: >> Hi All, >> >> Some vlv/chv tablets ship with Android as factory OS. The factory OS >> BSP style kernel on these tablets does not use the normal x86 hw >> autodetection instead it hardcodes a whole bunch of things including >> using panel drivers instead of relying on VBT MIPI sequences to >> turn the panel/backlight on/off. >> >> The normal i915 driver (which does not use panel drivers) mostly works >> since the VBT still needs to contain valid info for the GOP, but because >> of the Android kernel relying on panel drivers with various things >> hardcoded some DMI quirks are necessary to fix some issues on these >> devices. >> >> Some of these issues also are related to which I2C bus to use for >> MIPI sequence elements which do I2C transfers. This series also >> includes a patch adding some extra debugging to mipi_exec_i2c() to >> help with debugging similar issues in the future. >> >> These patches have been posted before but back then I did not get around >> to follow up on the series: >> https://lore.kernel.org/intel-gfx/20220225214934.383168-1-hdego...@redhat.com/ >> >> Changes compared to this old version: >> - Drop the changes how the I2C bus number is found, instead just have >> the quirks set the right number directly where necessary. This should >> avoid any chances of causing regressions on devices where the quirks >> do not apply. >> >> - New quirk for backlight control issues on Lenovo Yoga Tab 3 >> >> - Address Jani Nikula's remark about __func__ being redundant when using >> drm_dbg_kms() >> >> >> Regards, >> >> Hans >> >> >> >> Hans de Goede (4): >> drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on >> Asus TF103C (v2) >> drm/i915/vlv_dsi: Add DMI quirk for wrong I2C bus and panel size on >> Lenovo Yoga Tablet 2 series (v2) >> drm/i915/vlv_dsi: Add DMI quirk for backlight control issues on Lenovo >> Yoga Tab 3 > > Please file a bug for each of these and attach the usual drm.debug=0xe > dmesg + VBT + any other relevant information there. Otherwise a few > years from now I'll be cursing at these commits as well for not leaving > a decent papertrail... Ok. I've just completed filing 3 issues (1 per affected model) and I've added Closes: tags to the patches pointing to the issues. I'll send out a v3 of this series with the Closes tags. Regards, Hans > >> drm/i915/dsi: Add some debug logging to mipi_exec_i2c (v2) >> >> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 + >> drivers/gpu/drm/i915/display/vlv_dsi.c | 124 +++ >> 2 files changed, 127 insertions(+) >> >> -- >> 2.41.0 >
Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
Hi, On 9/19/23 19:46, Julius Zint wrote: > > > On Wed, 6 Sep 2023, Hans de Goede wrote: > >> Hi Julius, >> >> On 9/4/23 21:02, Julius Zint wrote: >>> >>> >>> On Mon, 4 Sep 2023, Thomas Weißschuh wrote: >>> >>>> +Cc Hans who ins involved with the backlight subsystem >>>> >>>> Hi Julius, >>>> >>>> today I stumbled upon a mail from Hans [0], which explains that the >>>> backlight subsystem is not actually a good fit (yet?) for external >>>> displays. >>>> >>>> It seems a new API is in the works that would better fit, but I'm not >>>> sure about the state of this API. Maybe Hans can clarify. >>>> >>>> This also ties back to my review question how userspace can figure out >>>> to which display a backlight devices applies. So far it can not. >>>> >>>> [0] >>>> https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfd...@redhat.com/ >>>> >>> >>> Hi Thomas, >>> >>> thanks for the hint. I will make sure to give this a proper read and >>> see, if it fits my use case better then the current backlight subsystem. >> >> Note the actual proposal for the new usespace API for display brightness >> control is here: >> >> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/ >> >> I have finished / stabilized the backlight code refactor which I landed >> in 6.1, which is a prerequisite for the above proposal. But I have not >> been able to make time to actually implement the above proposal; and >> I don't know when I will be able to make time for this. >> >>> Especially since I wasnt able to properly address your other review >>> comments for now. You are right that the name should align better with >>> the kernel module and also, that it is possible for multiple displays to >>> be attached. >>> >>> In its current state, this would mean that you could only control the >>> backlight for the first HID device (enough for me :-). >>> >>> The systemd-backlight@.service uses not only the file name, but also the >>> full bus path for storing/restoring backlights. I did not yet get around >>> to see how the desktops handle brightness control, but since the >>> systemd-backlight@.service already uses the name, its important to stay >>> the same over multiple boots. >>> >>> I would be able to get a handle on the underlying USB device and use the >>> serial to uniquely (and persistently) name the backlight. But it does >>> feel hacky doing it this way. >> >> So mutter (gnome-shell compositor library) has a similar issue when saving >> monitor layouts and I can tell you beforehand that monitor serial numbers >> by themselves are not unique enough. Some models just report 123456789 >> as serial and if you have a dual-monitor setup with 2 such monitors >> and name the backlight class device -vcp-hid or something like that >> you will still end up with 2 identical names. >> >> To avoid this when saving monitor layouts mutter saves both the port >> to which the monitor is attached (e.g. DP-1 DP-2) and the serialnumber >> and on startup / monitor hotplug when it checks to see if it has saved >> layout info for the monitor it checks the port+serialnr combination. >> >> So what I think you should do is figure out a way to map which >> VCP HID device maps to which drm-connector and then use >> the connector-name + serial-nr to generate the backlight device name. >> >> We will need the mapping the a drm-connector object anyway for >> the new brightness API proposal from above. >> >> Note this does NOT solve the fact that registering a new backlight >> class device for an external monitor on a laptop will hopelessly >> confuse userspace, see: >> >> https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfd...@redhat.com/ >> >> Regards, >> >> Hans >> > > Thank you for all this additional information. I have watched the talks > and read up upon the mail threads you`ve linked. Now I wonder which presentation you have watched, did you watch the old XDC2014 presentation ? Note I gave a much more up2date presentation on this at kernel-recipes last year: https://kernel-recipes.org/en/2022/talks/new-userspace-api-for-display-panel-brightness-control/ > I will see if I can make the mapping to the DRM connector and plan to > update this patchset. Sounds good. Regards, Hans
[PATCH 4/4] drm/i915/dsi: Add some debug logging to mipi_exec_i2c (v2)
Add some debug logging to mipi_exec_i2c, to make debugging various
issues seen with it easier.
Changes in v2:
- Drop unnecessary __func__ drm_dbg_kms() argument
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index e56ec3f2d84a..24b2cbcfc1ef 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -565,6 +565,9 @@ static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi,
const u8 *data)
u8 payload_size = *(data + 6);
u8 *payload_data;
+ drm_dbg_kms(>drm, "bus %d client-addr 0x%02x reg 0x%02x data
%*ph\n",
+ vbt_i2c_bus_num, slave_addr, reg_offset, payload_size, data
+ 7);
+
if (intel_dsi->i2c_bus_num < 0) {
intel_dsi->i2c_bus_num = vbt_i2c_bus_num;
i2c_acpi_find_adapter(intel_dsi, slave_addr);
--
2.41.0
[PATCH 2/4] drm/i915/vlv_dsi: Add DMI quirk for wrong I2C bus and panel size on Lenovo Yoga Tablet 2 series (v2)
On the Lenovo Yoga Tablet 2 830 / 1050 there are 2 problems:
1. The I2C MIPI sequence elements reference bus 3. ACPI has I2C1 - I2C7
which under Linux become bus 0 - 6. And the MIPI sequence reference
to bus 3 is indented for I2C3 which is bus 2 under Linux.
This leads to errors like these:
[ 178.244049] i2c_designware 80860F41:03: controller timed out
[ 178.245703] i915 :00:02.0: [drm] *ERROR* Failed to xfer payload of
size (1) to reg (169)
There are 3 timeouts when the panel is on, delaying
waking up the screen on a key press by 3 seconds.
Note mipi_exec_i2c() cannot just subtract 1 from the bus
given in the I2C MIPI sequence element. Since on other
devices the I2C bus-numbers used in the MIPI sequences do
actually start at 0.
2. width_/height_mm contain a bogus 192mm x 120mm size. This is
especially a problem on the 8" 830 version which uses a 10:16
portrait screen where as the bogus size is 16:10.
Add a DMI quirk to override the I2C bus and the panel size with
the correct values.
Note both the 10" 1050 models as well as the 8" 830 models use the same
mainboard and thus the same DMI strings. The 10" 1050 uses a 1920x1200
landscape screen, where as the 8" 830 uses a 1200x1920 portrait screen,
so the quirk handling uses the display resolution to detect the model.
Changes in v2:
- Also override i2c_bus_num to fix mipi_exec_i2c() timeouts
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/vlv_dsi.c | 50 ++
1 file changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index 51c4b1491fa2..e247e3413d90 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1765,6 +1765,42 @@ static void vlv_dsi_asus_tf103c_mode_fixup(struct
intel_dsi *intel_dsi)
fixed_mode->vtotal -= 4;
}
+/*
+ * On the Lenovo Yoga Tablet 2 830 / 1050 there are 2 problems:
+ * 1. The I2C MIPI sequence elements reference bus 3. ACPI has I2C1 - I2C7
+ *which under Linux become bus 0 - 6. And the MIPI sequence reference
+ *to bus 3 is indented for I2C3 which is bus 2 under Linux.
+ *
+ *Note mipi_exec_i2c() cannot just subtract 1 from the bus
+ *given in the I2C MIPI sequence element. Since on other
+ *devices the I2C bus-numbers used in the MIPI sequences do
+ *actually start at 0.
+ *
+ * 2. width_/height_mm contain a bogus 192mm x 120mm size. This is
+ *especially a problem on the 8" 830 version which uses a 10:16
+ *portrait screen where as the bogus size is 16:10.
+ */
+static void vlv_dsi_lenovo_yoga_tab2_size_fixup(struct intel_dsi *intel_dsi)
+{
+ const struct drm_display_mode *fixed_mode =
+ intel_panel_preferred_fixed_mode(intel_dsi->attached_connector);
+ struct drm_display_info *info =
_dsi->attached_connector->base.display_info;
+
+ intel_dsi->i2c_bus_num = 2;
+
+ /*
+* The 10" 1050 uses a 1920x1200 landscape screen, where as the 8" 830
+* uses a 1200x1920 portrait screen.
+*/
+ if (fixed_mode->hdisplay == 1920) {
+ info->width_mm = 216;
+ info->height_mm = 135;
+ } else {
+ info->width_mm = 107;
+ info->height_mm = 171;
+ }
+}
+
static const struct dmi_system_id vlv_dsi_dmi_quirk_table[] = {
{
/* Asus Transformer Pad TF103C */
@@ -1774,6 +1810,20 @@ static const struct dmi_system_id
vlv_dsi_dmi_quirk_table[] = {
},
.driver_data = (void *)vlv_dsi_asus_tf103c_mode_fixup,
},
+ {
+ /*
+* Lenovo Yoga Tablet 2 830F/L or 1050F/L (The 8" and 10"
+* Lenovo Yoga Tablet 2 use the same mainboard)
+*/
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Intel Corp."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "VALLEYVIEW C0 PLATFORM"),
+ DMI_MATCH(DMI_BOARD_NAME, "BYT-T FFD8"),
+ /* Partial match on beginning of BIOS version */
+ DMI_MATCH(DMI_BIOS_VERSION, "BLADE_21"),
+ },
+ .driver_data = (void *)vlv_dsi_lenovo_yoga_tab2_size_fixup,
+ },
{ }
};
--
2.41.0
[PATCH 3/4] drm/i915/vlv_dsi: Add DMI quirk for backlight control issues on Lenovo Yoga Tab 3
On the Lenovo Yoga Tab 3 Pro YT3-X90F there are 2 issues with the backlight
on/off MIPI sequences:
1. The backlight on sequence has an I2C MIPI sequence element which uses
bus 0, but there is a bogus I2cSerialBus resource under the GPU in
the DSDT which causes i2c_acpi_find_adapter() to pick the wrong bus.
2. There is no backlight off sequence, causing the backlight to stay on.
Add a DMI quirk fixing both issues.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/vlv_dsi.c | 32 ++
1 file changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index e247e3413d90..db519cfc243b 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1801,6 +1801,29 @@ static void vlv_dsi_lenovo_yoga_tab2_size_fixup(struct
intel_dsi *intel_dsi)
}
}
+/*
+ * On the Lenovo Yoga Tab 3 Pro YT3-X90F there are 2 problems:
+ * 1. i2c_acpi_find_adapter() picks the wrong adapter causing mipi_exec_i2c()
+ *to not work. Fix this by setting i2c_bus_num.
+ * 2. There is no backlight off MIPI sequence, causing the backlight to stay
on.
+ *Add a backlight off sequence mirroring the existing backlight on
sequence.
+ */
+static void vlv_dsi_lenovo_yoga_tab3_backlight_fixup(struct intel_dsi
*intel_dsi)
+{
+ static const u8 backlight_off_sequence[16] = {
+ /* Header Seq-id 7, length after header 11 bytes */
+ 0x07, 0x0b, 0x00, 0x00, 0x00,
+ /* MIPI_SEQ_ELEM_I2C bus 0 addr 0x2c reg 0x00 data-len 1 data
0x00 */
+ 0x04, 0x08, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x01, 0x00,
+ /* MIPI_SEQ_ELEM_END */
+ 0x00
+ };
+ struct intel_connector *connector = intel_dsi->attached_connector;
+
+ intel_dsi->i2c_bus_num = 0;
+ connector->panel.vbt.dsi.sequence[MIPI_SEQ_BACKLIGHT_OFF] =
backlight_off_sequence;
+}
+
static const struct dmi_system_id vlv_dsi_dmi_quirk_table[] = {
{
/* Asus Transformer Pad TF103C */
@@ -1824,6 +1847,15 @@ static const struct dmi_system_id
vlv_dsi_dmi_quirk_table[] = {
},
.driver_data = (void *)vlv_dsi_lenovo_yoga_tab2_size_fixup,
},
+ {
+ /* Lenovo Yoga Tab 3 Pro YT3-X90F */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "CHERRYVIEW D1 PLATFORM"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "Blade3-10A-001"),
+ },
+ .driver_data = (void *)vlv_dsi_lenovo_yoga_tab3_backlight_fixup,
+ },
{ }
};
--
2.41.0
[PATCH 0/4] drm/i915/vlv_dsi: Add quirks for x86 android tablets (v2)
Hi All, Some vlv/chv tablets ship with Android as factory OS. The factory OS BSP style kernel on these tablets does not use the normal x86 hw autodetection instead it hardcodes a whole bunch of things including using panel drivers instead of relying on VBT MIPI sequences to turn the panel/backlight on/off. The normal i915 driver (which does not use panel drivers) mostly works since the VBT still needs to contain valid info for the GOP, but because of the Android kernel relying on panel drivers with various things hardcoded some DMI quirks are necessary to fix some issues on these devices. Some of these issues also are related to which I2C bus to use for MIPI sequence elements which do I2C transfers. This series also includes a patch adding some extra debugging to mipi_exec_i2c() to help with debugging similar issues in the future. These patches have been posted before but back then I did not get around to follow up on the series: https://lore.kernel.org/intel-gfx/20220225214934.383168-1-hdego...@redhat.com/ Changes compared to this old version: - Drop the changes how the I2C bus number is found, instead just have the quirks set the right number directly where necessary. This should avoid any chances of causing regressions on devices where the quirks do not apply. - New quirk for backlight control issues on Lenovo Yoga Tab 3 - Address Jani Nikula's remark about __func__ being redundant when using drm_dbg_kms() Regards, Hans Hans de Goede (4): drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on Asus TF103C (v2) drm/i915/vlv_dsi: Add DMI quirk for wrong I2C bus and panel size on Lenovo Yoga Tablet 2 series (v2) drm/i915/vlv_dsi: Add DMI quirk for backlight control issues on Lenovo Yoga Tab 3 drm/i915/dsi: Add some debug logging to mipi_exec_i2c (v2) drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 + drivers/gpu/drm/i915/display/vlv_dsi.c | 124 +++ 2 files changed, 127 insertions(+) -- 2.41.0
[PATCH 1/4] drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on Asus TF103C (v2)
Vtotal is wrong in the BIOS supplied modeline for the DSI panel on
the Asus TF103C leading to the last line of the display being shown
as the first line.
Original: "1280x800": 60 67700 1280 1312 1328 1376 800 808 812 820 0x8 0xa
Fixed:"1280x800": 60 67700 1280 1312 1328 1376 800 808 812 816 0x8 0xa
The factory installed Android has a hardcoded modeline in its kernel,
causing it to not suffer from this BIOS bug;
and the Android boot-splash which uses the EFI FB which does have this bug
has the last line all black causing the bug to not be visible.
This commit introduces a generic DMI based quirk mechanism to vlv_dsi for
doing various fixups, and uses this to correct the modeline.
v2:
- s/mode_fixup/dmi_quirk/ to make the new DMI quirk mechanism more generic
- Add a comment with the old and new modelines to the patch and commit msg
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/i915/display/vlv_dsi.c | 42 ++
1 file changed, 42 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
b/drivers/gpu/drm/i915/display/vlv_dsi.c
index a96e7d028c5c..51c4b1491fa2 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -23,6 +23,7 @@
* Author: Jani Nikula
*/
+#include
#include
#include
@@ -1744,6 +1745,38 @@ static void vlv_dphy_param_init(struct intel_dsi
*intel_dsi)
intel_dsi_log_params(intel_dsi);
}
+typedef void (*vlv_dsi_dmi_quirk_func)(struct intel_dsi *intel_dsi);
+
+/*
+ * Vtotal is wrong on the Asus TF103C leading to the last line of the display
+ * being shown as the first line. The factory installed Android has a hardcoded
+ * modeline, causing it to not suffer from this BIOS bug.
+ *
+ * Original mode: "1280x800": 60 67700 1280 1312 1328 1376 800 808 812 820 0x8
0xa
+ * Fixedmode: "1280x800": 60 67700 1280 1312 1328 1376 800 808 812 816 0x8
0xa
+ */
+static void vlv_dsi_asus_tf103c_mode_fixup(struct intel_dsi *intel_dsi)
+{
+ /* Cast away the const as we want to fixup the mode */
+ struct drm_display_mode *fixed_mode = (struct drm_display_mode *)
+ intel_panel_preferred_fixed_mode(intel_dsi->attached_connector);
+
+ if (fixed_mode->vtotal == 820)
+ fixed_mode->vtotal -= 4;
+}
+
+static const struct dmi_system_id vlv_dsi_dmi_quirk_table[] = {
+ {
+ /* Asus Transformer Pad TF103C */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "TF103C"),
+ },
+ .driver_data = (void *)vlv_dsi_asus_tf103c_mode_fixup,
+ },
+ { }
+};
+
void vlv_dsi_init(struct drm_i915_private *dev_priv)
{
struct intel_dsi *intel_dsi;
@@ -1752,6 +1785,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
struct intel_connector *intel_connector;
struct drm_connector *connector;
struct drm_display_mode *current_mode;
+ const struct dmi_system_id *dmi_id;
enum port port;
enum pipe pipe;
@@ -1883,6 +1917,14 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
goto err_cleanup_connector;
}
+ dmi_id = dmi_first_match(vlv_dsi_dmi_quirk_table);
+ if (dmi_id) {
+ vlv_dsi_dmi_quirk_func quirk_func =
+ (vlv_dsi_dmi_quirk_func)dmi_id->driver_data;
+
+ quirk_func(intel_dsi);
+ }
+
intel_panel_init(intel_connector, NULL);
intel_backlight_setup(intel_connector, INVALID_PIPE);
--
2.41.0
Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
Hi Julius, On 9/4/23 21:02, Julius Zint wrote: > > > On Mon, 4 Sep 2023, Thomas Weißschuh wrote: > >> +Cc Hans who ins involved with the backlight subsystem >> >> Hi Julius, >> >> today I stumbled upon a mail from Hans [0], which explains that the >> backlight subsystem is not actually a good fit (yet?) for external >> displays. >> >> It seems a new API is in the works that would better fit, but I'm not >> sure about the state of this API. Maybe Hans can clarify. >> >> This also ties back to my review question how userspace can figure out >> to which display a backlight devices applies. So far it can not. >> >> [0] >> https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfd...@redhat.com/ >> > > Hi Thomas, > > thanks for the hint. I will make sure to give this a proper read and > see, if it fits my use case better then the current backlight subsystem. Note the actual proposal for the new usespace API for display brightness control is here: https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/ I have finished / stabilized the backlight code refactor which I landed in 6.1, which is a prerequisite for the above proposal. But I have not been able to make time to actually implement the above proposal; and I don't know when I will be able to make time for this. > Especially since I wasnt able to properly address your other review > comments for now. You are right that the name should align better with > the kernel module and also, that it is possible for multiple displays to > be attached. > > In its current state, this would mean that you could only control the > backlight for the first HID device (enough for me :-). > > The systemd-backlight@.service uses not only the file name, but also the > full bus path for storing/restoring backlights. I did not yet get around > to see how the desktops handle brightness control, but since the > systemd-backlight@.service already uses the name, its important to stay > the same over multiple boots. > > I would be able to get a handle on the underlying USB device and use the > serial to uniquely (and persistently) name the backlight. But it does > feel hacky doing it this way. So mutter (gnome-shell compositor library) has a similar issue when saving monitor layouts and I can tell you beforehand that monitor serial numbers by themselves are not unique enough. Some models just report 123456789 as serial and if you have a dual-monitor setup with 2 such monitors and name the backlight class device -vcp-hid or something like that you will still end up with 2 identical names. To avoid this when saving monitor layouts mutter saves both the port to which the monitor is attached (e.g. DP-1 DP-2) and the serialnumber and on startup / monitor hotplug when it checks to see if it has saved layout info for the monitor it checks the port+serialnr combination. So what I think you should do is figure out a way to map which VCP HID device maps to which drm-connector and then use the connector-name + serial-nr to generate the backlight device name. We will need the mapping the a drm-connector object anyway for the new brightness API proposal from above. Note this does NOT solve the fact that registering a new backlight class device for an external monitor on a laptop will hopelessly confuse userspace, see: https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfd...@redhat.com/ Regards, Hans
Re: [PATCH v2] drm: gm12u320: Fix the timeout usage for usb_bulk_msg()
Hi,
On 9/4/23 04:14, Jinjie Ruan wrote:
> The timeout arg of usb_bulk_msg() is ms already, which has been converted
> to jiffies by msecs_to_jiffies() in usb_start_wait_urb(). So fix the usage
> by removing the redundant msecs_to_jiffies() in the macros.
>
> And as Hans suggested, also remove msecs_to_jiffies() for the IDLE_TIMEOUT
> macro to make it consistent here and so change IDLE_TIMEOUT to
> msecs_to_jiffies(IDLE_TIMEOUT) where it is used.
>
> Fixes: e4f86e437164 ("drm: Add Grain Media GM12U320 driver v2")
> Signed-off-by: Jinjie Ruan
> Suggested-by: Hans de Goede
> ---
> v2:
> - Remove the msecs_to_jiffies() also for IDLE_TIMEOUT.
> - Update the fix tag.
> - Update the commit message.
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede
Note I don't gave time to merge this myself atm.
If one of the drm-misc maintainers can pick this up that would
be great.
Regards,
Hans
> ---
> drivers/gpu/drm/tiny/gm12u320.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index c5bb683e440c..0187539ff5ea 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -70,10 +70,10 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less
> bright, more silent)");
> #define READ_STATUS_SIZE 13
> #define MISC_VALUE_SIZE 4
>
> -#define CMD_TIMEOUT msecs_to_jiffies(200)
> -#define DATA_TIMEOUT msecs_to_jiffies(1000)
> -#define IDLE_TIMEOUT msecs_to_jiffies(2000)
> -#define FIRST_FRAME_TIMEOUT msecs_to_jiffies(2000)
> +#define CMD_TIMEOUT 200
> +#define DATA_TIMEOUT 1000
> +#define IDLE_TIMEOUT 2000
> +#define FIRST_FRAME_TIMEOUT 2000
>
> #define MISC_REQ_GET_SET_ECO_A 0xff
> #define MISC_REQ_GET_SET_ECO_B 0x35
> @@ -389,7 +389,7 @@ static void gm12u320_fb_update_work(struct work_struct
> *work)
>* switches back to showing its logo.
>*/
> queue_delayed_work(system_long_wq, >fb_update.work,
> -IDLE_TIMEOUT);
> +msecs_to_jiffies(IDLE_TIMEOUT));
>
> return;
> err:
Re: [PATCH] drm: gm12u320: Fix the timeout usage for usb_bulk_msg()
Hi Jinjie,
On 9/3/23 10:55, Jinjie Ruan wrote:
> The timeout arg of usb_bulk_msg() is ms already and it has convert it
> to jiffies by msecs_to_jiffies() in usb_start_wait_urb(). So fix the usage
> by remove the redundant msecs_to_jiffies() in the macros.
>
> Fixes: 77b8cabf3d52 ("drm/gm12u320: Move driver to drm/tiny")
> Signed-off-by: Jinjie Ruan
Good catch, thank you.
> ---
> drivers/gpu/drm/tiny/gm12u320.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index c5bb683e440c..31fa50c665d1 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -70,10 +70,10 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less
> bright, more silent)");
> #define READ_STATUS_SIZE 13
> #define MISC_VALUE_SIZE 4
>
> -#define CMD_TIMEOUT msecs_to_jiffies(200)
> -#define DATA_TIMEOUT msecs_to_jiffies(1000)
> +#define CMD_TIMEOUT 200
> +#define DATA_TIMEOUT 1000
> #define IDLE_TIMEOUT msecs_to_jiffies(2000)
> -#define FIRST_FRAME_TIMEOUT msecs_to_jiffies(2000)
> +#define FIRST_FRAME_TIMEOUT 2000
Lets be consistent here and change all *_TIMEOUT defines to
be in milliseconds.
And then in the 1 place where IDLE_TIMEOUT is used, change
"IDLE_TIMEOUT" to "msecs_to_jiffies(IDLE_TIMEOUT)"
REgards,
Hans
>
> #define MISC_REQ_GET_SET_ECO_A 0xff
> #define MISC_REQ_GET_SET_ECO_B 0x35
Re: [PATCH] drm: panel-orientation-quirks: Change Air's quirk to support Air Plus
Hi,
On 5/15/23 20:48, Maya Matuszczyk wrote:
> It turned out that Aya Neo Air Plus had a different board name than
> expected.
> This patch changes Aya Neo Air's quirk to account for that, as both
> devices share "Air" in DMI product name.
>
> Tested on Air claiming to be an Air Pro, and on Air Plus.
>
> Signed-off-by: Maya Matuszczyk
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede
I have just pushed this to drm-misc-fixes, so it should get
send to Linus with the next drm-fixes pull-req.
Regards,
Hans
> ---
> drivers/gpu/drm/drm_panel_orientation_quirks.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> index b1a38e6ce2f8..0cb646cb04ee 100644
> --- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> @@ -179,7 +179,7 @@ static const struct dmi_system_id orientation_data[] = {
> }, {/* AYA NEO AIR */
> .matches = {
> DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AYANEO"),
> - DMI_MATCH(DMI_BOARD_NAME, "AIR"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "AIR"),
> },
> .driver_data = (void *)_leftside_up,
> }, {/* AYA NEO NEXT */
Re: [PATCH 6.2 regression fix] drm/nouveau/kms: Fix backlight registration
Hi,
On 3/29/23 00:23, Lyude Paul wrote:
> Reviewed-by: Lyude Paul
>
> (Also note to Mark: this is my way of letting you know someone fixed the
> regression with backlight controls upstream, looking into the weird bright
> screen after resume issue)
Thanks.
I have pushed this to drm-misc-fixes now.
I'll also submit a downstream Fedora kernel pull-req with this
to get this resolved in the Fedora kernels .
Regards,
Hans
>
> On Sun, 2023-03-26 at 22:54 +0200, Hans de Goede wrote:
>> The nouveau code used to call drm_fb_helper_initial_config() from
>> nouveau_fbcon_init() before calling drm_dev_register(). This would
>> probe all connectors so that drm_connector->status could be used during
>> backlight registration which runs from nouveau_connector_late_register().
>>
>> After commit 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
>> the fbdev emulation code, which now is a drm-client, can only run after
>> drm_dev_register(). So during backlight registration the connectors are
>> not probed yet and the drm_connector->status == connected check in
>> nv50_backlight_init() would now always fail.
>>
>> Replace the drm_connector->status == connected check with
>> a drm_helper_probe_detect() == connected check to fix nv_backlight
>> no longer getting registered because of this.
>>
>> Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
>> Link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/202
>> Signed-off-by: Hans de Goede
>> ---
>> drivers/gpu/drm/nouveau/nouveau_backlight.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
>> b/drivers/gpu/drm/nouveau/nouveau_backlight.c
>> index 40409a29f5b6..91b5ecc57538 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
>> @@ -33,6 +33,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "nouveau_drv.h"
>> #include "nouveau_reg.h"
>> @@ -299,8 +300,12 @@ nv50_backlight_init(struct nouveau_backlight *bl,
>> struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
>> struct nvif_object *device = >client.device.object;
>>
>> +/*
>> + * Note when this runs the connectors have not been probed yet,
>> + * so nv_conn->base.status is not set yet.
>> + */
>> if (!nvif_rd32(device, NV50_PDISP_SOR_PWM_CTL(ffs(nv_encoder->dcb->or)
>> - 1)) ||
>> -nv_conn->base.status != connector_status_connected)
>> +drm_helper_probe_detect(_conn->base, NULL, false) !=
>> connector_status_connected)
>> return -ENODEV;
>>
>> if (nv_conn->type == DCB_CONNECTOR_eDP) {
>
[PATCH 6.2 regression fix] drm/nouveau/kms: Fix backlight registration
The nouveau code used to call drm_fb_helper_initial_config() from
nouveau_fbcon_init() before calling drm_dev_register(). This would
probe all connectors so that drm_connector->status could be used during
backlight registration which runs from nouveau_connector_late_register().
After commit 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
the fbdev emulation code, which now is a drm-client, can only run after
drm_dev_register(). So during backlight registration the connectors are
not probed yet and the drm_connector->status == connected check in
nv50_backlight_init() would now always fail.
Replace the drm_connector->status == connected check with
a drm_helper_probe_detect() == connected check to fix nv_backlight
no longer getting registered because of this.
Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
Link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/202
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/nouveau/nouveau_backlight.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 40409a29f5b6..91b5ecc57538 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include "nouveau_drv.h"
#include "nouveau_reg.h"
@@ -299,8 +300,12 @@ nv50_backlight_init(struct nouveau_backlight *bl,
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
struct nvif_object *device = >client.device.object;
+ /*
+* Note when this runs the connectors have not been probed yet,
+* so nv_conn->base.status is not set yet.
+*/
if (!nvif_rd32(device, NV50_PDISP_SOR_PWM_CTL(ffs(nv_encoder->dcb->or)
- 1)) ||
- nv_conn->base.status != connector_status_connected)
+ drm_helper_probe_detect(_conn->base, NULL, false) !=
connector_status_connected)
return -ENODEV;
if (nv_conn->type == DCB_CONNECTOR_eDP) {
--
2.39.1
Re: [PATCH] drm: panel-orientation-quirks: Add quirk for Lenovo Yoga Book X90F
Hi, On 3/13/23 16:39, Javier Martinez Canillas wrote: > Hans de Goede writes: > > Hello Hans, > >> Like the Windows Lenovo Yoga Book X91F/L the Android Lenovo Yoga Book >> X90F/L has a portrait 1200x1920 screen used in landscape mode, >> add a quirk for this. >> >> When the quirk for the X91F/L was initially added it was written to >> also apply to the X90F/L but this does not work because the Android >> version of the Yoga Book uses completely different DMI strings. >> Also adjust the X91F/L quirk to reflect that it only applies to >> the X91F/L models. >> >> Signed-off-by: Hans de Goede >> --- > > Patch looks good to me. > > Reviewed-by: Javier Martinez Canillas Thanks, I have pushed this to drm-misc-fixes now. Regards, Hans
Re: [REGRESSION] Missing nouveau backlight control on 6.2.x kernel
Hi Takashi, On 3/17/23 11:04, Takashi Iwai wrote: > Hi, > > we've received a regression report on openSUSE Bugzilla about the > missing backlight control of nouveau device: > https://bugzilla.opensuse.org/show_bug.cgi?id=1209296 > > On 6.1, with acpi_video=native option, the system provided > /sys/class/backlight/nv_backlight entry. > > On 6.2, with acpi_video=native option, there is no entry in > /sys/class/backlight. And without acpi_video option, it falls back to > /sys/class/backlight/acpi_video0, which doesn't work as expected. > > Hans, could you check whether the recent change in video_detect.c (or > anything else) may influence on this? My gut feeling is that the > culprit could be rather some change in nouveau (so dri-devel is > Cc'ed), but I'm not entirely sure... As I already told the reporter, who first contacted me by private email about this, all the drivers/acpi/video_detect.c changes which may influence this are also present in 6.1.y (IIRC >= 6.1.5) . So since this is being reported as only happening with 6.2.y I don't think the recent backlight detect rework is involved. And even if the recent backlight detect rework were involved, specifying acpi_backlight=native on the kernel commandline (as the reporter was doing before) should still work since it overrides all other backlight detection. So yes this seems to be a nouveau bug and should probably be reported following the instructions from: https://nouveau.freedesktop.org/Bugs.html (which I havealso told the reporter already). Regards, Hans
Re: [PATCH v3 0/6] drm/amd/display: Pass proper parent for DM backlight device v3
Hi, On 3/16/23 15:57, Rodrigo Siqueira Jordao wrote: > > > On 3/12/23 13:17, Hans de Goede wrote: >> Hi All, >> >> Here is version 3 of my patch series to pass the proper parent device >> to backlight_device_register(). >> >> Changes in v3: >> - Make amdgpu_dm_register_backlight_device() check bl_idx != 1 before >> registering the backlight since amdgpu_dm_connector_late_register() >> now calls it for _all_ connectors. >> >> Changes in v2: >> - Patches 1 - 5 are new, reworking the code a bit to allow delaying >> the registering, so this has turned from a single patch into >> a 6 patch set. >> - Patch 6 now delays the registering of the backlight_dev till >> after the drm_connector is registered by doing it from >> drm_connector_funcs.late_register. >> >> Note this no longer is RFC since this has been successfully >> tested on 3 laptops which hit the affected code path. >> >> Version 3 has also been tested on my personal AMD Ryzen 7 5700G APU >> desktop machine and now no longer tries to register a backlight >> device for each connector there. >> >> Regards, >> >> Hans >> >> >> Hans de Goede (6): >> drm/amd/display/amdgpu_dm: Fix backlight_device_register() error >> handling >> drm/amd/display/amdgpu_dm: Refactor register_backlight_device() >> drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector >> drm/amd/display/amdgpu_dm: Move most backlight setup into >> setup_backlight_device() >> drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() >> take an amdgpu_dm_connector >> drm/amd/display/amdgpu_dm: Pass proper parent for backlight device >> registration v3 >> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 100 -- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + >> 2 files changed, 46 insertions(+), 55 deletions(-) >> > > Hi, > > First of all, thanks a lot for this patchset. > > I run your series in our CI (IGT-based), and I also conducted some manual > tests in my ASICs. Everything looks fine. > > I also reviewed this series, and it LGTM: > > Reviewed-by: Rodrigo Siqueira > > Finally, I pushed it to amd-staging-drm-next. Great, thank you. Regards, Hans
[GIT PULL] Immutable branch between pdx86 and backlight due for the v6.4 merge window
Hi Lee, As requested here is a pull-request for the apple-gmux / apple-backlight changes between the pdx86 and backlight subsytems. Regards, Hans The following changes since commit fe15c26ee26efa11741a7b632e9f23b01aca4cc6: Linux 6.3-rc1 (2023-03-05 14:52:03 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git tags/ib-pdx86-backlight-6.4 for you to fetch changes up to 3608a2cd818a8d97bd3d4dc32f6c0afaf09b15ec: backlight: apple_bl: Use acpi_video_get_backlight_type() (2023-03-16 13:33:11 +0100) Immutable branch between pdx86 and backlight due for the v6.4 merge window Dan Carpenter (1): platform/x86: apple-gmux: return -EFAULT if copy fails Hans de Goede (2): platform/x86: apple-gmux: Add acpi_video_get_backlight_type() check backlight: apple_bl: Use acpi_video_get_backlight_type() Orlando Chamberlain (5): platform/x86: apple-gmux: use first bit to check switch state platform/x86: apple-gmux: refactor gmux types platform/x86: apple-gmux: support MMIO gmux on T2 Macs platform/x86: apple-gmux: add debugfs interface platform/x86: apple-gmux: Update apple_gmux_detect documentation drivers/platform/x86/Kconfig | 1 - drivers/platform/x86/apple-gmux.c | 404 ++--- drivers/video/backlight/Kconfig| 1 + drivers/video/backlight/apple_bl.c | 31 +-- include/linux/apple-gmux.h | 72 +-- include/linux/apple_bl.h | 27 --- 6 files changed, 393 insertions(+), 143 deletions(-) delete mode 100644 include/linux/apple_bl.h
[PATCH 2/2] efi: sysfb_efi: Add quirk for Lenovo Yoga Book X91F/L
Another Lenovo convertable which reports a landscape resolution of
1920x1200 with a pitch of (1920 * 4) bytes, while the actual framebuffer
has a resolution of 1200x1920 with a pitch of (1200 * 4) bytes.
Signed-off-by: Hans de Goede
---
drivers/firmware/efi/sysfb_efi.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/firmware/efi/sysfb_efi.c b/drivers/firmware/efi/sysfb_efi.c
index e76d6803bdd0..456d0e5eaf78 100644
--- a/drivers/firmware/efi/sysfb_efi.c
+++ b/drivers/firmware/efi/sysfb_efi.c
@@ -272,6 +272,14 @@ static const struct dmi_system_id
efifb_dmi_swap_width_height[] __initconst = {
"IdeaPad Duet 3 10IGL5"),
},
},
+ {
+ /* Lenovo Yoga Book X91F / X91L */
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ /* Non exact match to match F + L versions */
+ DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X91"),
+ },
+ },
{},
};
--
2.39.1
[PATCH 1/2] efi: sysfb_efi: Fix DMI quirks not working for simpledrm
Commit 8633ef82f101 ("drivers/firmware: consolidate EFI framebuffer setup
for all arches") moved the sysfb_apply_efi_quirks() call in sysfb_init()
from before the [sysfb_]parse_mode() call to after it.
But sysfb_apply_efi_quirks() modifies the global screen_info struct which
[sysfb_]parse_mode() parses, so doing it later is too late.
This has broken all DMI based quirks for correcting wrong firmware efifb
settings when simpledrm is used.
To fix this move the sysfb_apply_efi_quirks() call back to its old place
and split the new setup of the efifb_fwnode (which requires
the platform_device) into its own function and call that at
the place of the moved sysfb_apply_efi_quirks(pd) calls.
Fixes: 8633ef82f101 ("drivers/firmware: consolidate EFI framebuffer setup for
all arches")
Cc: sta...@vger.kernel.org
Cc: Javier Martinez Canillas
Cc: Thomas Zimmermann
Signed-off-by: Hans de Goede
---
drivers/firmware/efi/sysfb_efi.c | 5 -
drivers/firmware/sysfb.c | 4 +++-
drivers/firmware/sysfb_simplefb.c | 2 +-
include/linux/sysfb.h | 9 +++--
4 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/efi/sysfb_efi.c b/drivers/firmware/efi/sysfb_efi.c
index f06fdacc9bc8..e76d6803bdd0 100644
--- a/drivers/firmware/efi/sysfb_efi.c
+++ b/drivers/firmware/efi/sysfb_efi.c
@@ -341,7 +341,7 @@ static const struct fwnode_operations efifb_fwnode_ops = {
#ifdef CONFIG_EFI
static struct fwnode_handle efifb_fwnode;
-__init void sysfb_apply_efi_quirks(struct platform_device *pd)
+__init void sysfb_apply_efi_quirks(void)
{
if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI ||
!(screen_info.capabilities & VIDEO_CAPABILITY_SKIP_QUIRKS))
@@ -355,7 +355,10 @@ __init void sysfb_apply_efi_quirks(struct platform_device
*pd)
screen_info.lfb_height = temp;
screen_info.lfb_linelength = 4 * screen_info.lfb_width;
}
+}
+__init void sysfb_set_efifb_fwnode(struct platform_device *pd)
+{
if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI &&
IS_ENABLED(CONFIG_PCI)) {
fwnode_init(_fwnode, _fwnode_ops);
pd->dev.fwnode = _fwnode;
diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 3fd3563d962b..3c197db42c9d 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -81,6 +81,8 @@ static __init int sysfb_init(void)
if (disabled)
goto unlock_mutex;
+ sysfb_apply_efi_quirks();
+
/* try to create a simple-framebuffer device */
compatible = sysfb_parse_mode(si, );
if (compatible) {
@@ -107,7 +109,7 @@ static __init int sysfb_init(void)
goto unlock_mutex;
}
- sysfb_apply_efi_quirks(pd);
+ sysfb_set_efifb_fwnode(pd);
ret = platform_device_add_data(pd, si, sizeof(*si));
if (ret)
diff --git a/drivers/firmware/sysfb_simplefb.c
b/drivers/firmware/sysfb_simplefb.c
index ce9c007ed66f..82c64cb9f531 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -141,7 +141,7 @@ __init struct platform_device *sysfb_create_simplefb(const
struct screen_info *s
if (!pd)
return ERR_PTR(-ENOMEM);
- sysfb_apply_efi_quirks(pd);
+ sysfb_set_efifb_fwnode(pd);
ret = platform_device_add_resources(pd, , 1);
if (ret)
diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
index 8ba8b5be5567..c1ef5fc60a3c 100644
--- a/include/linux/sysfb.h
+++ b/include/linux/sysfb.h
@@ -70,11 +70,16 @@ static inline void sysfb_disable(void)
#ifdef CONFIG_EFI
extern struct efifb_dmi_info efifb_dmi_list[];
-void sysfb_apply_efi_quirks(struct platform_device *pd);
+void sysfb_apply_efi_quirks(void);
+void sysfb_set_efifb_fwnode(struct platform_device *pd);
#else /* CONFIG_EFI */
-static inline void sysfb_apply_efi_quirks(struct platform_device *pd)
+static inline void sysfb_apply_efi_quirks(void)
+{
+}
+
+static inline void sysfb_set_efifb_fwnode(struct platform_device *pd)
{
}
--
2.39.1
Re: [PATCH] backlight: apple_bl: Use acpi_video_get_backlight_type()
Hi, On 3/9/23 18:09, Daniel Thompson wrote: > On Tue, Mar 07, 2023 at 01:05:40PM +0100, Hans de Goede wrote: >> On some MacBooks both the apple_bl and the apple-gmux backlight drivers >> may be able to export a /sys/class/backlight device. >> >> To avoid having 2 backlight devices for one LCD panel until now >> the apple-gmux driver has been calling apple_bl_unregister() to move >> the apple_bl backlight device out of the way when it loads. >> >> Similar problems exist on other x86 laptops and all backlight drivers >> which may be used on x86 laptops have moved to using >> acpi_video_get_backlight_type() to determine whether they should load >> or not. >> >> Switch apple_bl to this model too, so that it is consistent with all >> the other x86 backlight drivers. >> [snip] >> Signed-off-by: Hans de Goede > > Reviewed-by: Daniel Thompson > > but... Thank you. >> --- >> A note to the backlight class / subsystem maintainers, this change >> applies on top of a similar patch for drivers/platform/x86/apple-gmux.c >> which makes that driver use acpi_video_get_backlight_type(). See: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> I believe it is easiest to also merge this patch through >> the platform-drivers-x86 tree, may I please have your Ack for this ? >> --- > > ... please don't treat above as an ack. Lee Jones will hopefully be > along shortly to discuss that! Understood. Regards, Hans
[PATCH v3 4/6] drm/amd/display/amdgpu_dm: Move most backlight setup into setup_backlight_device()
Rename register_backlight_device() to setup_backlight_device()
and move all backlight setup related calls from
amdgpu_dm_register_backlight_device() and from
amdgpu_dm_initialize_drm_device() there.
This leaves amdgpu_dm_register_backlight_device() dealing purely
with registering the actual backlight class device.
This is a preparation patch for moving the actual backlight class device
registering to drm_connector_funcs.late_register.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index eb1f2073b0cf..757202af2eec 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4145,9 +4145,6 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
char bl_name[16];
struct backlight_properties props = { 0 };
- amdgpu_dm_update_backlight_caps(dm, dm->num_of_edps);
- dm->brightness[dm->num_of_edps] = AMDGPU_MAX_BL_LEVEL;
-
if (!acpi_video_backlight_use_native()) {
drm_info(adev_to_drm(dm->adev), "Skipping amdgpu DM backlight
registration\n");
/* Try registering an ACPI video backlight device instead. */
@@ -4216,8 +4213,8 @@ static int initialize_plane(struct amdgpu_display_manager
*dm,
}
-static void register_backlight_device(struct amdgpu_display_manager *dm,
- struct amdgpu_dm_connector *aconnector)
+static void setup_backlight_device(struct amdgpu_display_manager *dm,
+ struct amdgpu_dm_connector *aconnector)
{
struct dc_link *link = aconnector->dc_link;
int bl_idx = dm->num_of_edps;
@@ -4233,6 +4230,9 @@ static void register_backlight_device(struct
amdgpu_display_manager *dm,
aconnector->bl_idx = bl_idx;
+ amdgpu_dm_update_backlight_caps(dm, bl_idx);
+ dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
+
amdgpu_dm_register_backlight_device(dm);
if (!dm->backlight_dev[bl_idx]) {
aconnector->bl_idx = -1;
@@ -4241,6 +4241,8 @@ static void register_backlight_device(struct
amdgpu_display_manager *dm,
dm->backlight_link[bl_idx] = link;
dm->num_of_edps++;
+
+ update_connector_ext_caps(aconnector);
}
static void amdgpu_set_panel_orientation(struct drm_connector *connector);
@@ -4423,10 +4425,7 @@ static int amdgpu_dm_initialize_drm_device(struct
amdgpu_device *adev)
if (ret) {
amdgpu_dm_update_connector_after_detect(aconnector);
- register_backlight_device(dm, aconnector);
-
- if (dm->num_of_edps)
- update_connector_ext_caps(aconnector);
+ setup_backlight_device(dm, aconnector);
if (psr_feature_enabled)
amdgpu_dm_set_psr_caps(link);
--
2.39.1
[PATCH v3 6/6] drm/amd/display/amdgpu_dm: Pass proper parent for backlight device registration v3
The parent for the backlight device should be the drm-connector object,
not the PCI device.
Userspace relies on this to be able to detect which backlight class device
to use on hybrid gfx devices where there may be multiple native (raw)
backlight devices registered.
Specifically gnome-settings-daemon expects the parent device to have
an "enabled" sysfs attribute (as drm_connector devices do) and tests
that this returns "enabled" when read.
This aligns the parent of the backlight device with i915, nouveau, radeon.
Note that drivers/gpu/drm/amd/amdgpu/atombios_encoders.c also already
uses the drm_connector as parent, only amdgpu_dm.c used the PCI device
as parent before this change.
Changes in v3:
Make amdgpu_dm_register_backlight_device() check bl_idx != 1 before
registering the backlight since amdgpu_dm_connector_late_register()
now calls it for _all_ connectors.
Changes in v2:
Together with changing the parent, also move the registration to
drm_connector_funcs.late_register() this is necessary because the parent
device (which now is the drm_connector) must be registered before
the backlight class device is, otherwise the backlight class device ends
up without any parent set at all.
This brings the backlight class device registration timing inline with
nouveau and i915 which also use drm_connector_funcs.late_register()
for this.
Note this slightly changes backlight_device_register() error handling,
instead of not increasing dm->num_of_edps and re-using the current
bl_idx for a potential other backlight device, dm->backlight_dev[bl_idx]
is now simply left NULL on failure. This is ok because all code
looking at dm->backlight_dev[i] also checks it is not NULL.
Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 038bf897cc28..169b307888c1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4147,6 +4147,9 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_dm_connector *aconnector)
struct backlight_properties props = { 0 };
char bl_name[16];
+ if (aconnector->bl_idx == -1)
+ return;
+
if (!acpi_video_backlight_use_native()) {
drm_info(drm, "Skipping amdgpu DM backlight registration\n");
/* Try registering an ACPI video backlight device instead. */
@@ -4162,7 +4165,7 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_dm_connector *aconnector)
drm->primary->index + aconnector->bl_idx);
dm->backlight_dev[aconnector->bl_idx] =
- backlight_device_register(bl_name, drm->dev, dm,
+ backlight_device_register(bl_name, aconnector->base.kdev, dm,
_dm_backlight_ops, );
if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
@@ -4232,13 +4235,6 @@ static void setup_backlight_device(struct
amdgpu_display_manager *dm,
amdgpu_dm_update_backlight_caps(dm, bl_idx);
dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
-
- amdgpu_dm_register_backlight_device(aconnector);
- if (!dm->backlight_dev[bl_idx]) {
- aconnector->bl_idx = -1;
- return;
- }
-
dm->backlight_link[bl_idx] = link;
dm->num_of_edps++;
@@ -6297,6 +6293,8 @@ amdgpu_dm_connector_late_register(struct drm_connector
*connector)
to_amdgpu_dm_connector(connector);
int r;
+ amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+
if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
(connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
amdgpu_dm_connector->dm_dp_aux.aux.dev = connector->kdev;
--
2.39.1
[PATCH v3 5/6] drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() take an amdgpu_dm_connector
Make amdgpu_dm_register_backlight_device() take an amdgpu_dm_connector
pointer to the connector for which it should register the backlight
as its only argument.
This is a preparation patch for moving the actual backlight class device
registering to drm_connector_funcs.late_register.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +--
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 757202af2eec..038bf897cc28 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4140,13 +4140,15 @@ static const struct backlight_ops
amdgpu_dm_backlight_ops = {
};
static void
-amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm)
+amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
{
- char bl_name[16];
+ struct drm_device *drm = aconnector->base.dev;
+ struct amdgpu_display_manager *dm = _to_adev(drm)->dm;
struct backlight_properties props = { 0 };
+ char bl_name[16];
if (!acpi_video_backlight_use_native()) {
- drm_info(adev_to_drm(dm->adev), "Skipping amdgpu DM backlight
registration\n");
+ drm_info(drm, "Skipping amdgpu DM backlight registration\n");
/* Try registering an ACPI video backlight device instead. */
acpi_video_register_backlight();
return;
@@ -4157,17 +4159,15 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
props.type = BACKLIGHT_RAW;
snprintf(bl_name, sizeof(bl_name), "amdgpu_bl%d",
-adev_to_drm(dm->adev)->primary->index + dm->num_of_edps);
+drm->primary->index + aconnector->bl_idx);
- dm->backlight_dev[dm->num_of_edps] = backlight_device_register(bl_name,
-
adev_to_drm(dm->adev)->dev,
- dm,
-
_dm_backlight_ops,
- );
+ dm->backlight_dev[aconnector->bl_idx] =
+ backlight_device_register(bl_name, drm->dev, dm,
+ _dm_backlight_ops, );
- if (IS_ERR(dm->backlight_dev[dm->num_of_edps])) {
+ if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
DRM_ERROR("DM: Backlight registration failed!\n");
- dm->backlight_dev[dm->num_of_edps] = NULL;
+ dm->backlight_dev[aconnector->bl_idx] = NULL;
} else
DRM_DEBUG_DRIVER("DM: Registered Backlight device: %s\n",
bl_name);
}
@@ -4233,7 +4233,7 @@ static void setup_backlight_device(struct
amdgpu_display_manager *dm,
amdgpu_dm_update_backlight_caps(dm, bl_idx);
dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
- amdgpu_dm_register_backlight_device(dm);
+ amdgpu_dm_register_backlight_device(aconnector);
if (!dm->backlight_dev[bl_idx]) {
aconnector->bl_idx = -1;
return;
--
2.39.1
[PATCH v3 3/6] drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector
Currently functions like update_connector_ext_caps() and
amdgpu_dm_connector_destroy() are iterating over dm->backlight_link[i]
to find the index of the (optional) backlight_dev associated with
the connector.
Instead make register_backlight_device() store the dm->backlight_dev[]
index used for the connector inside the amdgpu_dm_connector struct.
This removes the need to iterate over the dm->backlight_link[]
array and this is necessary as a preparation patch for moving
the actual backlight_device_register()
call to drm_connector_funcs.late_register.
While reworking update_connector_ext_caps() also remove the aconnector
and aconnector->dc_link NULL checks in this function. These are both
never NULL and are unconditionally derefed in its callers.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 +++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
2 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1b5efa56ec15..eb1f2073b0cf 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2936,30 +2936,18 @@ static struct drm_mode_config_helper_funcs
amdgpu_dm_mode_config_helperfuncs = {
static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
{
struct amdgpu_dm_backlight_caps *caps;
- struct amdgpu_display_manager *dm;
struct drm_connector *conn_base;
struct amdgpu_device *adev;
- struct dc_link *link = NULL;
struct drm_luminance_range_info *luminance_range;
- int i;
-
- if (!aconnector || !aconnector->dc_link)
- return;
- link = aconnector->dc_link;
- if (link->connector_signal != SIGNAL_TYPE_EDP)
+ if (aconnector->bl_idx == -1 ||
+ aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
return;
conn_base = >base;
adev = drm_to_adev(conn_base->dev);
- dm = >dm;
- for (i = 0; i < dm->num_of_edps; i++) {
- if (link == dm->backlight_link[i])
- break;
- }
- if (i >= dm->num_of_edps)
- return;
- caps = >backlight_caps[i];
+
+ caps = >dm.backlight_caps[aconnector->bl_idx];
caps->ext_caps = >dc_link->dpcd_sink_ext_caps;
caps->aux_support = false;
@@ -4229,8 +4217,9 @@ static int initialize_plane(struct amdgpu_display_manager
*dm,
static void register_backlight_device(struct amdgpu_display_manager *dm,
- struct dc_link *link)
+ struct amdgpu_dm_connector *aconnector)
{
+ struct dc_link *link = aconnector->dc_link;
int bl_idx = dm->num_of_edps;
if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
@@ -4242,9 +4231,13 @@ static void register_backlight_device(struct
amdgpu_display_manager *dm,
return;
}
+ aconnector->bl_idx = bl_idx;
+
amdgpu_dm_register_backlight_device(dm);
- if (!dm->backlight_dev[bl_idx])
+ if (!dm->backlight_dev[bl_idx]) {
+ aconnector->bl_idx = -1;
return;
+ }
dm->backlight_link[bl_idx] = link;
dm->num_of_edps++;
@@ -4430,7 +4423,7 @@ static int amdgpu_dm_initialize_drm_device(struct
amdgpu_device *adev)
if (ret) {
amdgpu_dm_update_connector_after_detect(aconnector);
- register_backlight_device(dm, link);
+ register_backlight_device(dm, aconnector);
if (dm->num_of_edps)
update_connector_ext_caps(aconnector);
@@ -6211,10 +6204,8 @@ static void amdgpu_dm_connector_unregister(struct
drm_connector *connector)
static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
{
struct amdgpu_dm_connector *aconnector =
to_amdgpu_dm_connector(connector);
- const struct dc_link *link = aconnector->dc_link;
struct amdgpu_device *adev = drm_to_adev(connector->dev);
struct amdgpu_display_manager *dm = >dm;
- int i;
/*
* Call only if mst_mgr was initialized before since it's not done
@@ -6223,11 +6214,9 @@ static void amdgpu_dm_connector_destroy(struct
drm_connector *connector)
if (aconnector->mst_mgr.dev)
drm_dp_mst_topology_mgr_destroy(>mst_mgr);
- for (i = 0; i < dm->num_of_edps; i++) {
- if ((link == dm->backlight_link[i]) && dm->backlight_dev[i]) {
- backlight_device_unregister(dm->backlight_dev[i]);
-
[PATCH v3 2/6] drm/amd/display/amdgpu_dm: Refactor register_backlight_device()
Refactor register_backlight_device():
1) Turn the connector-type + signal check into an early exit
condition to avoid the indentation level of the rest of the code
2) Add an array bounds check for the arrays indexed by dm->num_of_edps
3) register_backlight_device() always increases dm->num_of_edps if
amdgpu_dm_register_backlight_device() has assigned a backlight_dev to
the current dm->backlight_link[dm->num_of_edps] slot.
So on its next call dm->backlight_dev[dm->num_of_edps] always point to
the next empty slot and the "if (!dm->backlight_dev[dm->num_of_edps])"
check will thus always succeed and can be removed.
4) Add a bl_idx local variable to use as array index, rather then
using dm->num_of_edps to improve the code readability.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 ++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 42b88ab5552d..1b5efa56ec15 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4231,21 +4231,23 @@ static int initialize_plane(struct
amdgpu_display_manager *dm,
static void register_backlight_device(struct amdgpu_display_manager *dm,
struct dc_link *link)
{
- if ((link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) &&
- link->type != dc_connection_none) {
- /*
-* Event if registration failed, we should continue with
-* DM initialization because not having a backlight control
-* is better then a black screen.
-*/
- if (!dm->backlight_dev[dm->num_of_edps])
- amdgpu_dm_register_backlight_device(dm);
+ int bl_idx = dm->num_of_edps;
- if (dm->backlight_dev[dm->num_of_edps]) {
- dm->backlight_link[dm->num_of_edps] = link;
- dm->num_of_edps++;
- }
+ if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
+ link->type == dc_connection_none)
+ return;
+
+ if (dm->num_of_edps >= AMDGPU_DM_MAX_NUM_EDP) {
+ drm_warn(adev_to_drm(dm->adev), "Too much eDP connections,
skipping backlight setup for additional eDPs\n");
+ return;
}
+
+ amdgpu_dm_register_backlight_device(dm);
+ if (!dm->backlight_dev[bl_idx])
+ return;
+
+ dm->backlight_link[bl_idx] = link;
+ dm->num_of_edps++;
}
static void amdgpu_set_panel_orientation(struct drm_connector *connector);
--
2.39.1
[PATCH v3 1/6] drm/amd/display/amdgpu_dm: Fix backlight_device_register() error handling
backlight_device_register() returns an ERR_PTR on error, but other code
such as amdgpu_dm_connector_destroy() assumes dm->backlight_dev[i] is NULL
if no backlight is registered.
Clear dm->backlight_dev[i] on registration failure, to avoid other code
trying to deref an ERR_PTR pointer.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 009ef917dad4..42b88ab5552d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4180,9 +4180,10 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
_dm_backlight_ops,
);
- if (IS_ERR(dm->backlight_dev[dm->num_of_edps]))
+ if (IS_ERR(dm->backlight_dev[dm->num_of_edps])) {
DRM_ERROR("DM: Backlight registration failed!\n");
- else
+ dm->backlight_dev[dm->num_of_edps] = NULL;
+ } else
DRM_DEBUG_DRIVER("DM: Registered Backlight device: %s\n",
bl_name);
}
--
2.39.1
[PATCH v3 0/6] drm/amd/display: Pass proper parent for DM backlight device v3
Hi All, Here is version 3 of my patch series to pass the proper parent device to backlight_device_register(). Changes in v3: - Make amdgpu_dm_register_backlight_device() check bl_idx != 1 before registering the backlight since amdgpu_dm_connector_late_register() now calls it for _all_ connectors. Changes in v2: - Patches 1 - 5 are new, reworking the code a bit to allow delaying the registering, so this has turned from a single patch into a 6 patch set. - Patch 6 now delays the registering of the backlight_dev till after the drm_connector is registered by doing it from drm_connector_funcs.late_register. Note this no longer is RFC since this has been successfully tested on 3 laptops which hit the affected code path. Version 3 has also been tested on my personal AMD Ryzen 7 5700G APU desktop machine and now no longer tries to register a backlight device for each connector there. Regards, Hans Hans de Goede (6): drm/amd/display/amdgpu_dm: Fix backlight_device_register() error handling drm/amd/display/amdgpu_dm: Refactor register_backlight_device() drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector drm/amd/display/amdgpu_dm: Move most backlight setup into setup_backlight_device() drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() take an amdgpu_dm_connector drm/amd/display/amdgpu_dm: Pass proper parent for backlight device registration v3 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 100 -- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + 2 files changed, 46 insertions(+), 55 deletions(-) -- 2.39.1
Re: [RFC v2 6/6] drm/amd/display: Pass proper parent for DM backlight device registration v2
Hi,
On 3/8/23 22:58, Hans de Goede wrote:
> The parent for the backlight device should be the drm-connector object,
> not the PCI device.
>
> Userspace relies on this to be able to detect which backlight class device
> to use on hybrid gfx devices where there may be multiple native (raw)
> backlight devices registered.
>
> Specifically gnome-settings-daemon expects the parent device to have
> an "enabled" sysfs attribute (as drm_connector devices do) and tests
> that this returns "enabled" when read.
>
> This aligns the parent of the backlight device with i915, nouveau, radeon.
> Note that drivers/gpu/drm/amd/amdgpu/atombios_encoders.c also already
> uses the drm_connector as parent, only amdgpu_dm.c used the PCI device
> as parent before this change.
>
> Changes in v2:
> Together with changing the parent, also move the registration to
> drm_connector_funcs.late_register() this is necessary because the parent
> device (which now is the drm_connector) must be registered before
> the backlight class device is, otherwise the backlight class device ends
> up without any parent set at all.
>
> This brings the backlight class device registration timing inline with
> nouveau and i915 which also use drm_connector_funcs.late_register()
> for this.
>
> Note this slightly changes backlight_device_register() error handling,
> instead of not increasing dm->num_of_edps and re-using the current
> bl_idx for a potential other backlight device, dm->backlight_dev[bl_idx]
> is now simply left NULL on failure. This is ok because all code
> looking at dm->backlight_dev[i] also checks it is not NULL.
>
> Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730
> Signed-off-by: Hans de Goede
Self nack, the amdgpu_dm_register_backlight_device() call in
amdgpu_dm_connector_late_register() leads to the driver now
trying to register a backlight class device for each connector
(I hit this on my AMD APU based desktop machine).
I'll prepare a non RFC version with this fixed.
Regards,
Hans
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 038bf897cc28..051074d5812f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4162,7 +4162,7 @@ amdgpu_dm_register_backlight_device(struct
> amdgpu_dm_connector *aconnector)
>drm->primary->index + aconnector->bl_idx);
>
> dm->backlight_dev[aconnector->bl_idx] =
> - backlight_device_register(bl_name, drm->dev, dm,
> + backlight_device_register(bl_name, aconnector->base.kdev, dm,
> _dm_backlight_ops, );
>
> if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
> @@ -4232,13 +4232,6 @@ static void setup_backlight_device(struct
> amdgpu_display_manager *dm,
>
> amdgpu_dm_update_backlight_caps(dm, bl_idx);
> dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
> -
> - amdgpu_dm_register_backlight_device(aconnector);
> - if (!dm->backlight_dev[bl_idx]) {
> - aconnector->bl_idx = -1;
> - return;
> - }
> -
> dm->backlight_link[bl_idx] = link;
> dm->num_of_edps++;
>
> @@ -6297,6 +6290,8 @@ amdgpu_dm_connector_late_register(struct drm_connector
> *connector)
> to_amdgpu_dm_connector(connector);
> int r;
>
> + amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
> +
> if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
> (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
> amdgpu_dm_connector->dm_dp_aux.aux.dev = connector->kdev;
Re: [RFC v2 0/6] drm/amd/display: Pass proper parent for DM backlight device v2
Hi Rodrigo, On 3/10/23 23:12, Rodrigo Siqueira Jordao wrote: > Hi Hans, > > Which AMD device do you have available for testing this series? As mentioned in a reply to the cover-letter (should have been in the cover-letter itself but I forgot, sorry. I don't have any hw to test this which is why this was marked as a RFC. In the mean time 2 reporters of: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 who have affected hw hitting the changed code paths have confirmed that this series works and that the correct parent now gets set. So as I also already mentioned in a reply to the cover-letter (1): this series no longer is RFC, but is ready for merging (from my pov) now. > P.s.: If you have a new version of this series, could you also Cc me? Sure, although atm I see no need to do a new version, please consider this a non RFC submission now and review it. If the review leads to changes being requested then I'll prepare a new version and Cc you. Regards, Hans 1) Next time mayvw read the entire thread before replying ? > On 3/8/23 14:58, Hans de Goede wrote: >> Hi All, >> >> Here is version 2 of my patch series to pass the proper parent device >> to backlight_device_register(). >> >> New in version 2 is delaying the registering of the backlight_dev till >> after the drm_connector is registered by doing it from >> drm_connector_funcs.late_register. >> >> This involves first reworking the code a bit to allow delaying >> the registering, so this has turned from a single patch into >> a 6 patch set. >> >> Regards, >> >> Hans >> >> >> Hans de Goede (6): >> drm/amd/display/amdgpu_dm: Fix backlight_device_register() error >> handling >> drm/amd/display/amdgpu_dm: Refactor register_backlight_device() >> drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector >> drm/amd/display/amdgpu_dm: Move most backlight setup into >> setup_backlight_device() >> drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() >> take an amdgpu_dm_connector >> drm/amd/display: Pass proper parent for DM backlight device >> registration v2 >> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + >> 2 files changed, 44 insertions(+), 56 deletions(-) >> >
Re: [RFC v2 0/6] drm/amd/display: Pass proper parent for DM backlight device v2
Hi all, On 3/8/23 23:10, Hans de Goede wrote: > Hi, > > On 3/8/23 22:58, Hans de Goede wrote: >> Hi All, >> >> Here is version 2 of my patch series to pass the proper parent device >> to backlight_device_register(). >> >> New in version 2 is delaying the registering of the backlight_dev till >> after the drm_connector is registered by doing it from >> drm_connector_funcs.late_register. >> >> This involves first reworking the code a bit to allow delaying >> the registering, so this has turned from a single patch into >> a 6 patch set. >> >> Regards, >> >> Hans > > p.s. > > Like last time this series is marked as RFC because I don't have hw > to test the fix myself. The previous version was tested by 2 reporters > of: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 > > I hope to get test results from them for this new version soon. I just heard back from one of the reporters that this fixes gnome-settings-daemon picking the wrong backlight device on a hybrid gfx laptop where both GPU-s register a native backlight control. So this series no longer is RFC, but is ready for merging (from my pov) now. Regards, Hans >> Hans de Goede (6): >> drm/amd/display/amdgpu_dm: Fix backlight_device_register() error >> handling >> drm/amd/display/amdgpu_dm: Refactor register_backlight_device() >> drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector >> drm/amd/display/amdgpu_dm: Move most backlight setup into >> setup_backlight_device() >> drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() >> take an amdgpu_dm_connector >> drm/amd/display: Pass proper parent for DM backlight device >> registration v2 >> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + >> 2 files changed, 44 insertions(+), 56 deletions(-) >> >
