Den 28.03.2019 10.31, skrev Daniel Vetter:
On Tue, Mar 26, 2019 at 06:55:30PM +0100, Noralf Trønnes wrote:
This moves the modesetting code from drm_fb_helper to drm_client so it can be shared by all internal clients.
I have also added a client display abstraction and a bootsplash example client to show where this might be heading. Hopefully Max Staudt will be able to pick up his bootsplash work now.
First a fairly unrelated thing that I noticed while reading stuff:
In drm_fbdev_generic_setup() we register the drm_client to the world (with drm_client_add) before it's fully set up. And the checks in the setup code aren't safe against a concurrent hotplug call from another thread. Which can happen, because usually by that point, and definitely by the time the driver called drm_dev_register() the hotplug handler is running.
Ah, this hasn't crossed my mind. I'll move drm_client_add() after setup.
Maybe good idea to rename drm_client_add to drm_client_register (to stay consistent with our naming scheme of _register() = others can start calling us from any thread).
Makes sense, I'll change the name.
We need to do the basic setup code _before_ we call drm_client_register. The kerneldoc for the various fbdev setup functions have explanations for when exactly it's ok to handle hotplug events.
The other bit is kinda the high-level review on the drm_client modeset api:
- Allowing multiple different modeset clients per drm_client feels like overkill. I think we can just require a 1:1 mapping between drm_client and modeset config. If a client wants to have multiple different modeset configs per drm_device they can create more drm_clients.
The reason I ended up doing this is that there can be more than one display connected. So for me the natural choice was to have each display represented individually with its own modeset(s).
I did consider having the modeset array attached to drm_client and use a modesets mask to tell the displays apart. It would have been easier if we didn't have those tiled monitors driven by multiple outputs, because that would have given us one modeset per display.
drm_fb_helper uses the same framebuffer for all displays, sizing it to match the smallest.
Ofc the easy way out is to only support one display per drm_device. I don't see how multiple drm_clients per drm_device should work. Should the client keep an array of drm_client for as many displays as it supports?
The bootsplash client example I made, puts a splash on all displays it can find. But maybe it should do it on only one display? If so we need some heuristics to determine which is the primary display.
Not sure how kmscon is supposed to work, but I believe it is meant to be a developer console. So I guess it will also just use the primary display.
That also fixes your "do we need embedding" question, since drm_client supports that already.
That means we could clean up the api considerably by embedding all the modeset stuff into drm_client, and e.g. allocating the modeset arrays at drm_client_init() time.
Except that wouldn't work with the current fbdev emulation code, because that one isn't always using drm_client.
Hence my question/suggestion: Could we rework the fbdev emulation to always allocate a drm_client, but only use drm_client for buffer allocation for generic_setup(). That could also provide us with a smoother upgrade path for other drivers to generic_setup, e.g. we could ditch all the hotplug handling already.
I'm thinking of embedding a drm_client into drm_fb_helper, and calling drm_client_init() on it at the right time. But only call drm_client_add() for generic_setup(). At least as a first step.
Yeah, this certainly makes sense if we have the modesets attached to drm_client_dev.
Related question: What's the plan for drivers which don't support generic_setup()? If we eventually have stuff like kmscon running on top of drm_client, we'd have to somehow port them all ...
It should just work as long the driver supports ->dumb_create and ->gem_prime_vmap.
rockchip and mediatek (I think) don't provide a virtual address on its buffers because of limited virtual address space. We could add a ->dumb_create_internal hook so these drivers could provide an address to internal clients. This way they could use the generic fbdev emualtion as well. Unless Rob finds a way to vmap a dma buffer after allocation. He has looked at converting rockchip to generic fbdev.
And finally the bikeshed: I thik drm_client_modeset would be a good prefix for all this (maybe even in a separate file):
I have used prefix drm_client_modesets_ for all functions that operates on the modeset array.
Emmanuel Vadot alerted me to the fact that dm_fb_helper is MIT licensed. I was now planning to change drm_client to MIT as well, so I can legally move over code. If we add drm_client_modeset.c with a MIT license, then I wouldn't have to change the license of drm_client.c. I think I would prefer to have it all in one file. drm_client.c is now 421 lines including everything, when I add the modeset code it becomes 1871. But it's not very important to me either way.
Noralf.
- we have a pretty clear split between basic drm stuff and kms
- modeset means kms, display usually only means the actual physical display. drm_simple_display_pipe always gets me with using display instead of modeset, but a bit too late to rename that one :-)
Thoughts on this?
Cheers, Daniel
Noralf.
Noralf Trønnes (16): drm/fb-helper: Remove unused gamma_size variable drm/fb-helper: dpms_legacy(): Only set on connectors in use drm/atomic: Move __drm_atomic_helper_disable_plane/set_config() drm/fb-helper: No need to cache rotation and sw_rotations drm/fb-helper: Remove drm_fb_helper_crtc->{x,y,desired_mode} drm/i915/fbdev: Move intel_fb_initial_config() to fbdev helper drm/fb-helper: Remove drm_fb_helper_crtc drm/fb-helper: Prepare to move out commit code drm/fb-helper: Move out commit code drm/fb-helper: Remove drm_fb_helper_connector drm/fb-helper: Prepare to move out modeset config code drm/fb-helper: Move out modeset config code drm/fb-helper: Avoid race with DRM userspace drm/client: Add display abstraction drm/client: Hack: Add bootsplash example drm/vc4: Call drm_dev_register() after all setup is done
Documentation/gpu/todo.rst | 10 + drivers/gpu/drm/Kconfig | 5 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_atomic.c | 168 ++++ drivers/gpu/drm/drm_atomic_helper.c | 164 --- drivers/gpu/drm/drm_auth.c | 20 + drivers/gpu/drm/drm_bootsplash.c | 216 ++++ drivers/gpu/drm/drm_client.c | 1449 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 5 + drivers/gpu/drm/drm_drv.c | 4 + drivers/gpu/drm/drm_fb_helper.c | 1151 ++------------------- drivers/gpu/drm/drm_internal.h | 2 + drivers/gpu/drm/i915/intel_fbdev.c | 218 ---- drivers/gpu/drm/vc4/vc4_drv.c | 6 +- include/drm/drm_atomic_helper.h | 4 - include/drm/drm_client.h | 117 +++ include/drm/drm_fb_helper.h | 127 +-- 17 files changed, 2128 insertions(+), 1539 deletions(-) create mode 100644 drivers/gpu/drm/drm_bootsplash.c
-- 2.20.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx