Re: [Intel-gfx] [bisected] suspend broken by DRM fbdev name change on i915 IVB
Hello, On 10/7/21 14:38, Ville Syrjälä wrote: [snip] >> >> So the ABI change in /proc/fb causes the pm-utils scripts to >> skip the --quirk-no-chvt and apply other quirks, >> /var/log/pm-suspend.log says: > > Nasty. This pm-utils quirk stuff really has no business existing IMO, > and so I recommend nuking pm-utils from your system as soon as possible. > Back when I still had it on my machines (due to some silly dependency > I think), I just created empty override files in /etc/pm/ to permanently > disable all the quirks. > > But as long people might be using it I guess we need some kind of > revert/fix to put the "drmfb" back into the name. Javier? > Yes, the change was just cosmetic because we had confusing names such as "simpledrmdrmfb". When it was proposed, the agreement was that /proc/fb shouldn't be considered an ABI but now we found that people are using it. So I agree that would be better to revert this patch. Johannes, will you post a revert or do you want me to do it ? Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 3/5] drm/dp: Move DisplayPort helpers into separate helper module
On Wed, Dec 15, 2021 at 12:12 PM Thomas Zimmermann wrote: > > Hi > > Am 15.12.21 um 12:04 schrieb Jani Nikula: > > On Wed, 15 Dec 2021, Thomas Zimmermann wrote: > >> * move DP helper code into dp/ (Jani) > > > > I suggested adding the subdirectory, but I'm going to bikeshed the name, > > which I didn't suggest. > > > > $ find drivers/gpu/drm -mindepth 1 -maxdepth 1 -type d | wc -l > > 68 > > > > Assuming we move more of the drm modules to subdirectories, how are they > > going to stand out from drivers? > > > > I suggested drm_dp, which I understand results in tautology, but hey, > > all the filenames under drm/ also have drm_*.[ch]. And I find that very > > useful for git greps and other code archeology. With just the dp name, > > you'd have to know and list all the drm subdirectories when looking up > > stuff that's part of drm but not drivers. > > I think we have enough filename prefixes already. drm/drm_dp/drm_dp_ is > just ridiculous. > Maybe what can be done is to just add a drivers/gpu/drm/core subdirectory that would contain all the DRM core code ? Then the dp helpers could be moved to drivers/gpu/drm/core/dp/drm_dp.c for example. This would also make easy to differentiate the drm modules from the drivers with just: $ find drivers/gpu/drm -mindepth 1 -maxdepth 1 -type d -not -name core Best regards, Javier
[Intel-gfx] [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
than .remove") attempted to fix a use-after-free error due driver freeing
the fb_info in the .remove handler instead of doing it in .fb_destroy.
But ironically that change introduced yet another use-after-free since the
fb_info was still used after the free.
This should fix for good by freeing the fb_info at the end of the handler.
Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than
.remove")
Reported-by: Ville Syrjälä
Reported-by: Andrzej Hajda
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/efifb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index cfa3dc0b4eee..b3d5f884c544 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -259,12 +259,12 @@ static void efifb_destroy(struct fb_info *info)
memunmap(info->screen_base);
}
- framebuffer_release(info);
-
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
fb_dealloc_cmap(&info->cmap);
+
+ framebuffer_release(info);
}
static const struct fb_ops efifb_ops = {
--
2.35.1
Re: [Intel-gfx] [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
Hello Lucas,
On 5/7/22 18:20, Lucas De Marchi wrote:
> On Fri, May 06, 2022 at 03:22:25PM +0200, Javier Martinez Canillas wrote:
>> Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
>> than .remove") attempted to fix a use-after-free error due driver freeing
>> the fb_info in the .remove handler instead of doing it in .fb_destroy.
>>
>> But ironically that change introduced yet another use-after-free since the
>> fb_info was still used after the free.
>>
>> This should fix for good by freeing the fb_info at the end of the handler.
>>
>> Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
>> than .remove")
>
> are these patches going through any CI before being applied? Maybe would
> be a good idea to cc intel-gfx mailing list on these fixes to have Intel
> CI to pick them up for some tests?
>
I Cc'ed intel-gfx for this particular patch. I should had done it for the
previous patches too, but I wasn't aware that Cc'ing that list would make
it run on your CI.
I tested locally the offending patch on an EFI platform before applying it
and I don't know why it didn't fail there. Sorry all for the inconvenience.
> pushed to drm-misc-fixes where the previous patch was applied.
>
Thanks.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] linux-next: build failure after merge of the drm-misc tree
Hello Stephen,
On 6/10/22 06:49, Stephen Rothwell wrote:
> Hi all,
>
> After merging the drm-misc tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> drivers/firmware/efi/sysfb_efi.c:29:10: fatal error: asm/efi.h: No such file
> or directory
>29 | #include
> | ^~~
>
I noticed that this header include is not necessary so I posted [0] to
just drop it, and mentioned the build error too with your Reported-by.
> Caused by commit
>
> fa0e256450f2 ("fbdev: vesafb: Allow to be built if COMPILE_TEST is enabled")
>
I posted a revert [1] for this but for a different reason (since after
[0] I believe the issue in powerpc should be fixed), which is that the
patch led to linking errors on arches that don't define a screen_info.
[0]: https://lkml.org/lkml/2022/6/10/323
[1]: https://lkml.org/lkml/2022/6/10/316
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
Hello Daniel,
On 4/5/22 10:40, Daniel Vetter wrote:
> On Tue, Apr 05, 2022 at 10:36:35AM +0200, Daniel Vetter wrote:
>> On Wed, Feb 09, 2022 at 01:19:26AM +0100, Javier Martinez Canillas wrote:
>>> On 2/8/22 22:08, Daniel Vetter wrote:
>>>> This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
>>>>
>>>> With
>>>>
>>>> commit 27599aacbaefcbf2af7b06b0029459bbf682000d
>>>> Author: Thomas Zimmermann
>>>> Date: Tue Jan 25 10:12:18 2022 +0100
>>>>
>>>> fbdev: Hot-unplug firmware fb devices on forced removal
>>>>
>>>> this should be fixed properly and we can remove this somewhat hackish
>>>> check here (e.g. this won't catch drm drivers if fbdev emulation isn't
>>>> enabled).
>>>>
>>>
>>> Unfortunately this hack can't be reverted yet. Thomas' patch solves the
>>> issue
>>> of platform devices matched with fbdev drivers to be properly unregistered
>>> if
>>> a DRM driver attempts to remove all the conflicting framebuffers.
>>>
>>> But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers
>>> if
>>> a FB is already registered") worked around is different. It happens when the
>>> DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
>>> kicking out of conflicting framebuffers already happened and these drivers
>>> will be allowed to probe even when a DRM driver is already present.
>>>
>>> We need a clearer way to prevent it, but can't revert fb561bf9abde until
>>> that.
>>
>> Yeah that entire area is a mess still, ideally we'd have something else
>> creating the platform devices, and efifb/offb and all these would just
>> bind against them.
>>
>> Hm one idea that just crossed my mind: Could we have a flag in fb_info for
>> fw drivers, and check this in framebuffer_register? Then at least all the
>> logic would be in the fbdev core.
>
I can't answer right away since I've since forgotten this part of the code
and will require to do a detailed read to refresh my memory.
I'll answer later but preferred to mention the other question ASAP.
> Ok coffee just kicked in, how exactly does your scenario work?
>
> This code I'm reverting here is in the platform_dev->probe function.
> Thomas' patch removes the platform_dev. How exactly can you still probe
> against a platform dev if that platform dev is gone?
>
Because the platform was not even registered by the time the DRM driver
probed and all the devices for the conflicting drivers were unregistered.
> Iow, now that I reponder your case after a few weeks I'm no longer sure
> things work like you claim.
>
This is how I think that work, please let me know if you see something
wrong in my logic:
1) A PCI device of OF device is registered for the GPU, this attempt to
match a registered driver but no driver was registered that match yet.
2) The efifb driver is built-in, will be initialized according to the link
order of the objects under drivers/video and the fbdev driver is registered.
There is no platform device or PCI/OF device registered that matches.
3) The DRM driver is built-in, will be initialized according to the link
order of the objects under drivers/gpu and the DRM driver is registered.
This matches the device registered in (1) and the DRM driver probes.
4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
before registering the DRM device.
There are no conflicting drivers or platform device at this point.
5) Latter at some point the drivers/firmware/sysfb.c init function is
executed, and this registers a platform device for the generic fb.
This device matches the efifb driver registered in (2) and the fbdev
driver probes.
Since that happens *after* the DRM driver already matched, probed
and registered the DRM device, that is a bug and what the reverted
patch worked around.
So we need to prevent (5) if (1) and (3) already happened. Having a flag
set in the fbdev core somewhere when remove_conflicting_framebuffers()
is called could be a solution indeed.
That is, the fbdev core needs to know that a DRM driver already probed
and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
I can attempt to write a patch for that.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
On 4/5/22 11:24, Daniel Vetter wrote: > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas [snip] >> >> This is how I think that work, please let me know if you see something >> wrong in my logic: >> >> 1) A PCI device of OF device is registered for the GPU, this attempt to >>match a registered driver but no driver was registered that match yet. >> >> 2) The efifb driver is built-in, will be initialized according to the link >>order of the objects under drivers/video and the fbdev driver is >> registered. >> >>There is no platform device or PCI/OF device registered that matches. >> >> 3) The DRM driver is built-in, will be initialized according to the link >>order of the objects under drivers/gpu and the DRM driver is registered. >> >>This matches the device registered in (1) and the DRM driver probes. >> >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev >>before registering the DRM device. >> >>There are no conflicting drivers or platform device at this point. >> >> 5) Latter at some point the drivers/firmware/sysfb.c init function is >>executed, and this registers a platform device for the generic fb. >> >>This device matches the efifb driver registered in (2) and the fbdev >>driver probes. >> >>Since that happens *after* the DRM driver already matched, probed >>and registered the DRM device, that is a bug and what the reverted >>patch worked around. >> >> So we need to prevent (5) if (1) and (3) already happened. Having a flag >> set in the fbdev core somewhere when remove_conflicting_framebuffers() >> is called could be a solution indeed. >> >> That is, the fbdev core needs to know that a DRM driver already probed >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE >> >> I can attempt to write a patch for that. > > Ah yeah that could be an issue. I think the right fix is to replace > the platform dev unregister with a sysfb_unregister() function in > sysfb.c, which is synced with a common lock with the sysfb_init > function and a small boolean. I think I can type that up quickly for > v3. It's more complicated than that since sysfb is just *one* of the several places where platform devices can be registered for video devices. For instance, the vga16fb driver registers its own platform device in its module_init() function so that can also happen after the conflicting framebuffers (and associated devices) were removed by a DRM driver probe. I tried to minimize the issue for that particular driver with commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f But the point stands, it all boils down to the fact that you have two different subsystems registering video drivers and they don't know all about each other to take a proper decision. Right now the drm_aperture_remove_conflicting_framebuffers() call signals in one direction from DRM to fbdev but there isn't a communication in the other direction, from fbdev to DRM. I believe the correct fix would be for the fbdev core to keep a list of the apertures struct that are passed to remove_conflicting_framebuffers(), that way it will know what apertures are not available anymore and prevent to register any fbdev framebuffer that conflicts with one already present. Let me know if you think that makes sense and I can attempt to write a fix. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
On 4/5/22 12:34, Daniel Vetter wrote:
> On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> wrote:
[snip]
>>
>> I believe the correct fix would be for the fbdev core to keep a list of
>> the apertures struct that are passed to remove_conflicting_framebuffers(),
>> that way it will know what apertures are not available anymore and prevent
>> to register any fbdev framebuffer that conflicts with one already present.
>
> Hm that still feels like reinventing a driver model, badly.
>
Yeah, you are correct.
> I think there's two cleaner solutions:
> - move all the firmware driver platform_dev into sysfb.c, and then
> just bind the special cases against that (e.g. offb, vga16fb and all
> these). Then we'd have one sysfb_try_unregister(struct device *dev)
> interface that fbmem.c uses.
I think this is the cleaner option. And makes sense to consolidate all
the firmware drivers platform device registration to sysfb.c.
Already does for VIDEO_TYPE_EFI ("efi-framebuffer") and VIDEO_TYPE_VLFB
("vesa-framebuffer"), so need to also make it cope with VIDEO_TYPE_EGAC
and VIDEO_TYPE_VGAC ("vga16fb").
For offb is less clear since currently the offb driver does not really
use the Linux device model, that is the driver does not match a device
that's registered, there's no device which is the bug that was reported
to Thomas in the other thread.
It's unclear how to properly fix that since we will need to convert the
offb driver to register a platform driver and match against a device that
is registered by some platform code that parses the OF...
> - let fbmem.c call into each of these firmware device providers, which
> means some loops most likely (like we can't call into vga16fb), so
> probably need to move that into fbmem.c and it all gets a bit messy.
>
Yup, that would get messy indeed so not a good option.
>> Let me know if you think that makes sense and I can attempt to write a fix.
>
> I still think unregistering the platform_dev properly makes the most
> sense, and feels like the most proper linux device model solution
> instead of hacks on top - if the firmware fb is unuseable because a
> native driver has taken over, we should nuke that. And also the
> firmware fb driver would then just bind to that platform_dev if it
> exists, and only if it exists. Also I think it should be the
> responsibility of whichever piece of code that registers these
> platform devices to ensure that platform_dev actually still exists.
> That's why I think pushing all that code into sysfb.c is probably the
> cleanest solution.
>
Agreed. Not registering the platform devices if there is already a DRM
driver for the same device is what makes the most sense. What I don't
understand is how sysfb would know that if run after a DRM registration.
The only way that could know is if sysfb would keep a list of apertures
for all the DRM drivers registered or if the DRM core somewhat notifies
to sysfb that a native driver was already registered.
Another option and probably the cleanest although the harder solution is
to finally bite the bullet and make all the DRM drivers to request their
memory region.
Or as you mentioned in the past, to move that logic into the device model
and then not allow to register devices that require an overlapping region.
And there could be a request_mem_region_remove_conflicting() or something
that real DRM drivers could use to force a memory region request and make
the device model to unregister any device that may already have that mem.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH 02/15] dma-buf: specify usage while adding fences to dma_resv obj v7
Hello Christian,
On 4/7/22 10:59, Christian König wrote:
> Instead of distingting between shared and exclusive fences specify
> the fence usage while adding fences.
>
> Rework all drivers to use this interface instead and deprecate the old one.
>
This patch broke compilation for the vc4 DRM driver. I've this patch locally
which seems to work but I don't know enough about the fence API to know if
is correct.
If you think is the proper fix then I can post it as a patch.
>From 3e96db4827ef69b38927476659cbb4469a0246e6 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Thu, 7 Apr 2022 14:54:07 +0200
Subject: [PATCH] drm/vc4: Use newer fence API to fix build error
The commit 73511edf8b19 ("dma-buf: specify usage while adding fences to
dma_resv obj v7") ported all the DRM drivers to use the newer fence API
that specifies the usage with the enum dma_resv_usage rather than doing
an explicit shared / exclusive distinction.
But the commit didn't do it properly in two callers of the vc4 driver,
leading to build errors.
Fixes: 73511edf8b19 ("dma-buf: specify usage while adding fences to dma_resv
obj v7")
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/vc4/vc4_gem.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 38550317e025..9eaf304fc20d 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -546,7 +546,8 @@ vc4_update_bo_seqnos(struct vc4_exec_info *exec, uint64_t
seqno)
bo = to_vc4_bo(&exec->bo[i]->base);
bo->seqno = seqno;
- dma_resv_add_fence(bo->base.base.resv, exec->fence);
+ dma_resv_add_fence(bo->base.base.resv, exec->fence,
+ DMA_RESV_USAGE_READ);
}
list_for_each_entry(bo, &exec->unref_list, unref_head) {
@@ -557,7 +558,8 @@ vc4_update_bo_seqnos(struct vc4_exec_info *exec, uint64_t
seqno)
bo = to_vc4_bo(&exec->rcl_write_bo[i]->base);
bo->write_seqno = seqno;
- dma_resv_add_excl_fence(bo->base.base.resv, exec->fence);
+ dma_resv_add_fence(bo->base.base.resv, exec->fence,
+ DMA_RESV_USAGE_WRITE);
}
}
--
2.35.1
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH 02/15] dma-buf: specify usage while adding fences to dma_resv obj v7
On 4/7/22 15:13, Christian König wrote: > Am 07.04.22 um 15:08 schrieb Javier Martinez Canillas: >> Hello Christian, >> >> On 4/7/22 10:59, Christian König wrote: >>> Instead of distingting between shared and exclusive fences specify >>> the fence usage while adding fences. >>> >>> Rework all drivers to use this interface instead and deprecate the old one. >>> >> This patch broke compilation for the vc4 DRM driver. > > My apologies for that. I've tried really hard to catch all cases, but > looks like I missed some. > No worries, I know that's not easy to get all callers when doing these subsystem wide changes. >> I've this patch locally >> which seems to work but I don't know enough about the fence API to know if >> is correct. >> >> If you think is the proper fix then I can post it as a patch. > > Yes, that patch looks absolutely correct to me. > Thanks for looking at it. > Feel free to add an Reviewed-by: Christian König > and CC me so that I can push it to > drm-misc-next ASAP. > I can also do it after posting (just to get a proper Link: tag with dim) Already have another set that wanted to push but found this issue after doing a build test before pushing. > Thanks, > Christian. > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH] fbcon: Fix delayed takeover locking
Hello Daniel,
On 4/13/22 10:21, Daniel Vetter wrote:
> I messed up the delayed takover path in the locking conversion in
> 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister").
>
Maybe a few more words of what the issue is ? Something like the following:
If CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER is enabled, fbcon take-over
doesn't take place when calling fbcon_fb_registered(). Instead, is deferred
using a workqueue and its fbcon_register_existing_fbs() function calls to
fbcon_fb_registered() again for each registered fbcon fb.
This leads to the console_lock tried to be held twice, causing a deadlock.
> Fix it by re-extracting the lockless function and using it in the
> delayed takeover path, where we need to hold the lock already to
> iterate over the list of already registered fb. Well the current code
> still is broken in there (since the list is protected by a
> registration_lock, which we can't take here because it nests the other
> way round with console_lock), but in the future this will be a list
> protected by console_lock when this is all sorted out.
>
[snip]
>
> -/* called with console_lock held */
> void fbcon_fb_unbind(struct fb_info *info)
> {
> int i, new_idx = -1;
> @@ -2822,7 +2821,6 @@ void fbcon_fb_unbind(struct fb_info *info)
> console_unlock();
> }
>
> -/* called with console_lock held */
> void fbcon_fb_unregistered(struct fb_info *info)
> {
Removing these comments feels like should be in a separate patch or at least
mention in the patch description that should had been removed in the commit
6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister"),
that made these functions to be called without the console_lock being held.
The changes themselves look good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH] fbcon: Fix delayed takeover locking
On 4/13/22 11:21, Daniel Vetter wrote:
> On Wed, Apr 13, 2022 at 11:16:08AM +0200, Javier Martinez Canillas wrote:
>> Hello Daniel,
>>
>> On 4/13/22 10:21, Daniel Vetter wrote:
>>> I messed up the delayed takover path in the locking conversion in
>>> 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister").
>>>
>>
>> Maybe a few more words of what the issue is ? Something like the following:
>>
>> If CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER is enabled, fbcon take-over
>> doesn't take place when calling fbcon_fb_registered(). Instead, is deferred
>> using a workqueue and its fbcon_register_existing_fbs() function calls to
>> fbcon_fb_registered() again for each registered fbcon fb.
>>
>> This leads to the console_lock tried to be held twice, causing a deadlock.
>
> Will add.
>>
Thanks.
[snip]
>> Removing these comments feels like should be in a separate patch or at least
>> mention in the patch description that should had been removed in the commit
>> 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister"),
>> that made these functions to be called without the console_lock being held.
>
> Yeah I forgot to mention that in the commit message - while reviewing all
> the locking to figure out the bug I also noticed that I didn't update the
> comments, and since it's all issues in the same patch I figured I'll do it
> all in one go.
>
> Ok if I explain that and then keep the comment removals?
Yes, I'm OK with that and agreed that all changes are to fix the same commit.
It's just that it took me some time to figure out why you were removing those.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[Intel-gfx] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
[ resend with all relevant people as Cc now, sorry to others for the spam ] There is a lot of historical baggage on this parameter. It's defined in the vgacon driver as a "nomodeset" parameter, but it's handler function is called text_mode() that sets a variable named vgacon_text_mode_force whose value is queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver could be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. There is also another aspect that could be improved, and is the fact that drivers are checking for the nomodeset being set as an indication if have to be loaded. But there may be other reasons why this could be the case, so it is better to encapsulate the logic in a separate function to make clear what's about. Patch #1 is just a trivial fix for a comment that isn't referring to the correct kernel parameter. Patch #2 moves the nomodeset logic to the DRM subsystem. Patch #3 renames the vgacon_text_force() function and accompaning logic as drm_modeset_disabled(), which is what this function is really about. Patch #4 adds a drm_drv_enabled() function that could be used by drivers to check if could be enabled. Patch #5 uses the drm_drv_enabled() function to check this instead of just checking if nomodeset has been set. Javier Martinez Canillas (5): drm/i915: Fix comment about modeset parameters drm: Move nomodeset kernel parameter handler to the DRM subsystem drm: Rename vgacon_text_force() function to drm_modeset_disabled() drm: Add a drm_drv_enabled() helper function drm: Use drm_drv_enabled() instead of drm_modeset_disabled() drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 ++--- drivers/gpu/drm/ast/ast_drv.c | 3 +-- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 10 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- drivers/gpu/drm/qxl/qxl_drv.c | 3 +-- drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- drivers/gpu/drm/tiny/bochs.c| 3 +-- drivers/gpu/drm/tiny/cirrus.c | 3 +-- drivers/gpu/drm/vboxvideo/vbox_drv.c| 5 + drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-- drivers/video/console/vgacon.c | 21 include/drm/drm_drv.h | 1 + include/drm/drm_mode_config.h | 6 ++ include/linux/console.h | 6 -- 19 files changed, 73 insertions(+), 57 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
[Intel-gfx] [RESEND PATCH 1/5] drm/i915: Fix comment about modeset parameters
The comment mentions that the KMS is enabled by default unless either the i915.modeset module parameter or vga_text_mode_force boot option are used. But the latter does not exist and instead the nomodeset option was meant. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/i915/i915_module.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c index ab2295dd4500..c7507266aa83 100644 --- a/drivers/gpu/drm/i915/i915_module.c +++ b/drivers/gpu/drm/i915/i915_module.c @@ -24,8 +24,8 @@ static int i915_check_nomodeset(void) /* * Enable KMS by default, unless explicitly overriden by -* either the i915.modeset prarameter or by the -* vga_text_mode_force boot option. +* either the i915.modeset parameter or by the +* nomodeset boot option. */ if (i915_modparams.modeset == 0) -- 2.33.1
[Intel-gfx] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.
But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.
Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.
Suggested-by: Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/drm_nomodeset.c | 16
drivers/gpu/drm/i915/i915_module.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
drivers/gpu/drm/tiny/bochs.c| 2 +-
drivers/gpu/drm/tiny/cirrus.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c| 4 +---
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
include/drm/drm_mode_config.h | 4 ++--
14 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
+ if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
+ if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
if (ast_modeset == 0)
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
#include
#include
-static bool vgacon_text_mode_force;
+static bool drm_nomodeset;
-bool vgacon_text_force(void)
+bool drm_modeset_disabled(void)
{
- return vgacon_text_mode_force;
+ return drm_nomodeset;
}
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
-static int __init text_mode(char *str)
+static int __init disable_modeset(char *str)
{
- vgacon_text_mode_force = true;
+ drm_nomodeset = true;
pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
}
-/* force text mode - used by kernel modesetting */
-__setup("nomodeset", text_mode);
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
- if (vgacon_text_force() && i915_modparams.modeset == -1)
+ if (drm_modeset_disabled() && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
static int __init mgag200_init(void)
{
- if (vgacon_text_force() && mgag200_modeset == -1)
+ if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
if (mgag200_modeset == 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 029997f50d1a..903d0e626954 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1321,7 +1321,7 @@ nouveau_drm_init(void)
nouveau_display_options();
if (nouveau_modeset == -1) {
- if (vgacon_text_force())
+ if (drm_modeset_disab
[Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it. Let's move that to DRM.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
17 files changed, 35 insertions(+), 41 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..0e2d60ea93ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-y += drm_nomodeset.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..2680a2aaa877 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
@@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
int r;
if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+ DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..048be607b182 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..1ac9a8d5a8fe
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool vgacon_text_mode_force;
+
+bool vgacon_text_force(void)
+{
+ return vgacon_text_mode_force;
+}
+EXPORT_SYMBOL(vgacon_text_force);
+
+static int __init text_mode(char *str)
+{
+ vgacon_text_mode_force = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+
+ return 1;
+}
+
+/* force text mode - used by kernel modesetting */
+__setup("nomodeset", text_mode);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c7507266aa83..14a59226519d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
* Copyright © 2021 Intel Corporation
*/
-#include
-
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_object.h"
#include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..685e766db6a4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -6,7 +6,6 @@
* Dave Airlie
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..029997f50d1a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,7 +22,6 @@
* Authors: Ben Skeggs
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index fc47b0deb021..3cd6bd9f059d 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -29,7 +29,6 @@
#include "qxl_
Re: [Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
Hello Thomas, On 11/3/21 13:41, Thomas Zimmermann wrote: > Hi > > Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas: >> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver >> but the exported vgacon_text_force() symbol is only used by DRM drivers. >> >> It makes much more sense for the parameter logic to be in the subsystem >> of the drivers that are making use of it. Let's move that to DRM. >> >> Suggested-by: Daniel Vetter >> Signed-off-by: Javier Martinez Canillas >> --- >> >> drivers/gpu/drm/Makefile| 2 ++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +-- >> drivers/gpu/drm/ast/ast_drv.c | 1 - >> drivers/gpu/drm/drm_nomodeset.c | 26 + >> drivers/gpu/drm/i915/i915_module.c | 2 -- >> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 - >> drivers/gpu/drm/nouveau/nouveau_drm.c | 1 - >> drivers/gpu/drm/qxl/qxl_drv.c | 1 - >> drivers/gpu/drm/radeon/radeon_drv.c | 1 - >> drivers/gpu/drm/tiny/bochs.c| 1 - >> drivers/gpu/drm/tiny/cirrus.c | 1 - >> drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 - >> drivers/gpu/drm/virtio/virtgpu_drv.c| 1 - >> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 - >> drivers/video/console/vgacon.c | 21 >> include/drm/drm_mode_config.h | 6 ++ >> include/linux/console.h | 6 -- >> 17 files changed, 35 insertions(+), 41 deletions(-) >> create mode 100644 drivers/gpu/drm/drm_nomodeset.c >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 1c41156deb5f..0e2d60ea93ca 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o >> drm_privacy_screen_x86. >> >> obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o >> >> +obj-y += drm_nomodeset.o > > Repeating my other comment, should this rather be protected by a > separate config symbol that is selected by CONFIG_DRM? > I actually thought about that and my opinion is that obj-y reflects what we really want here or do you envision this getting disabled in some cases ? Probably the problem is Kbuild descending into the drivers/gpu dir even when CONFIG_DRM is not set. Maybe what we want is something like the following instead? diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile index 835c88318cec..ef12ee05ba6e 100644 --- a/drivers/gpu/Makefile +++ b/drivers/gpu/Makefile @@ -3,6 +3,7 @@ # taken to initialize them in the correct order. Link order is the only way # to ensure this currently. obj-$(CONFIG_TEGRA_HOST1X) += host1x/ -obj-y += drm/ vga/ +obj-$(CONFIG_DRM) += drm/ +obj-y += vga/ obj-$(CONFIG_IMX_IPUV3_CORE) += ipu-v3/ obj-$(CONFIG_TRACE_GPU_MEM)+= trace/ Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
Hello Jani,
On 11/3/21 13:56, Jani Nikula wrote:
[snip]
>>
>> +obj-y += drm_nomodeset.o
>
> This is a subtle functional change. With this, you'll always have
> __setup("nomodeset", text_mode) builtin and the parameter available. And
> using nomodeset will print out the pr_warn() splat from text_mode(). But
> removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
> leads to vgacon_text_force() always returning false.
>
Yes, that's what I decided at the end to make it unconditional. That
way the same behaviour is preserved (even when only DRM drivers are
using the exported symbol).
> To not make functional changes, this should be:
>
> obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>
Right, that should work.
> Now, going with the cleanup in this series, maybe we should make the
> functional change, and break the connection to CONFIG_VGA_CONSOLE
> altogether, also in the header?
>
> (Maybe we'll also need a proxy drm kconfig option to only have
> drm_modeset.o builtin when CONFIG_DRM != n.)
>
See my other email. I believe the issue is drivers/gpu/drm always
being included even when CONFIG_DRM is not set.
Best regards, --
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
On 11/3/21 13:57, Thomas Zimmermann wrote:
[snip]
>>
>> -if (vgacon_text_force()) {
>> +if (drm_modeset_disabled()) {
>> DRM_ERROR("amdgpu kernel modesetting disabled.\n");
>
> Please remove all such error messages from drivers.
> drm_modeset_disabled() should print a unified message instead.
>
Agreed.
>> -static bool vgacon_text_mode_force;
>> +static bool drm_nomodeset;
>>
>> -bool vgacon_text_force(void)
>> +bool drm_modeset_disabled(void)
>
> I suggest to rename this function to drm_check_modeset() and have it
> return a negative errno code on failure. This gives maximum flexibility
> and reduces errors in drivers. Right now the drivers return something
> like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.
>
Good idea. I'll do it in v2 as well.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
Hello Thomas, On 11/3/21 14:01, Thomas Zimmermann wrote: [snip] >> >> >> Javier Martinez Canillas (5): >>drm/i915: Fix comment about modeset parameters >>drm: Move nomodeset kernel parameter handler to the DRM subsystem >>drm: Rename vgacon_text_force() function to drm_modeset_disabled() >>drm: Add a drm_drv_enabled() helper function >>drm: Use drm_drv_enabled() instead of drm_modeset_disabled() > > There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And > I'd put patch (4+5) first, so you have the drivers out of the way. After > that patch (2+3) should only modify drm_drv_enabled(). > Sure, I'm happy with less patches. Thanks for your feedback. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[Intel-gfx] [PATCH v2 0/2] Cleanups for the nomodeset kernel command line parameter logic
There is a lot of historical baggage on this parameter. It is defined in the vgacon driver as nomodeset, but its set function is called text_mode() and the value queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver should be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. There is also another aspect that could be improved, and is the fact that drivers are checking for the nomodeset being set as an indication if have to be loaded. But there may be other reasons why this could be the case, so it is better to encapsulate the logic in a separate function to make clear what's about. This is a v2 of the patches, that address the issues pointed out by Thomas Zimmermann and Jani Nikula in v1: https://lore.kernel.org/lkml/5b4e4534-4786-d231-e331-78fdb5d84...@redhat.com/T/ Patch #1 adds a drm_drv_enabled() function that could be used by drivers to check if these could be enabled, instead of just using vgacon_text_force(). Patch #2 moves the nomodeset logic to the DRM subsystem and renames the functions and variables to better explain what these actually do. Changes in v2: - Squash patch to add drm_drv_enabled() and make drivers use it. - Make the drivers changes before moving nomodeset logic to DRM. - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset. - Remove debug and error messages in drivers. - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set. - Squash patches to move nomodeset logic to DRM and do the renaming. - Name the function drm_check_modeset() and make it return -ENODEV. Javier Martinez Canillas (2): drm: Add a drm_drv_enabled() to check if drivers should be enabled drm: Move nomodeset kernel parameter to the DRM subsystem drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 +++- drivers/gpu/drm/ast/ast_drv.c | 8 +--- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 8 +--- drivers/gpu/drm/mgag200/mgag200_drv.c | 8 +--- drivers/gpu/drm/nouveau/nouveau_drm.c | 6 -- drivers/gpu/drm/qxl/qxl_drv.c | 8 +--- drivers/gpu/drm/radeon/radeon_drv.c | 7 --- drivers/gpu/drm/tiny/bochs.c| 8 +--- drivers/gpu/drm/tiny/cirrus.c | 9 ++--- drivers/gpu/drm/vboxvideo/vbox_drv.c| 10 +- drivers/gpu/drm/virtio/virtgpu_drv.c| 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 +++--- drivers/video/console/vgacon.c | 21 include/drm/drm_drv.h | 1 + include/drm/drm_mode_config.h | 6 ++ include/linux/console.h | 6 -- 19 files changed, 109 insertions(+), 66 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
[Intel-gfx] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Some DRM drivers check the vgacon_text_force() function return value as an
indication on whether they should be allowed to be enabled or not.
This function returns true if the nomodeset kernel command line parameter
was set. But there may be other conditions besides this to determine if a
driver should be enabled.
Let's add a drm_drv_enabled() helper function to encapsulate that logic so
can be later extended if needed, without having to modify all the drivers.
Also, while being there do some cleanup. The vgacon_text_force() function
is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Squash patch to add drm_drv_enabled() and make drivers use it.
- Make the drivers changes before moving nomodeset logic to DRM.
- Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
- Remove debug and error messages in drivers.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++
drivers/gpu/drm/ast/ast_drv.c | 7 +--
drivers/gpu/drm/drm_drv.c | 20
drivers/gpu/drm/i915/i915_module.c | 6 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +--
drivers/gpu/drm/nouveau/nouveau_drm.c | 5 -
drivers/gpu/drm/qxl/qxl_drv.c | 7 +--
drivers/gpu/drm/radeon/radeon_drv.c | 6 --
drivers/gpu/drm/tiny/bochs.c| 7 +--
drivers/gpu/drm/tiny/cirrus.c | 8 ++--
drivers/gpu/drm/vboxvideo/vbox_drv.c| 9 +
drivers/gpu/drm/virtio/virtgpu_drv.c| 5 +++--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +++--
include/drm/drm_drv.h | 1 +
14 files changed, 74 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..7fde40d06181 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2514,10 +2514,9 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
- return -EINVAL;
- }
+ r = drm_drv_enabled(&amdgpu_kms_driver)
+ if (r)
+ return r;
r = amdgpu_sync_init();
if (r)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..802063279b86 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -233,8 +233,11 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
- return -EINVAL;
+ int ret;
+
+ ret = drm_drv_enabled(&ast_driver);
+ if (ret && ast_modeset == -1)
+ return ret;
if (ast_modeset == 0)
return -EINVAL;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..3fb567d62881 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const char
*name)
}
EXPORT_SYMBOL(drm_dev_set_unique);
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_drv_enabled(const struct drm_driver *driver)
+{
+ if (vgacon_text_force()) {
+ DRM_INFO("%s driver is disabled\n", driver->name);
+ return -ENODEV;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
+
/*
* DRM Core
* The DRM core module initializes all global DRM objects and makes them
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index ab2295dd4500..45cb3e540eff 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -18,9 +18,12 @@
#include "i915_selftest.h"
#include "i915_vma.h"
+static const struct drm_driver driver;
+
static int i915_check_nomodeset(void)
{
bool use_kms = true;
+ int ret;
/*
* Enable KMS by default, unless explicitly overriden by
@@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
- if (vgacon_text_force() && i915_modparams.modeset == -1)
+ ret = drm_drv_enabled(&driver);
+ if (ret && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..2a581094ba2b 100644
--- a/drivers/gpu/drm/mgag
[Intel-gfx] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it.
Let's move the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename the function to drm_check_modeset()
which better reflects what the function is really used to test for.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 -
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_drv.c | 9 +
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
18 files changed, 39 insertions(+), 44 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 7fde40d06181..b4b6993861e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 802063279b86..6222082c3082 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3fb567d62881..80b85b8ea776 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
*/
int drm_drv_enabled(const struct drm_driver *driver)
{
- if (vgacon_text_force()) {
+ int ret;
+
+ ret = drm_check_modeset();
+ if (ret)
DRM_INFO("%s driver is disabled\n", driver->name);
- return -ENODEV;
- }
- return 0;
+ return ret;
}
EXPORT_SYMBOL(drm_drv_enabled);
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..6683e396d2c5
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool drm_nomodeset;
+
+int drm_check_modeset(void)
+{
+ return drm_nomodeset ? -ENODEV : 0;
+}
+EXPORT_SYMBOL(drm_check_modeset);
+
+static int __init disable_modeset(char *str)
+{
+ drm_nomodeset = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+
+ return 1;
+}
+
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 45cb3e540eff..c890c1ca20c4 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
* Copyright © 2021 Intel Corporation
*/
-#include
-
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_object.h"
#include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 2a581094ba2b.
Re: [Intel-gfx] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On 11/4/21 17:24, Jani Nikula wrote: [snip] >> index ab2295dd4500..45cb3e540eff 100644 >> --- a/drivers/gpu/drm/i915/i915_module.c >> +++ b/drivers/gpu/drm/i915/i915_module.c >> @@ -18,9 +18,12 @@ >> #include "i915_selftest.h" >> #include "i915_vma.h" >> >> +static const struct drm_driver driver; >> + > > No, this makes absolutely no sense, and will also oops on nomodeset. > Ups, sorry about that. For some reason I thought that it was defined in the same compilation unit, but I noticed now that it is in i915_drv.c. > BR, > Jani. > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Sam,
On 11/4/21 18:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Sam Ravnborg wrote:
>> Hi Javier,
>>
>> On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
>>> Some DRM drivers check the vgacon_text_force() function return value as an
>>> indication on whether they should be allowed to be enabled or not.
>>>
>>> This function returns true if the nomodeset kernel command line parameter
>>> was set. But there may be other conditions besides this to determine if a
>>> driver should be enabled.
>>>
>>> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
>>> can be later extended if needed, without having to modify all the drivers.
>>>
>>> Also, while being there do some cleanup. The vgacon_text_force() function
>>> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
>>>
>>> Suggested-by: Thomas Zimmermann
>>> Signed-off-by: Javier Martinez Canillas
>>> ---
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 8214a0b1ab7f..3fb567d62881 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const
>>> char *name)
>>> }
>>> EXPORT_SYMBOL(drm_dev_set_unique);
>>>
>>> +/**
>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>> + * @driver: DRM driver to check
>>> + *
>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>> + * if the "nomodeset" kernel command line parameter is used.
>>> + *
>>> + * Return: 0 on success or a negative error code on failure.
>>> + */
>>> +int drm_drv_enabled(const struct drm_driver *driver)
>>> +{
>>> + if (vgacon_text_force()) {
>>> + DRM_INFO("%s driver is disabled\n", driver->name);
>>
>> DRM_INFO is deprecated, please do not use it in new code.
>> Also other users had an error message and not just info - is info
>> enough?
>>
Thanks, I didn't know that. Right, they had an error but I do wonder
if that was correct though. After all isn't an error but an explicit
disable due "nomodeset" being set in the kernel command line.
[snip]
>>>
>>> - if (vgacon_text_force() && i915_modparams.modeset == -1)
>>> + ret = drm_drv_enabled(&driver);
>>
>> You pass the local driver variable here - which looks wrong as this is
>> not the same as the driver variable declared in another file.
>
Yes, Jani mentioned it already. I got confused and thought that the driver
variable was also defined in the same compilation unit...
Maybe I could squash the following change?
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b18a250e5d2e..b8f399b76363 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -89,7 +89,7 @@
#include "intel_region_ttm.h"
#include "vlv_suspend.h"
-static const struct drm_driver driver;
+const struct drm_driver driver;
static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
{
@@ -1777,7 +1777,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
};
-static const struct drm_driver driver = {
+const struct drm_driver driver = {
/* Don't use MTRRs here; the Xserver or userspace app should
* deal with them for Intel hardware.
*/
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c890c1ca20c4..88f770920324 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -16,7 +16,7 @@
#include "i915_selftest.h"
#include "i915_vma.h"
-static const struct drm_driver driver;
+extern const struct drm_driver driver;
static int i915_check_nomodeset(void)
{
That should work and I actually got a laptop with an i915 and tested the change
both when CONFIG_DRM_I915=m and CONFIG_DRM_I915=y is set.
Another option is to declare it in i915_drv.h and not make the definition
static.
> Indeed.
>
>> Maybe move the check to new function you can add to init_funcs,
>> and locate the new function in i915_drv - so it has access to driver?
>
> We don't really want that, though. This check is pretty much as early as
> it can be, and there's a ton of useless initialization that would happen
> if we waited until drm_driver is available.
>
Agreed.
> From my POV, drm_drv_enabled() is a solution that creates a worse
> problem for us than it solves.
>
I don't have a strong opinion on this. I could just do patch #2 without adding
a level of indirection through drm_drv_enabled(). But Thomas and Daniel Vetter
suggested that we should do this change before.
That is, the drivers could just check if should be enabled by calling to the
drm_check_modeset() function directly if people agree that encapsulating that
logic in a drm_drv_enabled() is not needed.
>
> BR,
> Jani.
>
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Jani,
On 11/4/21 20:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
>> +/**
>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>> + * @driver: DRM driver to check
>> + *
>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>> + * if the "nomodeset" kernel command line parameter is used.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int drm_drv_enabled(const struct drm_driver *driver)
>> +{
>> +if (vgacon_text_force()) {
>> +DRM_INFO("%s driver is disabled\n", driver->name);
>> +return -ENODEV;
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(drm_drv_enabled);
>
> The name implies a bool return, but it's not.
>
> if (drm_drv_enabled(...)) {
> /* surprise, it's disabled! */
> }
>
It used to return a bool in v2 but Thomas suggested an int instead to
have consistency on the errno code that was returned by the callers.
I should probably name that function differently to avoid confusion.
But I think you are correct and this change is caused too much churn
for not that much benefit, specially since is unclear that there might
be another condition to prevent a DRM driver to load besides nomodeset.
I'll just drop this patch and post only #2 but making drivers to test
using the drm_check_modeset() function (which doesn't have a name that
implies a bool return).
>
> BR,
> Jani.
>
>
>
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Thomas,
On 11/5/21 09:43, Thomas Zimmermann wrote:
> Hi
>
> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>> Hello Jani,
>>
>> On 11/4/21 20:57, Jani Nikula wrote:
>>> On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
>>>> +/**
>>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>>> + * @driver: DRM driver to check
>>>> + *
>>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>>> + * if the "nomodeset" kernel command line parameter is used.
>>>> + *
>>>> + * Return: 0 on success or a negative error code on failure.
>>>> + */
>>>> +int drm_drv_enabled(const struct drm_driver *driver)
>
> Jani mentioned that i915 absolutely wants this to run from the
> module_init function. Best is to drop the parameter.
>
Ok. I now wonder though how much value would add this function since
it will just be a wrapper around the nomodeset check.
We talked about adding a new DRIVER_GENERIC feature flag and check for
this, but as danvet mentioned that is not really needed. We just need
to avoid testing for nomodeset in the simpledrm driver.
Do you envision other condition that could be added later to disable a
DRM driver ? Or do you think that just from a code readability point of
view makes worth it ?
>>>> +{
>>>> + if (vgacon_text_force()) {
>>>> + DRM_INFO("%s driver is disabled\n", driver->name);
>>>> + return -ENODEV;
>>>> + }
>
> If we run this from within a module_init function, we'd get plenty of
> these warnings if drivers are compiled into the kernel. Maybe simply
> remove the message. There's already a warning printed by the nomodeset
> handler.
>
Indeed. I'll just drop it.
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>>
>>> The name implies a bool return, but it's not.
>>>
>>> if (drm_drv_enabled(...)) {
>>> /* surprise, it's disabled! */
>>> }
>>>
>>
>> It used to return a bool in v2 but Thomas suggested an int instead to
>> have consistency on the errno code that was returned by the callers.
>>
>> I should probably name that function differently to avoid confusion.
>
> Yes, please.
>
drm_driver_check() maybe ?
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
On 11/5/21 10:00, Thomas Zimmermann wrote:
[snip]
>> +
>> +static int __init disable_modeset(char *str)
>> +{
>> +drm_nomodeset = true;
>> +
>> +pr_warn("You have booted with nomodeset. This means your GPU drivers
>> are DISABLED\n");
>> +pr_warn("Any video related functionality will be severely degraded, and
>> you may not even be able to suspend the system properly\n");
>> +pr_warn("Unless you actually understand what nomodeset does, you should
>> reboot without enabling it\n");
>
> I'd update this text to be less sensational.
>
This is indeed quite sensational. But think we can do it as a follow-up patch.
Since we will have to stick with nomodeset for backward compatibility, I was
planning to add documentation to explain what this parameter is about and what
is the actual effect of setting it.
So I think we can change this as a part of that patch-set.
>> +
>> +return 1;
>> +}
>> +
>> +/* Disable kernel modesetting */
>> +__setup("nomodeset", disable_modeset);
>> diff --git a/drivers/gpu/drm/i915/i915_module.c
>> b/drivers/gpu/drm/i915/i915_module.c
>> index 45cb3e540eff..c890c1ca20c4 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -4,8 +4,6 @@
>>* Copyright © 2021 Intel Corporation
>>*/
>>
>> -#include
>> -
>
> These changes should be in patch 1?
>
Yes, I forgot to move these when changed the order of the patches.
Best regards, --
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
On 11/5/21 10:39, Thomas Zimmermann wrote: [snip] >>>> >>>> +obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o >>>> + >>> >>> This now depends on the VGA textmode console. Even if you have no VGA >>> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can >>> provide graphics. Non-PC systems don't even have a VGA device. >> >> This was discussed in an earlier version, which had this builtin but the >> header still had a stub for CONFIG_VGA_CONSOLE=n. >> >>> I think we really want a separate boolean config option that gets >>> selected by CONFIG_DRM. >> >> Perhaps that should be a separate change on top. > > Sure, make it a separate patch. > Agreed. I was planning to do it as a follow-up as well and drop the #ifdef CONFIG_VGA_CONSOLE guard in the header. > We want to make this work on ARM systems. I even have a request to > replace offb on Power architecture by simpledrm. So the final config has > to be system agnostic. > Same, since we want to drop the fbdev drivers in Fedora, for all arches. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On 11/5/21 11:04, Jani Nikula wrote: > On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: [snip] >> >> Do you envision other condition that could be added later to disable a >> DRM driver ? Or do you think that just from a code readability point of >> view makes worth it ? > > Taking a step back for perspective. > > I think there's broad consensus in moving the parameter to drm, naming > the check function to drm_something_something(), and breaking the ties > to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that > effect. > Thanks, I appreciate your feedback and comments. > I think everything beyond that is still a bit vague and/or > contentious. So how about making the first 2-3 patches just that? > Something we can all agree on, makes good progress, improves the kernel, > and gives us something to build on? > That works for me. Thomas, do you agree with that approach ? Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [drm-tip:drm-tip 4/8] drivers/gpu/drm/solomon/ssd130x.c:451:18: error: incomplete definition of type 'struct dma_buf_map'
Hello Lucas,
On 2/17/22 07:36, Lucas De Marchi wrote:
> +Javier
>
>
> On Thu, Feb 17, 2022 at 02:25:08PM +0800, kernel test robot wrote:
>> tree: git://anongit.freedesktop.org/drm/drm-tip drm-tip
>> head: e141e36b2871c529379f7ec7d5d6ebae3137a51b
>> commit: 7ca6504c36709f35c4cc38ae6acc1c9c3d72136f [4/8] Merge remote-tracking
>> branch 'drm-misc/drm-misc-next' into drm-tip
>> config: mips-buildonly-randconfig-r002-20220217
>> (https://download.01.org/0day-ci/archive/20220217/202202171455.bclm1ybc-...@intel.com/config)
>> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project
>> 0bad7cb56526f2572c74449fcf97c1fcda42b41d)
>> reproduce (this is a W=1 build):
>>wget
>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
>> ~/bin/make.cross
>>chmod +x ~/bin/make.cross
>># install mips cross compiling tool for clang build
>># apt-get install binutils-mips-linux-gnu
>>git remote add drm-tip git://anongit.freedesktop.org/drm/drm-tip
>>git fetch --no-tags drm-tip drm-tip
>>git checkout 7ca6504c36709f35c4cc38ae6acc1c9c3d72136f
>># save the config file to linux build tree
>>mkdir build_dir
>>COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
>> O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpu/drm/solomon/
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot
>>
>> All error/warnings (new ones prefixed by >>):
>>
>>>> drivers/gpu/drm/solomon/ssd130x.c:447:74: warning: declaration of 'struct
>>>> dma_buf_map' will not be visible outside of this function [-Wvisibility]
>> static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct
>> dma_buf_map *map,
>>^
>
> this is now called iosys_map in drm-intel... drm-tip will need a fixup
> for the merge.
>
I thought that the drm-intel tree was only for Intel DRM drivers changes and
subsystem wide changes should be merged through drm-mic ?
Doing refactoring in that tree will likely lead to merge conflicts like this.
Noticed your series in dri-devel but missed that already landed in drm-intel.
The resolution should just be [0] right? If you confirm that then I can post
a proper patch to dri-devel.
>>>> drivers/gpu/drm/solomon/ssd130x.c:451:18: error: incomplete definition of
>>>> type 'struct dma_buf_map'
>> void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly
>> */
>>~~~^
>
> this shouldn't really be done.
>
Yes, I know but asked what would be the proper way and didn't get an answer.
We have many drivers doing the same and I couldn't find one that was doing
it correctly to use as a reference:
$ git grep "TODO: Use mapping abstraction properly" | wc -l
15
If you point me the proper way, I'll be happy to post a patch to change it.
> Lucas De Marchi
>
[0]
>From f8268e5b15c321b56862904665f5e312bf50d397 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Thu, 17 Feb 2022 09:52:37 +0100
Subject: [PATCH] drm/solomon: Rename dma-buf-map to iosys-map
Commit 7938f4218168 ("dma-buf-map: Rename to iosys-map") renamed the struct
dma_buf_map to struct iosys_map, but this change wasn't present in drm-misc
when the ssd130x driver was merged, and it created a merge conflict.
Fix this by renaming the data structure type in the ssd130x driver.
Reported-by: kernel test robot
Suggested-by: Lucas De Marchi
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/solomon/ssd130x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c
b/drivers/gpu/drm/solomon/ssd130x.c
index 19697c8c5a2c..92c1902f53e4 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -444,7 +444,7 @@ static void ssd130x_clear_screen(struct ssd130x_device
*ssd130x)
kfree(buf);
}
-static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct
dma_buf_map *map,
+static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct
iosys_map *map,
struct drm_rect *rect)
{
struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
--
2.34.1
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [drm-tip:drm-tip 4/8] drivers/gpu/drm/solomon/ssd130x.c:451:18: error: incomplete definition of type 'struct dma_buf_map'
On 2/17/22 10:25, Lucas De Marchi wrote: > On Thu, Feb 17, 2022 at 10:00:42AM +0100, Javier Martinez Canillas wrote: [snip] >>> this is now called iosys_map in drm-intel... drm-tip will need a fixup >>> for the merge. >>> >> >> I thought that the drm-intel tree was only for Intel DRM drivers changes and >> subsystem wide changes should be merged through drm-mic ? >> >> Doing refactoring in that tree will likely lead to merge conflicts like this. > > Yes, I know. My initial proposal was to split the rename and do it per > branch to avoid this kind of situation, but it was requested to be done > all in a single patch. Since I had other ~15 patches dependent on that > one to be merged in drm-intel, it was agreed to do the rename via > drm-intel. See > https://lore.kernel.org/lkml/e3813696-7b91-510c-987f-85ed2fd50...@suse.de/ > Got it. Thanks for the explanation. > I guess the conflicts won't be that terrible and can be fixed as they > show up. > Agreed. >> Noticed your series in dri-devel but missed that already landed in drm-intel. >> >> The resolution should just be [0] right? If you confirm that then I can post >> a proper patch to dri-devel. >> >>>>>> drivers/gpu/drm/solomon/ssd130x.c:451:18: error: incomplete definition >>>>>> of type 'struct dma_buf_map' >>>> void *vmap = map->vaddr; /* TODO: Use mapping abstraction >>>> properly */ >>>>~~~^ >>> >>> this shouldn't really be done. >>> >> >> Yes, I know but asked what would be the proper way and didn't get an answer. >> We have many drivers doing the same and I couldn't find one that was doing >> it correctly to use as a reference: >> >> $ git grep "TODO: Use mapping abstraction properly" | wc -l >> 15 >> >> If you point me the proper way, I'll be happy to post a patch to change it. > > It depends what you want to do with the address. There are APIs to copy > from/to. I also added a few to read/write to an offset. It seems the > problem here is that you need to pass that to a helper, > drm_fb_xrgb_to_mono_reversed(). I think the proper solution would be > to change the helper to accept an iosys_map* as argument rather than a > void*. > That makes a lot of sense. Once the dust settles and your series land in drm-misc-next, I can take a look at this and removing the TODO comment. > Lucas De Marchi >>> >>> Lucas De Marchi Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [drm-tip:drm-tip 4/8] drivers/gpu/drm/solomon/ssd130x.c:451:18: error: incomplete definition of type 'struct dma_buf_map'
On 2/17/22 10:41, Thomas Zimmermann wrote: > Hi > > Am 17.02.22 um 10:25 schrieb Lucas De Marchi: > [...] >>> $ git grep "TODO: Use mapping abstraction properly" | wc -l >>> 15 >>> >>> If you point me the proper way, I'll be happy to post a patch to >>> change it. >> >> It depends what you want to do with the address. There are APIs to copy >> from/to. I also added a few to read/write to an offset. It seems the >> problem here is that you need to pass that to a helper, >> drm_fb_xrgb_to_mono_reversed(). I think the proper solution would be >> to change the helper to accept an iosys_map* as argument rather than a >> void*. > > There are several of these TODOs in the DRM code, because our > format-conversion helpers are still from the time before > dma_buf_map/iosys_map. The easiest workaround is to take the raw pointer > and give it to them. One day, DRM's public blit and conversion > interfaces will take an iosys_map and handle the different memory types > internally. > Yes, as Lucas mentioned there are copy from/to helpers but didn't want to use it because would be another unnecessary copy just to avoid to take the map->vaddr raw pointer and pass directly to the format-conversion helper. > Javierm, that's when the internal _line() helpers will become useful. > They can use system memory directly, and for I/O memory the > blit/conversion helpers allocate an internal temporary per-line buffer. > The code you made for the new driver has this designed outlined already. > Indeed. I understand now what you meant when proposing to add those helpers. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH 3/5] drm/i915/dsi: Add some debug logging to mipi_exec_i2c
Hello Hans, On 2/25/22 22:49, Hans de Goede wrote: > Add some debug logging to mipi_exec_i2c, to make debugging various > issues seen with it easier. > > Signed-off-by: Hans de Goede > --- > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 > 1 file changed, 4 insertions(+) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH 01/21] MAINTAINERS: Add entry for fbdev core
On 1/31/22 22:05, Daniel Vetter wrote: > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin > > I think ideally we'd also record that the various firmware fb drivers > (efifb, vesafb, ...) are also maintained in drm-misc because for the > past few years the patches have either been to fix handover issues > with drm drivers, or caused handover issues with drm drivers. So any > other tree just doesn't make sense. But also, there's plenty of > outdated MAINTAINER entries for these with people and git trees that > haven't been active in years, so maybe let's just leave them alone. > And furthermore distros are now adopting simpledrm as the firmware fb > driver, so hopefully the need to care about the fbdev firmware drivers > will go down going forward. > > Note that drm-misc is group maintained, I expect that to continue like > we've done before, so no new expectations that patches all go through > my hands. That would be silly. This also means I'm happy to put any > other volunteer's name in the M: line, but otherwise git log says I'm > the one who's stuck with this. > > Cc: Dave Airlie > Cc: Jani Nikula > Cc: Linus Torvalds > Cc: Linux Fbdev development list > Cc: Pavel Machek > Cc: Sam Ravnborg > Cc: Greg Kroah-Hartman > Cc: Javier Martinez Canillas > Cc: DRI Development > Cc: Linux Kernel Mailing List > Cc: Claudio Suarez > Cc: Tomi Valkeinen > Cc: Geert Uytterhoeven > Cc: Thomas Zimmermann > Cc: Daniel Vetter > Cc: Sven Schnelle > Cc: Gerd Hoffmann > Signed-off-by: Daniel Vetter > --- Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions
On 2/8/22 22:08, Daniel Vetter wrote: > Avoids two forward declarations, and more importantly, matches what > I've done in my fbcon scrolling restore patches - so I need this to > avoid a bunch of conflicts in rebasing since we ended up merging > Helge's series instead. > > Signed-off-by: Daniel Vetter Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 06/19] fbcon: Use delayed work for cursor
Hello Daniel,
On 2/8/22 22:08, Daniel Vetter wrote:
> Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
> code we initialize the cursor_work completely when we allocate the
> fbcon_ops structure, instead of trying to cope with console
> re-initialization.
>
Maybe also make it more explicit in the commit message that the delayed
work is replacing a timer that was used before for the cursor ?
> The motiviation here is that fbcon code stops using the fb_info.queue,
motivation
[snip]
> /*
> *This is the interface between the low-level console driver and the
> @@ -68,7 +68,7 @@ struct fbcon_ops {
> int (*update_start)(struct fb_info *info);
> int (*rotate_font)(struct fb_info *info, struct vc_data *vc);
> struct fb_var_screeninfo var; /* copy of the current fb_var_screeninfo
> */
> - struct timer_list cursor_timer; /* Cursor timer */
> + struct delayed_work cursor_work; /* Cursor timer */
A delayed_work uses a timer underneath but I wonder if the comment also
needs to be updated since technically isn't a timer anymore but deferred
work that gets re-scheduled each time on fb_flashcursor().
The patch looks good to me and makes the logic much simpler than before.
Reviewed-by: Javier Martinez Canillas
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Intel-gfx] [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
On 2/8/22 22:08, Daniel Vetter wrote:
> This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
>
> With
>
> commit 27599aacbaefcbf2af7b06b0029459bbf682000d
> Author: Thomas Zimmermann
> Date: Tue Jan 25 10:12:18 2022 +0100
>
> fbdev: Hot-unplug firmware fb devices on forced removal
>
> this should be fixed properly and we can remove this somewhat hackish
> check here (e.g. this won't catch drm drivers if fbdev emulation isn't
> enabled).
>
Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
of platform devices matched with fbdev drivers to be properly unregistered if
a DRM driver attempts to remove all the conflicting framebuffers.
But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
a FB is already registered") worked around is different. It happens when the
DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
kicking out of conflicting framebuffers already happened and these drivers
will be allowed to probe even when a DRM driver is already present.
We need a clearer way to prevent it, but can't revert fb561bf9abde until that.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
[Intel-gfx] [PATCH] drm/i915/irq: Fix misspelled word register in kernel-doc
There is a typo in the function i915_handle_error() kernel-doc and the word register is spelled wrongly. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/i915/i915_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 0344a9181dac..38dadd23684d 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2571,7 +2571,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev) * i915_handle_error - handle a gpu error * @dev: drm device * - * Do some basic checking of regsiter state at error time and + * Do some basic checking of register state at error time and * dump it to the syslog. Also call i915_capture_error_state() to make * sure we get a record and make it available in debugfs. Fire a uevent * so userspace knows something bad happened (should trigger collection -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/irq: Fix kernel-doc warnings
Add the dev parameter for the functions i915_enable_asle_pipestat() and
i915_reset_and_wakeup() to the kernel-doc to fix the following warnings:
.//drivers/gpu/drm/i915/i915_irq.c:586: warning: No description found for
parameter 'dev'
.//drivers/gpu/drm/i915/i915_irq.c:2400: warning: No description found for
parameter 'dev'
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/i915/i915_irq.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8ca772deabdb..0344a9181dac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -581,6 +581,7 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv,
enum pipe pipe,
/**
* i915_enable_asle_pipestat - enable ASLE pipestat for OpRegion
+ * @dev: drm device
*/
static void i915_enable_asle_pipestat(struct drm_device *dev)
{
@@ -2392,6 +2393,7 @@ static void i915_error_wake_up(struct drm_i915_private
*dev_priv,
/**
* i915_reset_and_wakeup - do process context error handling work
+ * @dev: drm device
*
* Fire an error uevent so userspace can see that a hang or error
* was detected.
--
2.4.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm: Spelling s/sempahore/semaphore/
Geert Uytterhoeven writes: > Fix misspellings of "semaphore". > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Hamza Mahfooz > --- > v2: > - Add Reviewed-by. > --- Pushed to drm-misc (drm-misc-next). Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 1/3] drm/fb-helper: set x/yres_virtual in drm_fb_helper_check_var
Daniel Vetter writes:
> Drivers are supposed to fix this up if needed if they don't outright
> reject it. Uncovered by 6c11df58fd1a ("fbmem: Check virtual screen
> sizes in fb_set_var()").
>
Should have a Fixes: tag ? I understand what was uncovered by that commit
but it help distros to figure out if something has to be cherry-picked by
them. So I believe that would be useful to have it.
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 2/3] drm/fb-helper: drop redundant pixclock check from drm_fb_helper_set_par()
Daniel Vetter writes: > The fb_check_var hook is supposed to validate all this stuff. Any > errors from fb_set_par are considered driver/hw issues and resulting > in dmesg warnings. > > Luckily we do fix up the pixclock already, so this is all fine. > > Signed-off-by: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > --- Makes sense to drop this. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var
Daniel Vetter writes:
> Apparently drivers need to check all this stuff themselves, which for
> most things makes sense I guess. And for everything else we luck out,
> because modern distros stopped supporting any other fbdev drivers than
> drm ones and I really don't want to argue anymore about who needs to
> check stuff. Therefore fixing all this just for drm fbdev emulation is
> good enough.
>
Agreed.
> Note that var->active is not set or validated. This is just control
> flow for fbmem.c and needs to be validated in there as needed.
>
> Signed-off-by: Daniel Vetter
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> ---
[...]
>
> +static void __fill_var(struct fb_var_screeninfo *var,
> +struct drm_framebuffer *fb)
> +{
> + int i;
> +
> + var->xres_virtual = fb->width;
> + var->yres_virtual = fb->height;
> + var->accel_flags = FB_ACCELF_TEXT;
> + var->bits_per_pixel = drm_format_info_bpp(fb->format, 0);
> +
> + var->height = var->width = 0;
> + var->left_margin = var->right_margin = 0;
> + var->upper_margin = var->lower_margin = 0;
> + var->hsync_len = var->vsync_len = 0;
> + var->sync = var->vmode = 0;
> + var->rotate = 0;
> + var->colorspace = 0;
> + for (i = 0; i < 4; i++)
> + var->reserved[i] = 0;
> +}
> +
> /**
> * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var
> * @var: screeninfo to check
> @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo
> *var,
> return -EINVAL;
> }
>
> - var->xres_virtual = fb->width;
> - var->yres_virtual = fb->height;
> + __fill_var(var, fb);
> +
[...]
There is the following here (in latest drm-misc/drm-misc-next at least):
/*
* Changes struct fb_var_screeninfo are currently not pushed back
* to KMS, hence fail if different settings are requested.
*/
bpp = drm_format_info_bpp(format, 0);
if (var->bits_per_pixel > bpp ||
var->xres > fb->width || var->yres > fb->height ||
var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in
current fb "
"request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
var->xres, var->yres, var->bits_per_pixel,
var->xres_virtual, var->yres_virtual,
fb->width, fb->height, bpp);
return -EINVAL;
}
but only the 'var->xres > fb->width || var->yres > fb->height' from the
conditions checked could be false after your __fill_var() call above.
You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual >
fb->width' and 'var->yres_virtual > fb->height' checks I believe since
those will always be true.
> + /*
> + * fb_pan_display() validates this, but fb_set_par() doesn't and just
> + * falls over. Note that __fill_var above adjusts y/res_virtual.
> + */
> + if (var->yoffset > var->yres_virtual - var->yres ||
> + var->xoffset > var->xres_virtual - var->xres)
> + return -EINVAL;
> +
> + /* We neither support grayscale nor FOURCC (also stored in here). */
> + if (var->grayscale > 0)
> + return -EINVAL;
> +
> + if (var->nonstd)
> + return -EINVAL;
>
> /*
>* Workaround for SDL 1.2, which is known to be setting all pixel format
> @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo
> *var,
> drm_fb_helper_fill_pixel_fmt(var, format);
> }
>
Other than what I mentioned, the patch makes sense to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Thomas Zimmermann writes:
[...]
>
> Your comment says that it calls a PCI function to clean up to vgacon.
> That comment explains what is happening, not why. And how the PCI and
> vgacon code work together is non-obvious.
>
> Again, here's my proposal for gma500:
>
> // call this from psb_pci_probe()
> int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
> struct drm_driver *req_driver)
> {
> resource_size_t base = 0;
> resource_size_t size = (resource_size_t)-1;
> const char *name = req_driver->name;
> int ret;
>
> /*
>* We cannot yet easily find the framebuffer's location in
>* memory. So remove all framebuffers here.
>*
>* TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
>* we might be able to read the framebuffer range from the
>* device.
>*/
> ret = aperture_remove_conflicting_devices(base, size, name);
> if (ret)
> return ret;
>
> /*
>* WARNING: Apparently we must kick fbdev drivers before vgacon,
>* otherwise the vga fbdev driver falls over.
>*/
> ret = vga_remove_vgacon(pdev);
> if (ret)
> return ret;
>
> return 0;
> }
>
If this is enough I agree that is much more easier code to understand.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 2/8] video/aperture: use generic code to figure out the vga default device
Daniel Vetter writes:
> Since vgaarb has been promoted to be a core piece of the pci subsystem
> we don't have to open code random guesses anymore, we actually know
> this in a platform agnostic way, and there's no need for an x86
> specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
> drivers/pci")
>
> This should not result in any functional change, and the non-x86
> multi-gpu pci systems are probably rare enough to not matter (I don't
> know of any tbh). But it's a nice cleanup, so let's do it.
>
> There's been a few questions on previous iterations on dri-devel and
> irc:
>
> - fb_is_primary_device() seems to be yet another implementation of
> this theme, and at least on x86 it checks for both
> vga_default_device OR rom shadowing. There shouldn't ever be a case
> where rom shadowing gives any additional hints about the boot vga
> device, but if there is then the default vga selection in vgaarb
> should probably be fixed. And not special-case checks replicated all
> over.
>
Agreed and if there are regressions reported then could be added there.
> - Thomas also brought up that on most !x86 systems
> fb_is_primary_device() returns 0, except on sparc/parisc. But these
> 2 special cases are about platform specific devices and not pci, so
> shouldn't have any interactions.
>
> - Furthermore fb_is_primary_device() is a bit a red herring since it's
> only used to select the right fbdev driver for fbcon, and not for
> the fw handover dance which the aperture helpers handle. At least
> for x86 we might want to look into unifying them, but that's a
> separate thing.
>
> v2: Extend commit message trying to summarize various discussions.
>
> Signed-off-by: Daniel Vetter
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Helge Deller
> Cc: linux-fb...@vger.kernel.org
> Cc: Bjorn Helgaas
> Cc: linux-...@vger.kernel.org
> ---
> drivers/video/aperture.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index b009468ffdff..8835d3bc39bf 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
> */
> int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char
> *name)
> {
> - bool primary = false;
> + bool primary;
> resource_size_t base, size;
> int bar, ret;
>
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags &
> IORESOURCE_ROM_SHADOW;
> -#endif
> + primary = pdev == vga_default_device();
>
Maybe enclose the check in parenthesis to make it easier to read ?
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 3/8] drm/aperture: Remove primary argument
Daniel Vetter writes: > Only really pci devices have a business setting this - it's for > figuring out whether the legacy vga stuff should be nuked too. And > with the preceeding two patches those are all using the pci version of > this. > > Which means for all other callers primary == false and we can remove > it now. > > v2: > - Reorder to avoid compile fail (Thomas) > - Include gma500, which retained it's called to the non-pci version. > > Signed-off-by: Daniel Vetter Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 4/8] video/aperture: Only kick vgacon when the pdev is decoding vga
Daniel Vetter writes: > Otherwise it's bit silly, and we might throw out the driver for the > screen the user is actually looking at. I haven't found a bug report > for this case yet, but we did get bug reports for the analog case > where we're throwing out the efifb driver. > > v2: Flip the check around to make it clear it's a special case for > kicking out the vgacon driver only (Thomas) > > References: https://bugzilla.kernel.org/show_bug.cgi?id=216303 > Signed-off-by: Daniel Vetter > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > Cc: Helge Deller > Cc: linux-fb...@vger.kernel.org > --- > drivers/video/aperture.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 5/8] video/aperture: Move vga handling to pci function
Daniel Vetter writes: > A few reasons for this: > > - It's really the only one where this matters. I tried looking around, > and I didn't find any non-pci vga-compatible controllers for x86 > (since that's the only platform where we had this until a few > patches ago), where a driver participating in the aperture claim > dance would interfere. > > - I also don't expect that any future bus anytime soon will > not just look like pci towards the OS, that's been the case for like > 25+ years by now for practically everything (even non non-x86). > > - Also it's a bit funny if we have one part of the vga removal in the > pci function, and the other in the generic one. > > v2: Rebase. > > Signed-off-by: Daniel Vetter > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > Cc: Helge Deller > Cc: linux-fb...@vger.kernel.org > --- > drivers/video/aperture.c | 15 +++ > 1 file changed, 7 insertions(+), 8 deletions(-) Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 6/8] video/aperture: Drop primary argument
Daniel Vetter writes: > With the preceeding patches it's become defunct. Also I'm about to add > a different boolean argument, so it's better to keep the confusion > down to the absolute minimum. > > v2: Since the hypervfb patch got droppped (it's only a pci device for > gen1 vm, not for gen2) there is one leftover user in an actual driver > left to touch. > > Signed-off-by: Daniel Vetter > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > Cc: Helge Deller > Cc: linux-fb...@vger.kernel.org > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: "K. Y. Srinivasan" > Cc: Haiyang Zhang > Cc: Wei Liu > Cc: Dexuan Cui > Cc: linux-hyp...@vger.kernel.org > --- > drivers/gpu/drm/drm_aperture.c | 2 +- > drivers/video/aperture.c| 7 +++ > drivers/video/fbdev/hyperv_fb.c | 2 +- > include/linux/aperture.h | 9 - > 4 files changed, 9 insertions(+), 11 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 7/8] video/aperture: Only remove sysfb on the default vga pci device
Daniel Vetter writes:
> Instead of calling aperture_remove_conflicting_devices() to remove the
> conflicting devices, just call to aperture_detach_devices() to detach
> the device that matches the same PCI BAR / aperture range. Since the
> former is just a wrapper of the latter plus a sysfb_disable() call,
> and now that's done in this function but only for the primary devices.
>
> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
> sysfb device registration when removing conflicting FBs"), where we
> remove the sysfb when loading a driver for an unrelated pci device,
> resulting in the user loosing their efifb console or similar.
>
> Note that in practice this only is a problem with the nvidia blob,
> because that's the only gpu driver people might install which does not
> come with an fbdev driver of it's own. For everyone else the real gpu
> driver will restore a working console.
>
> Also note that in the referenced bug there's confusion that this same
> bug also happens on amdgpu. But that was just another amdgpu specific
> regression, which just happened to happen at roughly the same time and
> with the same user-observable symptoms. That bug is fixed now, see
> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
>
> Note that we should not have any such issues on non-pci multi-gpu
> issues, because I could only find two such cases:
> - SoC with some external panel over spi or similar. These panel
> drivers do not use drm_aperture_remove_conflicting_framebuffers(),
> so no problem.
> - vga+mga, which is a direct console driver and entirely bypasses all
> this.
>
> For the above reasons the cc: stable is just notionally, this patch
> will need a backport and that's up to nvidia if they care enough.
>
> v2:
> - Explain a bit better why other multi-gpu that aren't pci shouldn't
> have any issues with making all this fully pci specific.
>
> v3
> - polish commit message (Javier)
>
> Fixes: ee7a69aa38d8 ("fbdev: Disable sysfb device registration when removing
> conflicting FBs")
> Tested-by: Aaron Plattner
> Reviewed-by: Javier Martinez Canillas
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
> Signed-off-by: Daniel Vetter
> Cc: Aaron Plattner
> Cc: Javier Martinez Canillas
> Cc: Thomas Zimmermann
> Cc: Helge Deller
> Cc: Sam Ravnborg
> Cc: Alex Deucher
> Cc: # v5.19+ (if someone else does the backport)
> ---
> drivers/video/aperture.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 8/8] fbdev: Simplify fb_is_primary_device for x86
Daniel Vetter writes:
> vga_default_device really is supposed to cover all corners, at least
> for x86. Additionally checking for rom shadowing should be redundant,
> because the bios/fw only does that for the boot vga device.
>
> If this turns out to be wrong, then most likely that's a special case
> which should be added to the vgaarb code, not replicated all over.
>
> Patch motived by changes to the aperture helpers, which also have this
> open code in a bunch of places, and which are all removed in a
> clean-up series. This function here is just for selecting the default
> fbdev device for fbcon, but I figured for consistency it might be good
> to throw this patch in on top.
>
> Note that the shadow rom check predates vgaarb, which was only wired
> up in 88674088d10c ("x86: Use vga_default_device() when determining
> whether an fb is primary"). That patch doesn't explain why we still
> fall back to the shadow rom check.
>
> Signed-off-by: Daniel Vetter
> Cc: Daniel Vetter
> Cc: Helge Deller
> Cc: Daniel Vetter
> Cc: Javier Martinez Canillas
> Cc: Thomas Zimmermann
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Borislav Petkov
> Cc: Dave Hansen
> Cc: x...@kernel.org
> Cc: "H. Peter Anvin"
> ---
> arch/x86/video/fbdev.c | 13 +
[...]
> + if (pci_dev == vga_default_device())
> return 1;
> return 0;
or just return pci_dev == vga_default_device() ;
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var
Daniel Vetter writes: [...] >> >> but only the 'var->xres > fb->width || var->yres > fb->height' from the >> conditions checked could be false after your __fill_var() call above. >> >> You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual > >> fb->width' and 'var->yres_virtual > fb->height' checks I believe since >> those will always be true. > > The __fill_var is after this. I'm honestly not sure what the exact Ah, your patch adds it after that indeed. Please ignore my comment then. > semantics are supposed to be, but essentially if userspace asks for too > big virtual size, we reject it. And for anything else we then tell it > (with __fill_var) how big the actually available space is. > > What I'm wondering now is whether too small x/yres won't lead to problems > of some sorts ... For multi-screen we set the virtual size to be big > enough for all crtc, and then just set x/yres to be the smallest output. > That way fbcon knows to only draw as much as is visible on all screens. > But if you then pan that too much, the bigger screens might not have a big > enough buffer anymore and things fail (but shouldn't). > > Not sure how to fix that tbh. Would this be a problem in practice? -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Daniel Vetter writes:
> On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
[...]
>> > > >/*
>> > > > * WARNING: Apparently we must kick fbdev drivers before vgacon,
>> > > > * otherwise the vga fbdev driver falls over.
>> > > > */
>> > > >ret = vga_remove_vgacon(pdev);
>> >
>> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.
[...]
>> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
>> {
>> aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
>>
>> return vga_remove_vgacon(pdev);
>> }
>>
>> And that can be called from gma500 and the pci aperture helper.
>
> But you still pass a pci_dev to that helper. Which just doesn't make any
> sense to me (assuming your entire point is that this isn't just a normal
> pci device but some special legacy vga thing), but if we go with (void)
> then there's more refactoring to do because the vga_remove_vgacon also
> wants a pdev.
>
> All so that we don't call aperture_detach_devices() on a bunch of pci
> bars, which apparently is not problem for any other driver, but absolutely
> is a huge problem for gma500 somehow.
>
> I don't understand why.
>
Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
is needed then starts to get a little silly. Maybe one option is to add a
3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
to iterate over PCI bars and call aperture_remove_conflicting_devices() ?
> Consider this me throwing in the towel. If you&Javier are convinced this
> makes sense please type it up and merge it, but I'm not going to type
> something that just doesn't make sense to me.
Honestly, I would just go with the double drm_aperture_remove_*() helper
calls (your original patch) unless that causes real issues. There is no
point on blocking all your series just for this IMO.
Then latter if Thomas has strong opinions can send a follow-up patch for
the gma500 driver and the aperture helpers.
> -Daniel
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var
Daniel Vetter writes:
> On Wed, Apr 05, 2023 at 06:27:17PM +0200, Javier Martinez Canillas wrote:
>> Daniel Vetter writes:
[...]
>> >
>> > The __fill_var is after this. I'm honestly not sure what the exact
>>
>> Ah, your patch adds it after that indeed. Please ignore my comment then.
>
> So rb: you?
>
Yes, I already provided it in my previous email and has been picked by
patchwork. I could do again but probably will confuse dim when applying.
The only patch from your series that is missing an {r,a}b is #1 right now:
https://patchwork.kernel.org/project/dri-devel/list/?series=736966&archived=both
[...]
>> > What I'm wondering now is whether too small x/yres won't lead to problems
>> > of some sorts ... For multi-screen we set the virtual size to be big
>> > enough for all crtc, and then just set x/yres to be the smallest output.
>> > That way fbcon knows to only draw as much as is visible on all screens.
>> > But if you then pan that too much, the bigger screens might not have a big
>> > enough buffer anymore and things fail (but shouldn't).
>> >
>> > Not sure how to fix that tbh.
>>
>> Would this be a problem in practice?
>
> I'm frankly not sure. You'd get a black screen for fbcon/fbdev across all
> outputs, but only if you have userspace doing this intentionally.
>
> In a way it's just another artifact of the drm fbdev emulation not using
> ATOMIC_TEST_ONLY in the various places where it should, and so doesn't
> really know whether a configuration change will work out.
>
> We already have this in obscure mulit-monitor cases where adding another
> screen kills fbcon, because the display hw is running out of fifo or
> clocks or whatever, and because the drm fbdev code doesn't check but just
> blindly commits the entire thing as an atomic commit, the overall commit
> fails.
>
> This worked "better" with legacy kms because there we commit per-crtc, so
> if any specific crtc runs into a limit check, only that one fails to light
> up.
>
> Imo given that no one cared enough yet to write up atomic TEST_ONLY
> support for fbdev emulation I think we can continue to just ignore this
> problem.
>
Agreed. If that ends being a problem for people in practice then I guess
someone can type atomic TEST_ONLY support for the fbdev emulation layer.
> What should not happen is that fbcon code blows up drawing out of bounds
> or something like that, resulting in a kernel crash. So from that pov I
> think it's "safe" :-)
Great. Thanks a lot for your explanations.
> -Daniel
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Daniel Vetter writes: > On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas > wrote: >> >> Daniel Vetter writes: [...] >> >> Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper >> is needed then starts to get a little silly. Maybe one option is to add a >> 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic >> to iterate over PCI bars and call aperture_remove_conflicting_devices() ? > > The thing I don't get: Why does this matter for gma500 and not any of > the other pci devices? Look at your gpu, realize there's a lot more Yes, I don't know why gma500 is special in that sense but I'm not familiar with that hardware to have an opinion on this. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var
Daniel Vetter writes:
> On Wed, Apr 05, 2023 at 07:42:08PM +0200, Javier Martinez Canillas wrote:
[...]
>> >> Ah, your patch adds it after that indeed. Please ignore my comment then.
>> >
>> > So rb: you?
>> >
>>
>> Yes, I already provided it in my previous email and has been picked by
>> patchwork. I could do again but probably will confuse dim when applying.
>
> Yeah just wanted to confirm I cleared up all your questions. Merged the
> entire series to drm-misc-next, thanks for the review.
>
You are welcome.
>> The only patch from your series that is missing an {r,a}b is #1 right now:
>>
>> https://patchwork.kernel.org/project/dri-devel/list/?series=736966&archived=both
>
> That's a different one :-)
>
Oh, sorry about that. Somehow I switched threads in my head in the middle
of the response.
> I'll respin with your comments and then let you&Thomas duke it out about
> patch 1.
> -Daniel
>
Perfect, thanks! It would be good to finally have that issue fixed.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Thomas Zimmermann writes:
[...]
> Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> Gma500 therefore calls both helpers to catch all cases. It's confusing
> as it implies that there's something about the PCI device that requires
> ownership management. The relationship between the PCI device and the
> VGA devices is non-obvious. At worst, readers might assume that calling
> two functions for aperture clearing ownership is a bug in the driver.
>
Yeah, or someone looking as the driver for reference may wrongly think
that calling both aperture helpers are needed for PCI drivers and that
is not the case.
> Hence, move the PCI removal helper's code for VGA functionality into
> a separate function and call this function from gma500. Documents the
> purpose of each call to aperture helpers. The change contains comments
> and example code form the discussion at [1].
>
> Signed-off-by: Thomas Zimmermann
> Link:
> https://patchwork.kernel.org/project/dri-devel/patch/20230404201842.567344-1-daniel.vet...@ffwll.ch/
> # 1
> ---
Looks good to me and I agree that it makes the code easier to understand.
Reviewed-by: Javier Martinez Canillas
I've a couple of comments below though:
[...]
> + * Hardware for gma500 is a hybrid device, which both acts as a PCI
> + * device (for legacy vga functionality) but also more like an
> + * integrated display on a SoC where the framebuffer simply
> + * resides in main memory and not in a special PCI bar (that
> + * internally redirects to a stolen range of main memory) like all
> + * other integrated PCI display devices have.
> + *
Is "have" the correct word here or "do" ? Or maybe "are implemented" ?
> + * To catch all cases we need to remove conflicting firmware devices
> + * for the stolen system memory and for the VGA functionality. As we
> + * currently cannot easily find the framebuffer's location in stolen
> + * memory, we remove all framebuffers here.
> + *
> + * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
> + * we might be able to read the framebuffer range from the
> + * device.
> + */
> +static int gma_remove_conflicting_framebuffers(struct pci_dev *pdev,
> +const struct drm_driver
> *req_driver)
> {
> - struct drm_psb_private *dev_priv;
> - struct drm_device *dev;
> + resource_size_t base = 0;
> + resource_size_t size = PHYS_ADDR_MAX;
> + const char *name = req_driver->name;
> int ret;
>
> - /*
> - * We cannot yet easily find the framebuffer's location in memory. So
> - * remove all framebuffers here. Note that we still want the pci special
> - * handling to kick out vgacon.
> - *
> - * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> - * might be able to read the framebuffer range from the device.
> - */
> - ret = drm_aperture_remove_framebuffers(&driver);
> + ret = aperture_remove_conflicting_devices(base, size, name);
> if (ret)
> return ret;
>
> - ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> + return __aperture_remove_legacy_vga_devices(pdev);
I don't like the __ prefix for this function name, as it usually implies
that is a function that is only local to the compilation unit. But it is
an exported symbol from the aperture infrastructure.
[...]
> +/**
> + * __aperture_remove_legacy_vga_devices - remove legacy VGA devices of a PCI
> devices
> + * @pdev: PCI device
> + *
> + * This function removes VGA devices provided by @pdev, such as a VGA
> + * framebuffer or a console. This is useful if you have a VGA-compatible
> + * PCI graphics device with framebuffers in non-BAR locations. Drivers
> + * should acquire ownership of those memory areas and afterwards call
> + * this helper to release remaining VGA devices.
> + *
> + * If your hardware has its framebuffers accessible via PCI BARS, use
> + * aperture_remove_conflicting_pci_devices() instead. The function will
> + * release any VGA devices automatically.
> + *
> + * WARNING: Apparently we must remove graphics drivers before calling
> + * this helper. Otherwise the vga fbdev driver falls over if
> + * we have vgacon configured.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise
> + */
> +int __aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> +{
> + /* VGA framebuffer */
> + aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> +
> + /* VGA textmode console */
> + return vga_remove_vgacon(pdev);
> +}
> +EXPORT_SYMBOL(__aperture_remove_legacy_vga_devices);
I would just call this symbol aperture_remove_legacy_vga_devices() as
mentioned, the fact that aperture_remove_conflicting_pci_devices() use it
internally is an implementation detail IMO. But it's an exported symbol so
the naming should be consistent.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Thomas Zimmermann writes: [...] >>> +EXPORT_SYMBOL(__aperture_remove_legacy_vga_devices); >> >> I would just call this symbol aperture_remove_legacy_vga_devices() as >> mentioned, the fact that aperture_remove_conflicting_pci_devices() use it >> internally is an implementation detail IMO. But it's an exported symbol so >> the naming should be consistent. > > That prefix __ is supposed to indicate that it's not a all-in-one > solution. Most of all, it doesn't do sysfb_disable(). The helper is > meant to be used as part of a larger function. I tried to outline this > in the comment, where I say that drivers should first aquire framebuffer > ownership and then call this helper. If naming isn't a showstopper, I'd > like to keep the underscores. > Sure, I see that we have other symbols exported in DRM that have a __ prefix in their name. So maybe I am the one who was confused on the meaning of it. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH] fbmem: Reject FB_ACTIVATE_KD_TEXT from userspace
Daniel Vetter writes:
> This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt
> restore") - I failed to realize that nasty userspace could set this.
>
> It's not pretty to mix up kernel-internal and userspace uapi flags
> like this, but since the entire fb_var_screeninfo structure is uapi
> we'd need to either add a new parameter to the ->fb_set_par callback
> and fb_set_par() function, which has a _lot_ of users. Or some other
> fairly ugly side-channel int fb_info. Neither is a pretty prospect.
>
> Instead just correct the issue at hand by filtering out this
> kernel-internal flag in the ioctl handling code.
>
> Signed-off-by: Daniel Vetter
> Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore")
[..]
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 875541ff185b..3fd95a79e4c3 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1116,6 +1116,8 @@ static long do_fb_ioctl(struct fb_info *info, unsigned
> int cmd,
> case FBIOPUT_VSCREENINFO:
> if (copy_from_user(&var, argp, sizeof(var)))
> return -EFAULT;
> + /* only for kernel-internal use */
> + var.activate &= ~FB_ACTIVATE_KD_TEXT;
> console_lock();
I don't have a better idea on how to fix this and as you said the whole
struct fb_var_screeninfo is an uAPI anyways...
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Uwe Kleine-König writes: [dropping some recipients since my SMTP server was complaining about the size] > Hello Thomas, > > On Wed, Jul 12, 2023 at 12:19:37PM +0200, Thomas Zimmermann wrote: >> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: >> > Hello, >> > >> > while I debugged an issue in the imx-lcdc driver I was constantly >> > irritated about struct drm_device pointer variables being named "dev" >> > because with that name I usually expect a struct device pointer. >> > >> > I think there is a big benefit when these are all renamed to "drm_dev". >> >> If you rename drm_crtc.dev, you should also address *all* other data >> structures. > > Yes. Changing drm_crtc::dev was some effort, so I thought to send that > one out before doing the same to > > drm_dp_mst_topology_mgr > drm_atomic_state > drm_master > drm_bridge > drm_client_dev > drm_connector > drm_debugfs_entry > drm_encoder > drm_fb_helper > drm_minor > drm_framebuffer > drm_gem_object > drm_plane > drm_property > drm_property_blob > drm_vblank_crtc > > when in the end the intention isn't welcome. > >> > I have no strong preference here though, so "drmdev" or "drm" are fine >> > for me, too. Let the bikesheding begin! >> >> We've discussed this to death. IIRC 'drm' would be the prefered choice. > > "drm" at least has the advantage to be the 2nd most common name. With > Paul Kocialkowski prefering "drm_dev" there is no clear favourite yet. I think that either "drm" or "drm_dev" would be more clear than "dev", which I also found it confusing and thinking about a "struct device". Probably leaning to "drm", since as you said is the second most used name in drivers that assign crtc->dev to a local variable. > Maybe all the other people with strong opinions are dead if this was > "discussed to death" before? :-) > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König| > Industrial Linux Solutions | https://www.pengutronix.de/ | -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
Thomas Zimmermann writes:
> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
> calling fbdev implementation. Avoids a possible stale mutex with
> generic fbdev code.
>
> As indicated by its name, drm_fb_helper_prepare() prepares struct
> drm_fb_helper before setting up the fbdev support with a call to
> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
> to each other. If successful, drm_fb_helper_fini() later tear down
> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>
> Generic fbdev emulation prepares struct drm_fb_helper immediately
> after allocating the instance. It only calls drm_fb_helper_init()
> as part of processing a hotplug event. If the hotplug-handling fails,
> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
> and the next hotplug event runs on stale data.
>
> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
> into the fbdev implementations. Call it right before freeing the
> fb-helper instance.
>
> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize
client unregistering") instead? Because commit 4825797c36da just added a
wrapper function for mutex_destroy(&fb_helper->lock), but it was commit
032116bbe152 that made drm_fbdev_cleanup() to call that helper function.
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-de...@lists.freedesktop.org
>
> Signed-off-by: Thomas Zimmermann
> ---
The change itself looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
Thomas Zimmermann writes:
> Hi
>
> Am 21.02.23 um 11:27 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann writes:
>>
>>> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
>>> calling fbdev implementation. Avoids a possible stale mutex with
>>> generic fbdev code.
>>>
>>> As indicated by its name, drm_fb_helper_prepare() prepares struct
>>> drm_fb_helper before setting up the fbdev support with a call to
>>> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
>>> to each other. If successful, drm_fb_helper_fini() later tear down
>>> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>>>
>>> Generic fbdev emulation prepares struct drm_fb_helper immediately
>>> after allocating the instance. It only calls drm_fb_helper_init()
>>> as part of processing a hotplug event. If the hotplug-handling fails,
>>> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
>>> and the next hotplug event runs on stale data.
>>>
>>> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
>>> into the fbdev implementations. Call it right before freeing the
>>> fb-helper instance.
>>>
>>> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
>>
>> I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize
>> client unregistering") instead? Because commit 4825797c36da just added a
>> wrapper function for mutex_destroy(&fb_helper->lock), but it was commit
>> 032116bbe152 that made drm_fbdev_cleanup() to call that helper function.
>
> Good point. After looking through the recent fbdev commits, I'll use
> commit 643231b28380 ("drm/fbdev-generic: Minimize hotplug error
> handling") for the tag. This is the one that added the call to
> drm_fb_helper_fini() to the client's hotplug handler. And _fini()
> currently does the _unprepare(), when it shouldn't.
>
Ah, much better indeed.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH v5 2/7] drm/client: Do not acquire module reference
Thomas Zimmermann writes: > Do not acquire a reference on the module that provides a client's > callback functions in drm_client_init(). The additional reference > prevents the user from unloading the callback functions' module and > thus creating dangling pointers. > > This is only necessary if there is no direct dependency between the > caller of drm_client_init() and the provider of the callbacks in > struct drm_client_funcs. If this case ever existed, it has been > removed from the DRM code. Callers of drm_client_init() also provide > the callback implementation. The lifetime of the clients is tied to > the dependency chain's outer-most module, which is the hardware's > DRM driver. Before client helpers could be unloaded, the driver module > would have to be unloaded, which also unregisters all clients. > > Driver modules that set up DRM clients can now be unloaded. > > Signed-off-by: Thomas Zimmermann > --- The change makes sense to me. Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [RFC] drm: enable W=1 warnings by default across the subsystem
Maxime Ripard writes: > Hi, > > On Thu, Nov 30, 2023 at 10:52:17AM +0200, Jani Nikula wrote: >> On Wed, 29 Nov 2023, Hamza Mahfooz wrote: >> > Cc: Nathan Chancellor >> > >> > On 11/29/23 13:12, Jani Nikula wrote: >> >> At least the i915 and amd drivers enable a bunch more compiler warnings >> >> than the kernel defaults. >> >> >> >> Extend the W=1 warnings to the entire drm subsystem by default. Use the >> >> copy-pasted warnings from scripts/Makefile.extrawarn with >> >> s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it easier to compare and keep >> >> up with them in the future. >> >> >> >> This is similar to the approach currently used in i915. >> >> >> >> Some of the -Wextra warnings do need to be disabled, just like in >> >> Makefile.extrawarn, but take care to not disable them for W=2 or W=3 >> >> builds, depending on the warning. >> > >> > I think this should go in after drm-misc-next has a clean build (for >> > COMPILE_TEST builds) with this patch applied. Otherwise, it will break a >> > lot of build configs. >> >> Oh, I'm absolutely not suggesting this should be merged before known >> warnings have been addressed one way or another. Either by fixing them >> or by disabling said warning in driver local Makefiles, depending on the >> case. > > I'm all for it, but yeah, we need some easy way to opt-in/opt-out. Some > drivers are pretty much unmaintained now and are likely to never fix > those warnings. > Maybe add a Kconfig symbol for it instead of making unconditional? Something like: +# Unconditionally enable W=1 warnings locally +# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn +subdir-ccflags-$(CONFIG_DRM_EXTRA_CHECKS) += -Wextra -Wunused -Wno-unused-parameter ... > Maxime -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [RFC] drm: enable W=1 warnings by default across the subsystem
Jani Nikula writes: [...] > > Then I'd go for enabling in drm level and disabling individual warnings > in the driver Makefile level if they won't get fixed. > >> Maybe add a Kconfig symbol for it instead of making unconditional? >> >> Something like: >> >> +# Unconditionally enable W=1 warnings locally >> +# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn >> +subdir-ccflags-$(CONFIG_DRM_EXTRA_CHECKS) += -Wextra -Wunused >> -Wno-unused-parameter >> ... > > Then we'll have a ping pong of people not using W=1 or > CONFIG_DRM_EXTRA_CHECKS introducing warnings, and people using them > fixing the warnings... > > I really do think making it unconditional is the only way. > Fair enough. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [RFC] drm: enable W=1 warnings by default across the subsystem
Maxime Ripard writes: > On Thu, Nov 30, 2023 at 11:46:00AM +0200, Jani Nikula wrote: [...] >> >> Then we'll have a ping pong of people not using W=1 or >> CONFIG_DRM_EXTRA_CHECKS introducing warnings, and people using them >> fixing the warnings... >> >> I really do think making it unconditional is the only way. > > Yeah, I agree. > > Plus, if we need to have an extra Kconfig option, it's pretty equivalent > to using W=1 > Yeah, with the difference that having a Kconfig symbol allows for example distros to set it in their kernel configs. It's a fair point though that probably the only option is to enable it unconditionally. Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/mm: Allow CONFIG_DRM_MM_DEBUG with DRM=m
Ville Syrjala writes:
Hello Ville,
> From: Ville Syrjälä
>
> The original rationale for
> commit cd456f8d06d2 ("drm: Restrict stackdepot usage to builtin drm.ko")
> was that depot_save_stack() (which is what we used back then)
> wasn't exported. stack_depot_save() (which is what we use now) is
> exported however, so relax the dependency allow CONFIG_DRM_MM_DEBUG
> with DRM=m.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 31cfe2c2a2af..4b8b8f8a0e72 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -42,7 +42,7 @@ config DRM_MIPI_DSI
> config DRM_DEBUG_MM
> bool "Insert extra checks and debug info into the DRM range managers"
> default n
> - depends on DRM=y
> + depends on DRM
> depends on STACKTRACE_SUPPORT
> select STACKDEPOT
> help
> --
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 1/2] drm: Don't unref the same fb many times by mistake due to deadlock handling
Ville Syrjala writes: Hello Ville, > From: Ville Syrjälä > > If we get a deadlock after the fb lookup in drm_mode_page_flip_ioctl() > we proceed to unref the fb and then retry the whole thing from the top. > But we forget to reset the fb pointer back to NULL, and so if we then > get another error during the retry, before the fb lookup, we proceed > the unref the same fb again without having gotten another reference. > The end result is that the fb will (eventually) end up being freed > while it's still in use. > > Reset fb to NULL once we've unreffed it to avoid doing it again > until we've done another fb lookup. > > This turned out to be pretty easy to hit on a DG2 when doing async > flips (and CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y). The first symptom I > saw that drm_closefb() simply got stuck in a busy loop while walking > the framebuffer list. Fortunately I was able to convince it to oops > instead, and from there it was easier to track down the culprit. > > Cc: sta...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- Acked-by: Javier Martinez Canillas > drivers/gpu/drm/drm_plane.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 9e8e4c60983d..672c655c7a8e 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -1503,6 +1503,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > out: > if (fb) > drm_framebuffer_put(fb); > + fb = NULL; > if (plane->old_fb) > drm_framebuffer_put(plane->old_fb); > plane->old_fb = NULL; Interesting that it was done correctly for old_fb. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 2/2] drm: Warn when freeing a framebuffer that's still on a list
Ville Syrjala writes: > From: Ville Syrjälä > > Sprinkle some extra WARNs around so that we might catch > premature framebuffer destruction more readily. > > Signed-off-by: Ville Syrjälä > --- Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
On 9/23/22 11:15, Thomas Zimmermann wrote:
> Hi
>
> Am 22.09.22 um 16:25 schrieb Maxime Ripard:
>> drm_connector_pick_cmdline_mode() is in charge of finding a proper
>> drm_display_mode from the definition we got in the video= command line
>> argument.
>>
>> Let's add some unit tests to make sure we're not getting any regressions
>> there.
>>
>> Signed-off-by: Maxime Ripard
>>
>> diff --git a/drivers/gpu/drm/drm_client_modeset.c
>> b/drivers/gpu/drm/drm_client_modeset.c
>> index bbc535cc50dd..d553e793e673 100644
>> --- a/drivers/gpu/drm/drm_client_modeset.c
>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev
>> *client, int mode)
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_client_modeset_dpms);
>> +
>> +#ifdef CONFIG_DRM_KUNIT_TEST
>> +#include "tests/drm_client_modeset_test.c"
>> +#endif
>
> I strongly dislike this style of including source files in each other.
> It's a recipe for all kind of build errors. Can you do something else?
>
This seems to be the convention used to test static functions and what's
documented in the Kunit docs:
https://kunit.dev/third_party/kernel/docs/tips.html#testing-static-functions
I agree with you that's not ideal but I think that consistency with how
it is done by other subsystems is also important.
> As the tested function is an internal interface, maybe export a wrapper
> if tests are enabled, like this:
>
> #ifdef CONFIG_DRM_KUNIT_TEST
> struct drm_display_mode *
> drm_connector_pick_cmdline_mode_kunit(drm_conenctor)
> {
>return drm_connector_pick_cmdline_mode(connector)
> }
> EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit)
> #endif
>
> The wrapper's declaration can be located in the kunit test file.
>
But that's also not nice since we are artificially exposing these only
to allow the static functions to be called from unit tests. And would
be a different approach than the one used by all other subsystems...
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
On 9/23/22 12:30, Thomas Zimmermann wrote:
[...]
>>>> +
>>>> +#ifdef CONFIG_DRM_KUNIT_TEST
>>>> +#include "tests/drm_client_modeset_test.c"
>>>> +#endif
>>>
>>> I strongly dislike this style of including source files in each other.
>>> It's a recipe for all kind of build errors. Can you do something else?
>>>
>>
>> This seems to be the convention used to test static functions and what's
>> documented in the Kunit docs:
>> https://kunit.dev/third_party/kernel/docs/tips.html#testing-static-functions
>
> That document says "...one option is to conditionally #include the test
> file...". This doesn't sound like a strong requirement.
>
That's true.
>>
>> I agree with you that's not ideal but I think that consistency with how
>> it is done by other subsystems is also important.
>>
>>> As the tested function is an internal interface, maybe export a wrapper
>>> if tests are enabled, like this:
>>>
>>> #ifdef CONFIG_DRM_KUNIT_TEST
>>> struct drm_display_mode *
>>> drm_connector_pick_cmdline_mode_kunit(drm_conenctor)
>>> {
>>> return drm_connector_pick_cmdline_mode(connector)
>>> }
>>> EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit)
>>> #endif
>>>
>>> The wrapper's declaration can be located in the kunit test file.
>>>
>>
>> But that's also not nice since we are artificially exposing these only
>> to allow the static functions to be called from unit tests. And would
>> be a different approach than the one used by all other subsystems...
>>
>
> There's the problem of interference between the source files when
> building the code. It's also not the same source code after including
> the test file. At a minimum, including the tests' source file further
> includes more files. also includes quite a few of Linux
> header files.
>
> IMHO the current convention (if any) is far from optimal and we should
> consider breaking it.
>
Yes, I agree with that. But probably we should be explicit about the
wrapper export symbols to access static functions pattern in the KUnit
docs so other subsystems could do the same and it becomes a convention ?
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH v3] drm: Only select I2C_ALGOBIT for drivers that actually need it
Hello Uwe, On 12/19/22 09:36, Uwe Kleine-König wrote: > While working on a drm driver that doesn't need the i2c algobit stuff I > noticed that DRM selects this code even though only 8 drivers actually use > it. While also only some drivers use i2c, keep the select for I2C for the > next cleanup patch. Still prepare this already by also selecting I2C for > the individual drivers. > > Signed-off-by: Uwe Kleine-König > --- Thanks for sending a v3 of this. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 01/18] fbcon: Remove trailing whitespaces
On 12/19/22 17:04, Thomas Zimmermann wrote: > Fix coding style. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 02/18] Revert "fbcon: don't lose the console font across generic->chip driver switch"
On 12/19/22 17:05, Thomas Zimmermann wrote:
> This reverts commit ae1287865f5361fa138d4d3b1b6277908b54eac9.
>
> Always free the console font when deinitializing the framebuffer
> console. Subsequent framebuffer consoles will then use the default
> font. Rely on userspace to load any user-configured font for these
> consoles.
>
> Commit ae1287865f53 ("fbcon: don't lose the console font across
> generic->chip driver switch") was introduced to work around losing
> the font during graphics-device handover. [1][2] It kept a dangling
> pointer with the font data between loading the two consoles, which is
> fairly adventurous hack. It also never covered cases when the other
> consoles, such as VGA text mode, where involved.
>
> The problem has meanwhile been solved in userspace. Systemd comes
> with a udev rule that re-installs the configured font when a console
> comes up. [3] So the kernel workaround can be removed.
>
> This also removes one of the two special cases triggered by setting
> FBINFO_MISC_FIRMWARE in an fbdev driver.
>
> Tested during device handover from efifb and simpledrm to radeon. Udev
> reloads the configured console font for the new driver's terminal.
>
> Signed-off-by: Thomas Zimmermann
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=892340 # 1
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1074624 # 2
> Link:
> https://cgit.freedesktop.org/systemd/systemd/tree/src/vconsole/90-vconsole.rules.in?h=v222
> # 3
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 03/18] drm/gma500: Do not set struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Generic fbdev drivers use the apertures field in struct fb_info to > control ownership of the framebuffer memory and graphics device. Do > not set the values in gma500. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 04/18] drm/i915: Do not set struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Generic fbdev drivers use the apertures field in struct fb_info to > control ownership of the framebuffer memory and graphics device. Do > not set the values in i915. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 05/18] drm/radeon: Do not set struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Generic fbdev drivers use the apertures field in struct fb_info to > control ownership of the framebuffer memory and graphics device. Do > not set the values in radeon. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 06/18] drm/fb-helper: Do not allocate unused apertures structure
On 12/19/22 17:05, Thomas Zimmermann wrote: > The apertures field in struct fb_info is not used by DRM drivers. Do > not allocate it. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 07/18] fbdev/clps711x-fb: Do not set struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Generic fbdev drivers use the apertures field in struct fb_info to > control ownership of the framebuffer memory and graphics device. Do > not set the values in clps711x-fb. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 08/18] fbdev/hyperv-fb: Do not set struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Generic fbdev drivers use the apertures field in struct fb_info to > control ownership of the framebuffer memory and graphics device. Do > not set the values in hyperv-fb. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 09/18] vfio-mdev/mdpy-fb: Do not set struct fb_info.apertures
[adding Kirti Wankhede and k...@vger.kernel.org to Cc list]
On 12/19/22 17:05, Thomas Zimmermann wrote:
> Generic fbdev drivers use the apertures field in struct fb_info to
> control ownership of the framebuffer memory and graphics device. Do
> not set the values in mdpy-fb.
>
> Signed-off-by: Thomas Zimmermann
> ---
> samples/vfio-mdev/mdpy-fb.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/samples/vfio-mdev/mdpy-fb.c b/samples/vfio-mdev/mdpy-fb.c
> index 9ec93d90e8a5..1de5801cd2e8 100644
> --- a/samples/vfio-mdev/mdpy-fb.c
> +++ b/samples/vfio-mdev/mdpy-fb.c
> @@ -161,14 +161,6 @@ static int mdpy_fb_probe(struct pci_dev *pdev,
> goto err_release_fb;
> }
>
> - info->apertures = alloc_apertures(1);
> - if (!info->apertures) {
> - ret = -ENOMEM;
> - goto err_unmap;
> - }
> - info->apertures->ranges[0].base = info->fix.smem_start;
> - info->apertures->ranges[0].size = info->fix.smem_len;
> -
> info->fbops = &mdpy_fb_ops;
> info->flags = FBINFO_DEFAULT;
> info->pseudo_palette = par->palette;
Reviewed-by: Javier Martinez Canillas
But I think an ack from Kirti Wankhede or other virt folk is needed if you
want to merge this through drm-misc-next.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Intel-gfx] [PATCH 10/18] fbdev/efifb: Add struct efifb_par for driver data
On 12/19/22 17:05, Thomas Zimmermann wrote: > The efifb_par structure holds the palette for efifb. It will also > be useful for storing the device's aperture range. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 11/18] fbdev/efifb: Do not use struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Acquire ownership of the firmware scanout buffer by calling Linux' > aperture helpers. Remove the use of struct fb_info.apertures and do > not set FBINFO_MISC_FIRMWARE; both of which previously configured > buffer ownership. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 12/18] fbdev/offb: Allocate struct offb_par with framebuffer_alloc()
On 12/19/22 17:05, Thomas Zimmermann wrote: > Move the palette array into struct offb_par and allocate both via > framebuffer_alloc(), as intended by fbdev. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 13/18] fbdev/offb: Do not use struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Acquire ownership of the firmware scanout buffer by calling Linux' > aperture helpers. Remove the use of struct fb_info.apertures and do > not set FBINFO_MISC_FIRMWARE; both of which previously configured > buffer ownership. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 14/18] fbdev/simplefb: Do not use struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Acquire ownership of the firmware scanout buffer by calling Linux' > aperture helpers. Remove the use of struct fb_info.apertures and do > not set FBINFO_MISC_FIRMWARE; both of which previously configured > buffer ownership. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 15/18] fbdev/vesafb: Remove trailing whitespaces
On 12/19/22 17:05, Thomas Zimmermann wrote: > Fix coding style. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 16/18] fbdev/vesafb: Do not use struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Acquire ownership of the firmware scanout buffer by calling Linux' > aperture helpers. Remove the use of struct fb_info.apertures and do > not set FBINFO_MISC_FIRMWARE; both of which previously configured > buffer ownership. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 17/18] fbdev/vga16fb: Do not use struct fb_info.apertures
On 12/19/22 17:05, Thomas Zimmermann wrote: > Acquire ownership of the firmware scanout buffer by calling Linux' > aperture helpers. Remove the use of struct fb_info.apertures and do > not set FBINFO_MISC_FIRMWARE; both of which previously configured > buffer ownership. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH 18/18] drm/fbdev: Remove aperture handling and FBINFO_MISC_FIRMWARE
On 12/19/22 17:05, Thomas Zimmermann wrote: > There are no users left of struct fb_info.apertures and the flag > FBINFO_MISC_FIRMWARE. Remove both and the aperture-ownership code > in the fbdev core. All code for aperture ownership is now located > in the fbdev drivers. > > Signed-off-by: Thomas Zimmermann > --- > drivers/video/fbdev/core/fbmem.c | 33 -- > drivers/video/fbdev/core/fbsysfs.c | 1 - > include/linux/fb.h | 22 > 3 files changed, 56 deletions(-) Nice patch! Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 01/21] drm/komeda: Don't set struct drm_driver.lastclose
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.lastclose. It's used to restore the > fbdev console. But as komeda uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See > the call to drm_client_dev_restore() in drm_lastclose(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 02/21] drm/mcde: Don't set struct drm_driver.lastclose
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.lastclose. It's used to restore the > fbdev console. But as mcde uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See > the call to drm_client_dev_restore() in drm_lastclose(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 03/21] drm/vboxvideo: Don't set struct drm_driver.lastclose
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.lastclose. It's used to restore the > fbdev console. But as vboxvideo uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See > the call to drm_client_dev_restore() in drm_lastclose(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 04/21] drm/amdgpu: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as amdgpu uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas Do you think that the fbdev helpers kernel doc has to be updated to mention that drm_fb_helper_lastclose() and drm_fb_helper_output_poll_changed() are not needed when generic fbdev emulation is used? Because by reading that is not clear that's the case: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L86 -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 04/21] drm/amdgpu: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as amdgpu uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 05/21] drm/imx/dcss: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as DCSS uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 06/21] drm/ingenic: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as ingenic uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian, Sergey) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 07/21] drm/logicvc: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as logicvc uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [PATCH v2 08/21] drm/rockchip: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as rockchip uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
