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

[Noralf Trønnes]

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