Message-ID: <20190710170116.GB15868@phenom.ffwll.local>
Date:   Wed, 10 Jul 2019 19:01:16 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        Haneen Mohammed <hamohammed.sa@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Simon Ser <contact@...rsion.fr>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] drm/vkms: Introduce basic support for configfs

On Mon, Jul 01, 2019 at 12:23:39AM -0300, Rodrigo Siqueira wrote:
> This patchset introduces the support for configfs in vkms by adding a
> primary structure for handling the vkms subsystem and exposing
> connectors as a use case.  This series allows enabling/disabling virtual
> and writeback connectors on the fly. The first patch of this series
> reworks the initialization and cleanup code of each type of connector,
> with this change, the second patch adds the configfs support for vkms.
> It is important to highlight that this patchset depends on
> https://patchwork.freedesktop.org/series/61738/.
> 
> After applying this series, the user can utilize these features with the
> following steps:
> 
> 1. Load vkms without parameter
> 
>   modprobe vkms
> 
> 2. Mount a configfs filesystem
> 
>   mount -t configfs none /mnt/
> 
> After that, the vkms subsystem will look like this:
> 
> vkms/
>  |__connectors
>     |__Virtual
>         |__ enable
> 
> The connectors directories have information related to connectors, and
> as can be seen, the virtual connector is enabled by default. Inside a
> connector directory (e.g., Virtual) has an attribute named ‘enable’
> which is used to enable and disable the target connector. For example,
> the Virtual connector has the enable attribute set to 1. If the user
> wants to enable the writeback connector it is required to use the mkdir
> command, as follows:
> 
>   cd /mnt/vkms/connectors
>   mkdir Writeback
> 
> After the above command, the writeback connector will be enabled, and
> the user could see the following tree:
> 
> vkms/
>  |__connectors
>     |__Virtual
>     |   |__ enable
>     |__Writeback
>         |__ enable
> 
> If the user wants to remove the writeback connector, it is required to
> use the command rmdir, for example
> 
>   rmdir Writeback
> 
> Another way to enable and disable a connector it is by using the enable
> attribute, for example, we can disable the Virtual connector with:
> 
>   echo 0 > /mnt/vkms/connectors/Virtual/enable
> 
> And enable it again with:
> 
>   echo 1 > /mnt/vkms/connectors/Virtual/enable
> 
> It is important to highlight that configfs 'obey' the parameters used
> during the vkms load and does not allow users to remove a connector
> directory if it was load via module parameter. For example:
> 
>   modprobe vkms enable_writeback=1
> 
> vkms/
>  |__connectors
>     |__Virtual
>     |   |__ enable
>     |__Writeback
>         |__ enable
> 
> If the user tries to remove the Writeback connector with “rmdir
> Writeback”, the operation will be not permitted because the Writeback
> connector was loaded with the modules. However, the user may disable the
> writeback connector with:
> 
>   echo 0 > /mnt/vkms/connectors/Writeback/enable

I guess I should have put a warning into that task that step one is
designing the interface. Here's the fundamental thoughts:

- The _only_ thing we can hotplug after drm_dev_register() is a
  drm_connector. That's an interesting use-case, but atm not really
  supported by the vkms codebase. So we can't just enable/disable
  writeback like this. We also can't change _anything_ else in the drm
  driver like this.

- The other bit we want is support multiple vkms instances, to simulate
  multi-gpus and fun stuff like that.

- Therefore vkms configs should be at the drm_device level, so a
  directory under configfs/vkms/ represents an entire device.

- We need a special config item to control
  drm_dev_register/drm_dev_unregister. While a drm_device is registers,
  all other config items need to fail if userspace tries to change them.
  Maybe this should be a top-level "enable" property.

- Every time enable goes from 0 -> 1 we need to create a completely new
  vkms instance. The old one might still be around, waiting for the last
  few references to disappear.

- enable going from 1 -> 0 needs to be treated like a physical hotunplug,
  i.e. not drm_dev_unregister but drm_dev_unplug. We also need to annotate
  all the vkms code with drm_dev_enter/exit() as the kerneldoc of
  drm_dev_unplug explains.

- rmdir should be treated like enable going from 1 -> 0. Or maybe we
  should disable enable every going from 1 -> 0, would propably simplify
  everything.

- The initial instance created at module load also neeeds to be removable
  like this.

Once we have all this, then can we start to convert driver module options
over to configs and add cool features. But lots of infrastructure needed
first.

Also, we probably want some nasty testcases which races an rmdir in
configfs against userspace still doing ioctl calls against vkms. This is
ideal for validation the hotunplug infrastructure we have in drm.

An alternative is adding connector hotplugging. But I think before we do
that we need to have support for more crtc and more connectors as static
module options. So maybe not a good starting point for configfs.

The above text should probably be added to the vkms.rst todo item ...
-Daniel

> 
> 
> Rodrigo Siqueira (2):
>   drm/vkms: Add enable/disable functions per connector
>   drm/vkms: Introduce configfs for enabling/disabling connectors
> 
>  drivers/gpu/drm/vkms/Makefile         |   3 +-
>  drivers/gpu/drm/vkms/vkms_configfs.c  | 229 ++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_drv.c       |   6 +
>  drivers/gpu/drm/vkms/vkms_drv.h       |  17 ++
>  drivers/gpu/drm/vkms/vkms_output.c    |  84 ++++++----
>  drivers/gpu/drm/vkms/vkms_writeback.c |  31 +++-
>  6 files changed, 332 insertions(+), 38 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
> 
> -- 
> 2.21.0
> 
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists