Re: [Intel-gfx] [PATCH 00/16] drm/fb-helper: Move modesetting code to drm_client

[Daniel Vetter <daniel@ffwll.ch>]

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel