On Thu, Feb 13, 2025 at 02:59:52PM +0100, Louis Chauvet wrote:
> On 11/02/25 - 12:09, José Expósito wrote:
> > From: Louis Chauvet <louis.chau...@bootlin.com>
> >
> > As the configuration will be used by userspace, add a validator to avoid
> > creating a broken DRM device.
> >
> > For the moment, the function always returns true, but rules will be
> > added in future patches.
> >
> > Reviewed-by: Louis Chauvet <louis.chau...@bootlin.com>
> > Signed-off-by: Louis Chauvet <louis.chau...@bootlin.com>
> > Co-developed-by: José Expósito <jose.exposit...@gmail.com>
> > Signed-off-by: José Expósito <jose.exposit...@gmail.com>
>
> The compilation is broken when building as module:
>
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c
> b/drivers/gpu/drm/vkms/vkms_config.c
> index b9267aef4804..82335006c94a 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -55,6 +55,7 @@ bool vkms_config_is_valid(struct vkms_config *config)
> {
> return true;
> }
> +EXPORT_SYMBOL_IF_KUNIT(vkms_config_is_valid);
Fixed the issue in all patches, thanks!
> > [...]
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_config.h
> > b/drivers/gpu/drm/vkms/vkms_config.h
> > index fcaa909fb2e0..0376dceaf6be 100644
> > --- a/drivers/gpu/drm/vkms/vkms_config.h
> > +++ b/drivers/gpu/drm/vkms/vkms_config.h
> > @@ -67,6 +67,16 @@ vkms_config_get_device_name(struct vkms_config *config)
> > return config->dev_name;
> > }
> >
> > +/**
> > + * vkms_config_is_valid() - Validate a configuration
> > + * @config: Configuration to validate
> > + *
> > + * Returns:
> > + * Whether the configuration is valid or not.
> > + * For example, a configuration without primary planes is not valid.
> > + */
> > +bool vkms_config_is_valid(struct vkms_config *config);
> > +
>
> I think here we can take a const pointer.
>
> > /**
> > * vkms_config_register_debugfs() - Register a debugfs file to show the
> > device's
> > * configuration
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h
> > b/drivers/gpu/drm/vkms/vkms_drv.h
> > index a74a7fc3a056..95afc39ce985 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -204,7 +204,7 @@ struct vkms_config;
> > struct vkms_device {
> > struct drm_device drm;
> > struct platform_device *platform;
> > - const struct vkms_config *config;
> > + struct vkms_config *config;
>
> So we can keep a const pointer here (for me the device should never modify
> vkms_config)
I tryed keeping the const pointer, but, since list_count_nodes() is used in
several valid_* functions and it takes a non-const pointer, it causes
warnings.
We can fix them with a cast:
n_planes = list_count_nodes((struct list_head *)&config->planes);
But I feel that keeping the "const" creates more issues than it solves.
Anyway, if you prefer this pointer to be const, I will change it in v3.
Jose
> > };
> >
> > /*
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c
> > b/drivers/gpu/drm/vkms/vkms_output.c
> > index 068a7f87ecec..414cc933af41 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -16,6 +16,9 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > int writeback;
> > unsigned int n;
> >
> > + if (!vkms_config_is_valid(vkmsdev->config))
> > + return -EINVAL;
> > +
> > /*
> > * Initialize used plane. One primary plane is required to perform the
> > composition.
> > *
> > --
> > 2.48.1
> >
