Message-ID: <ZsS7x2y_HKgqGUFR@fedora>
Date: Tue, 20 Aug 2024 17:52:39 +0200
From: José Expósito <jose.exposito89@...il.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: rodrigosiqueiramelo@...il.com, melissa.srw@...il.com,
	mairacanal@...eup.net, hamohammed.sa@...il.com, daniel@...ll.ch,
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, airlied@...il.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	thomas@...tlin.com
Subject: Re: [RFC PATCH 00/17] VKMS: Add configfs support

Hi Louis,

Thanks a lot for the review and for your comments!

On Tue, Aug 13, 2024 at 07:58:55PM +0200, Louis Chauvet wrote:
> Le 13/08/24 - 12:44, José Expósito a écrit :
> > Hi everyone,
> 
> Hi José,
>  
> > This RFC implements support to configure VKMS using configfs.
> > It allows to:
> > 
> >  - Create multiple devices
> >  - Configure multiple overlay planes, CRTCs, encoders and
> >    connectors
> >  - Enable or disable cursor plane and writeback connector for
> >    each CRTC
> >  - Hot-plug/unplug connectors after device creation
> >  - Disable the creation of the default VKMS instance to be
> >    able to use only the configfs ones
> > 
> > This work is based on a previous attempt to implement configfs
> > support by Jim Shargo and Brandon Pollack [1].
> > I tried to keep the changes as minimal and simple as possible
> > and addressed Sima's comments on [1].
> > 
> > Currently, there is another RFC by Louis Chauvet [2]. As I
> > mentioned on his RFC, I'm not trying to push my implementation.
> > Instead, I think that having 2 implementations will make code
> > review way easier and I don't mind which implementation is used
> > as long as we get the feature implemented :)
> 
> I will send few series tomorrow, don't panic, there will be 9 series and a 
> total of ~50 commits (I have many conflict to rebase only the configFS 
> part, and even if it was easy, I plan to submit all of my work, not 
> everything will be RFC).

Nice work! I already reviewed "drm/vkms: Miscelanious clarifications",
"drm/vkms: Completly split headers" and "drm/vkms: Switch all vkms
object to DRM managed objects".

I'll have a look to the vkms_config and configfs as soon as possible,
but I think they'll have to wait until next week.

> > I'm looking forward to analyzing Louis's implementation, seeing
> > what the differences are and finding a common solution.
> 
> There are four main differences:
> - I complelty splitted vkms_config and vkms_configfs structures

I considered this and I didn't do it because it duplicates a lot of
fields, but your implementation does the right think. I need to
split both structures.

> - I splitted my work in many different series

Which is really nice, I think that the 3 series I reviewed can be
easily rebased on drm-misc-next and get merged. I'd be able to drop
at least 4 patches if your code is merged.

> - I created a real platform device driver
> - I did not manage index by hand, I let drm core doing it
> - I used list to link crtc/planes/encoders and not bitfield (because of 
>   the previous point)

I need to look about your solution for the indices. It is the root
cause for the problems I'm having with vblanks not being referenced
by the correct index.

> - The primary and cursor planes are fully configurable

I think this is the right approach. However, there are some restrictions
on primary planes and a user will be able to create some invalid setups.
The DRM subsystem complains if you create a CRTC without a primary plane,
but I'm not sure if we want to do the validation in VKMS as well.

There are other restrictions handled by DRM (for example, only 32 encoders
and CRTCs are allowed) and I don't know if re-implement all validation
rules in VKMS is the best idea.

