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.
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).
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.
- 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.
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 ...
And finally the bikeshed: I thik drm_client_modeset would be a good prefix
for all this (maybe even in a separate file):
- 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
