On Wed, Feb 12, 2025 at 03:10:49PM +0100, Louis Chauvet wrote:
>
>
> Le 11/02/2025 à 11:47, José Expósito a écrit :
> > On Thu, Jan 30, 2025 at 02:48:21PM +0100, Louis Chauvet wrote:
> > > On 29/01/25 - 12:00, José Expósito wrote:
> > > > Add a list of possible CRTCs to the plane configuration and helpers to
> > > > attach, detach and get the primary and cursor planes attached to a CRTC.
> > > >
> > > > Now that the default configuration has its planes and CRTC correctly
> > > > attached, configure the output following the configuration.
> > > >
> > > > Signed-off-by: Louis Chauvet <louis.chau...@bootlin.com>
> > > > Signed-off-by: José Expósito <jose.exposit...@gmail.com>
> > >
> > > Co-developped-by: Louis Chauvet <louis.chau...@bootlin.com>
> > > Signed-off-by: Louis Chauvet <louis.chau...@bootlin.com>
> > > Signed-off-by: José Expósito <jose.exposit...@gmail.com>
> > >
> > > [...]
> > >
> > > > +int __must_check vkms_config_plane_attach_crtc(struct
> > > > vkms_config_plane *plane_cfg,
> > > > + struct vkms_config_crtc
> > > > *crtc_cfg)
> > > > +{
> > > > + struct vkms_config_crtc *possible_crtc;
> > > > + unsigned long idx = 0;
> > > > + u32 crtc_idx = 0;
> > > > +
> > > > + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc) {
> > > > + if (possible_crtc == crtc_cfg)
> > > > + return -EINVAL;
> > >
> > > Is it really an error? After this call, we expect plane and crtc to be
> > > attached, so if the plane is already attached, I don't see any issue.
> >
> > In my opinion, this could be either handled as an error or not. I think that
> > there are arguments for both approaches but, for our use case, I think that
> > it
> > is better to return an error.
> >
> > Since the main (and for the moment only) user of this function will be
> > configfs,
> > it is very convenient to return an error to avoid creating 2 links between
> > plane <-> crtc.
> >
> > If we allow to create multiple links, and the user deletes one of them, the
> > items would be still linked, which is a bit unexpected.
> >
> > The same applies to the other vkms_config_*_attach_* functions.
>
> I see your reasoning, I did not think about this issue.
> We can keep the error, but can we use `EEXIST` to reflect better the status?
Very good point, I'll change it to EEXIST in v3.
> And I just think about it, maybe we can add here the check "is the crtc part
> of the same config as the plane" (and return EINVAL if not)? It will remove
> the need to do the check in configFS + avoid errors for future users of
> vkms_config.
Another excellent point. Since we can not use the vkms_config_plane.plane and
vkms_config_crtc.crtc pointers to check if they belong to the same device, the
only solution I see is adding a pointer to the parent vkms_config to every
structure (vkms_config_plane, vkms_config_crtc, etc).
In this way we can do something like:
if (plane_cfg->config != crtc_cfg->config)
return -EINVAL;
I tried with container_of(), but I don't think it'll work with the current data
structure.
Unless you can think of a better solution, I'll implement this in v3.
Thanks for the great comments,
Jose
> > For these reasons, I didn't change it in v2, let me know your opinion.
> > Jose
> >
> > > > + }
> > > > +
> > > > + return xa_alloc(&plane_cfg->possible_crtcs, &crtc_idx, crtc_cfg,
> > > > + xa_limit_32b, GFP_KERNEL);
> > > > +}
> > > > +
> > >
> > > [...]
> > >
> > > > +struct vkms_config_crtc **vkms_config_plane_get_possible_crtcs(struct
> > > > vkms_config_plane *plane_cfg,
> > > > + size_t
> > > > *out_length)
> > > > +{
> > > > + struct vkms_config_crtc **array;
> > > > + struct vkms_config_crtc *possible_crtc;
> > > > + unsigned long idx;
> > > > + size_t length = 0;
> > > > + int n = 0;
> > > > +
> > > > + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc)
> > > > + length++;
> > > > +
> > > > + if (length == 0) {
> > > > + *out_length = length;
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + array = kmalloc_array(length, sizeof(*array), GFP_KERNEL);
> > > > + if (!array)
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc) {
> > > > + array[n] = possible_crtc;
> > > > + n++;
> > > > + }
> > > > +
> > > > + *out_length = length;
> > > > + return array;
> > > > +}
> > >
> > > Same as before, can we use an iterator?
> > >
> > > > +static struct vkms_config_plane *vkms_config_crtc_get_plane(const
> > > > struct vkms_config *config,
> > > > + struct
> > > > vkms_config_crtc *crtc_cfg,
> > > > + enum
> > > > drm_plane_type type)
> > >
> > > Even if this is a private function, can we add a comment explaning that
> > > the returned value is only one of the available planes of this type?
> > >
> > > /**
> > > * vkms_config_crtc_get_plane() - Get the first attached plane
> > > * found of a specific type
> > > * @config: configuration containing the crtc and the planes
> > > * @crtc_cfg: Only find planes attached to this CRTC
> > > * @type: Plane type to search
> > > *
> > > * Returns:
> > > * The first plane found attached to @crtc_cfg with the type
> > > * @type.
> > > */
> > >
> > > > +{
> > > > + struct vkms_config_plane *plane_cfg;
> > > > + struct vkms_config_crtc *possible_crtc;
> > > > + enum drm_plane_type current_type;
> > > > + unsigned long idx;
> > > > +
> > > > + list_for_each_entry(plane_cfg, &config->planes, link) {
> > > > + current_type = vkms_config_plane_get_type(plane_cfg);
> > > > +
> > > > + xa_for_each(&plane_cfg->possible_crtcs, idx,
> > > > possible_crtc) {
> > > > + if (possible_crtc == crtc_cfg && current_type
> > > > == type)
> > > > + return plane_cfg;
> > > > + }
> > > > + }
> > > > +
> > > > + return NULL;
> > > > +}
> > >
> > > [...]
> > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_config.h
> > > > b/drivers/gpu/drm/vkms/vkms_config.h
> > >
> > > [...]
> > >
> > > > +/**
> > > > + * vkms_config_crtc_primary_plane() - Return the primary plane for a
> > > > CRTC
> > > > + * @config: Configuration containing the CRTC
> > > > + * @crtc_config: Target CRTC
> > > > + *
> > > > + * Returns:
> > > > + * The primary plane or NULL if none is assigned yet.
> > > > + */
> > >
> > > Same as above, can you speficy that it is one of the primary plane?
> > >
> > > > +struct vkms_config_plane *vkms_config_crtc_primary_plane(const struct
> > > > vkms_config *config,
> > > > + struct
> > > > vkms_config_crtc *crtc_cfg);
> > > > +
> > > > +/**
> > > > + * vkms_config_crtc_cursor_plane() - Return the cursor plane for a CRTC
> > >
> > > Ditto
> > >
> > > [...]
> > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c
> > > > b/drivers/gpu/drm/vkms/vkms_output.c
> > >
> > > [...]
> > >
> > > > @@ -35,19 +41,54 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > > > ret = PTR_ERR(plane_cfg->plane);
> > > > goto err_free;
> > > > }
> > > > + }
> > > > +
> > > > + for (n = 0; n < n_crtcs; n++) {
> > > > + struct vkms_config_crtc *crtc_cfg;
> > > > + struct vkms_config_plane *primary, *cursor;
> > > > +
> > > > + crtc_cfg = crtc_cfgs[n];
> > > > + primary =
> > > > vkms_config_crtc_primary_plane(vkmsdev->config, crtc_cfg);
> > > > + cursor = vkms_config_crtc_cursor_plane(vkmsdev->config,
> > > > crtc_cfg);
> > >
> > > Linked with a previous comment: here we have no garantee that primary is a
> > > valid pointer, can we check it or call vkms_config_is_valid to ensure it?
> > >
> > > > + crtc_cfg->crtc = vkms_crtc_init(dev,
> > > > &primary->plane->base,
> > > > + cursor ?
> > > > &cursor->plane->base : NULL);
> > > > + if (IS_ERR(crtc_cfg->crtc)) {
> > > > + DRM_ERROR("Failed to allocate CRTC\n");
> > > > + ret = PTR_ERR(crtc_cfg->crtc);
> > > > + goto err_free;
> > > > + }
> > >
> > > [...]
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