> The first two points are personnal preferences, so I am open to 
> discussion.
> 
> The third point was already discussed before, I don't know if it is a good 
> solution or not. I think it should be easy to remove it.
> 
> But for the index managment, I really think that for our usage 
> in ConfigFS, bitfields are not a good solution and as shown in this 
> series, very error-prone. If you have a better solution than what I did, 
> let me know, I am not very happy with mine too.
> 
> The last point is also important, we don't want to break uAPI once this 
> series is merged, so having "default hidden planes" that can't be 
> configured is annoying as we will have to manage them with a special case.
> 
> > What's missing?
> > 
> >  - DebugFS only works for the default VKMS instance.
> >    If we want to support it on instances created with configfs
> >    I'll need to implement it.
> 
> Same on my side, I forgot to reimplement this :-). It will not be in my 
> RFC, but on the v1 for sure!
> 
> > Known bugs:
> > 
> >  - When a CRTC is added and removed before device creation, there
> >    is a vblank warning.
> >    The issue is caused because vblanks are referenced using the
> >    CRTC index but, because one of the CRTCs is removed, the
> >    indices are not consecutives and drm_crtc_vblank_crtc() tries to
> >    access and invalid index
> >    I'm not sure if CRTC's indices *must* start at 0 and be
> >    consecutives or if this is a bug in the drm_crtc_vblank_crtc()
> >    implementation.
> 
> Very nice work, but you hurted many issue I had too, and I attempted to 
> solve them as nicely as I can. Overall there is one main issues for me:
> the crtc index managment is not correct and the configfs behavior is very 
> easily broken because of this.
> 
> This is an issue for two reason I think:
> - We are trying to implement a new index allocation mecanism, but it is 
>   not very difficult to let drm manage this part on device creation, so 
>   maybe just dont store indexes in config
> - The usage of a simple index++ is not suitable for configFS usecase, 
>   crating 32 crtcs and deleting 1 should be possible:
> 	mkdir {1..32};rmdir 1;mkdir 1
>   but the index of 1 is now 33, which is forbidden by drm, so you have to 
>   do a "complex" algorithim "find_first_value_not_used_bellow_32".
> 
> Thanks for all your work! You were right, while reviewing your work, I 
> found issues in mine :-)
> 
> Have a nice day,
> Louis Chauvet

I ran out of time this week to work on VKMS, but I'll try to solve the
issues you pointed out.

Thanks again for your review,
Jose

> > 
> > Best wishes,
> > José Expósito
> > 
> > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=780110&archive=both
> > [2] https://lore.kernel.org/dri-devel/ZrZZFQW5RiG12ApN@louis-chauvet-laptop/T/#u
> > 
> > José Expósito (17):
> >   drm/vkms: Extract vkms_config header
> >   drm/vkms: Move default_config creation to its own function
> >   drm/vkms: Set device name from vkms_config
> >   drm/vkms: Allow to configure multiple CRTCs
> >   drm/vkms: Use managed memory to create encoders
> >   drm/vkms: Allow to configure multiple encoders
> >   drm/vkms: Use managed memory to create connectors
> >   drm/vkms: Allow to configure multiple connectors
> >   drm/vkms: Allow to configure multiple overlay planes
> >   drm/vkms: Allow to change connector status
> >   drm/vkms: Add and remove VKMS instances via configfs
> >   drm/vkms: Allow to configure multiple CRTCs via configfs
> >   drm/vkms: Allow to configure multiple encoders via configfs
> >   drm/vkms: Allow to configure multiple encoders
> >   drm/vkms: Allow to configure multiple planes via configfs
> >   drm/vkms: Allow to configure the default device creation
> >   drm/vkms: Remove completed task from the TODO list
> > 
> >  Documentation/gpu/vkms.rst            | 102 +++-
> >  drivers/gpu/drm/vkms/Kconfig          |   1 +
> >  drivers/gpu/drm/vkms/Makefile         |   4 +-
> >  drivers/gpu/drm/vkms/vkms_composer.c  |  30 +-
> >  drivers/gpu/drm/vkms/vkms_config.c    | 265 ++++++++++
> >  drivers/gpu/drm/vkms/vkms_config.h    | 101 ++++
> >  drivers/gpu/drm/vkms/vkms_configfs.c  | 721 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/vkms/vkms_configfs.h  |   9 +
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |  99 ++--
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  75 ++-
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  52 +-
> >  drivers/gpu/drm/vkms/vkms_output.c    | 187 ++++---
> >  drivers/gpu/drm/vkms/vkms_plane.c     |   6 +-
> >  drivers/gpu/drm/vkms/vkms_writeback.c |  27 +-
> >  14 files changed, 1464 insertions(+), 215 deletions(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_config.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_config.h
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h
> > 
> > -- 
> > 2.46.0
> > 
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Powered by blists - more mailing lists