by Brandon Ross Pollack

[permalink] [raw]

Subject:

Any progress on this? Is it ok if [email protected] and I do the
followups on this patch so that we can also submit the Hotplug patch I
wrote (that's now archived?).

2023-08-19 12:26:44

by Marius Vlad

[permalink] [raw]

Subject: Re: [PATCH v2 5/6] drm/vkms: Support enabling ConfigFS devices

Hi,

See below some minor comments:

On Fri, Jun 23, 2023 at 06:23:47PM -0400, Jim Shargo wrote:
> VKMS now supports creating and using virtual devices!
>
> In addition to the enabling logic, this commit also prevents users from
> adding new objects once a card is registered.
>
> Signed-off-by: Jim Shargo <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_configfs.c | 37 +++--
> drivers/gpu/drm/vkms/vkms_crtc.c | 4 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 3 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
> drivers/gpu/drm/vkms/vkms_output.c | 201 +++++++++++++++++++++++----
> 5 files changed, 202 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 544024735d19..f5eed6d23dcd 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -504,29 +504,40 @@ static ssize_t device_enabled_store(struct config_item *item, const char *buf,
> {
> struct vkms_configfs *configfs = item_to_configfs(item);
> struct vkms_device *device;
> - int value, ret;
> + int enabled, ret;
>
> - ret = kstrtoint(buf, 0, &value);
> + ret = kstrtoint(buf, 0, &enabled);
> if (ret)
> return ret;
>
> - if (value != 1)
> - return -EINVAL;
> -
> - mutex_lock(&configfs->lock);
> -
> - if (configfs->vkms_device) {
> + if (enabled == 0) {
> + mutex_lock(&configfs->lock);
> + if (configfs->vkms_device) {
> + vkms_remove_device(configfs->vkms_device);
> + configfs->vkms_device = NULL;
> + }
> mutex_unlock(&configfs->lock);
> +
> return len;
> }
>
> - device = vkms_add_device(configfs);
> - mutex_unlock(&configfs->lock);
> + if (enabled == 1) {
> + mutex_lock(&configfs->lock);
> + if (!configfs->vkms_device) {
> + device = vkms_add_device(configfs);
> + if (IS_ERR(device)) {
> + mutex_unlock(&configfs->lock);
> + return -PTR_ERR(device);
> + }
> +
> + configfs->vkms_device = device;
> + }
> + mutex_unlock(&configfs->lock);
>
> - if (IS_ERR(device))
> - return -PTR_ERR(device);
> + return len;
> + }
>
> - return len;
> + return -EINVAL;
> }
>
> CONFIGFS_ATTR(device_, enabled);
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index d91e49c53adc..5ebb5264f6ef 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -278,7 +278,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
>
> struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
> struct drm_plane *primary,
> - struct drm_plane *cursor)
> + struct drm_plane *cursor, const char *name)
> {
> struct drm_device *dev = &vkmsdev->drm;
> struct vkms_crtc *vkms_crtc;
> @@ -290,7 +290,7 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
> vkms_crtc = &vkmsdev->output.crtcs[vkmsdev->output.num_crtcs++];
>
> ret = drmm_crtc_init_with_planes(dev, &vkms_crtc->base, primary, cursor,
> - &vkms_crtc_funcs, NULL);
> + &vkms_crtc_funcs, name);
> if (ret) {
> DRM_ERROR("Failed to init CRTC\n");
> goto out_error;
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1b5b7143792f..314a04659c5f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -210,7 +210,7 @@ static int vkms_platform_probe(struct platform_device *pdev)
> ret = drm_dev_register(&vkms_device->drm, 0);
> if (ret) {
> DRM_ERROR("Unable to register device with id %d\n", pdev->id);
> - return ret;
> + goto out_release_group;
> }
>
> drm_fbdev_generic_setup(&vkms_device->drm, 0);
> @@ -256,6 +256,7 @@ struct vkms_device *vkms_add_device(struct vkms_configfs *configfs)
> dev, &vkms_platform_driver.driver))) {
> pdev = to_platform_device(dev);
> max_id = max(max_id, pdev->id);
> + put_device(dev);
> }
>
> pdev = platform_device_register_data(NULL, DRIVER_NAME, max_id + 1,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 3634eeeb4548..3d592d085f49 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -239,7 +239,7 @@ void vkms_remove_device(struct vkms_device *vkms_device);
> /* CRTC */
> struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
> struct drm_plane *primary,
> - struct drm_plane *cursor);
> + struct drm_plane *cursor, const char *name);
>
> int vkms_output_init(struct vkms_device *vkmsdev);
> int vkms_output_init_default(struct vkms_device *vkmsdev);
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index c9e6c653de19..806ff01954ad 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -2,8 +2,10 @@
>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_simple_kms_helper.h>
>
> @@ -82,7 +84,6 @@ static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device)
>
> int vkms_output_init_default(struct vkms_device *vkmsdev)
> {
> - struct vkms_output *output = &vkmsdev->output;
> struct drm_device *dev = &vkmsdev->drm;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> @@ -101,8 +102,7 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
> struct vkms_plane *overlay = vkms_plane_init(
> vkmsdev, DRM_PLANE_TYPE_OVERLAY);
> if (IS_ERR(overlay)) {
> - ret = PTR_ERR(overlay);
> - goto err_planes;
> + return PTR_ERR(overlay);
> }
> }
> }
> @@ -110,17 +110,16 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
> if (vkmsdev->config.cursor) {
> cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> if (IS_ERR(cursor)) {
> - ret = PTR_ERR(cursor);
> - goto err_planes;
> + return PTR_ERR(cursor);
> }
> }
>
> vkms_crtc = vkms_crtc_init(vkmsdev, &primary->base,
> - cursor ? &cursor->base : NULL);
> + cursor ? &cursor->base : NULL,
> + "crtc-default");
> if (IS_ERR(vkms_crtc)) {
> DRM_ERROR("Failed to init crtc\n");
> - ret = PTR_ERR(vkms_crtc);
> - goto err_planes;
> + return PTR_ERR(vkms_crtc);
> }
>
> for (int i = 0; i < vkmsdev->output.num_planes; i++) {
> @@ -131,22 +130,20 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
> connector = vkms_connector_init(vkmsdev);
> if (IS_ERR(connector)) {
> DRM_ERROR("Failed to init connector\n");
> - ret = PTR_ERR(connector);
> - goto err_connector;
> + return PTR_ERR(connector);
> }
>
> encoder = vkms_encoder_init(vkmsdev);
> if (IS_ERR(encoder)) {
> DRM_ERROR("Failed to init encoder\n");
> - ret = PTR_ERR(encoder);
> - goto err_encoder;
> + return PTR_ERR(encoder);
> }
> encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
>
> ret = drm_connector_attach_encoder(connector, encoder);
> if (ret) {
> DRM_ERROR("Failed to attach connector to encoder\n");
> - goto err_attach;
> + return ret;
> }
>
> if (vkmsdev->config.writeback) {
> @@ -158,27 +155,175 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
> drm_mode_config_reset(dev);
>
> return 0;
> +}
>
> -err_attach:
> - drm_encoder_cleanup(encoder);
> +static bool is_object_linked(struct vkms_config_links *links, unsigned long idx)
> +{
> + return links->linked_object_bitmap & (1 << idx);
> +}
>
> -err_encoder:
> - drm_connector_cleanup(connector);
> +int vkms_output_init(struct vkms_device *vkmsdev)
> +{
> + struct drm_device *dev = &vkmsdev->drm;
> + struct vkms_configfs *configfs = vkmsdev->configfs;
> + struct vkms_output *output = &vkmsdev->output;
> + struct plane_map {
> + struct vkms_config_plane *config_plane;
> + struct vkms_plane *plane;
> + } plane_map[VKMS_MAX_PLANES] = { 0 };
> + struct encoder_map {
> + struct vkms_config_encoder *config_encoder;
> + struct drm_encoder *encoder;
> + } encoder_map[VKMS_MAX_OUTPUT_OBJECTS] = { 0 };
> + struct config_item *item;
> + int map_idx = 0;
> +
> + list_for_each_entry(item, &configfs->planes_group.cg_children,
> + ci_entry) {
> + struct vkms_config_plane *config_plane =
> + item_to_config_plane(item);
> + struct vkms_plane *plane =
> + vkms_plane_init(vkmsdev, config_plane->type);
> +
> + if (IS_ERR(plane)) {
> + DRM_ERROR("Unable to init plane from config: %s",
> + item->ci_name);
> + return PTR_ERR(plane);
> + }
>
> -err_connector:
> - drm_crtc_cleanup(&vkms_crtc->base);
> + plane_map[map_idx].config_plane = config_plane;
> + plane_map[map_idx].plane = plane;
> + map_idx += 1;
> + }
>
> -err_planes:
> - for (int i = 0; i < output->num_planes; i++) {
> - drm_plane_cleanup(&output->planes[i].base);
> + map_idx = 0;
> + list_for_each_entry(item, &configfs->encoders_group.cg_children,
> + ci_entry) {
> + struct vkms_config_encoder *config_encoder =
> + item_to_config_encoder(item);
> + struct drm_encoder *encoder = vkms_encoder_init(vkmsdev);
> +
> + if (IS_ERR(encoder)) {
> + DRM_ERROR("Failed to init config encoder: %s",
> + item->ci_name);
> + return PTR_ERR(encoder);
> + }
> + encoder_map[map_idx].config_encoder = config_encoder;
A two connector configuration without an encoder would have the
potential of blowing up if we don't create a second_encoder.
> + encoder_map[map_idx].encoder = encoder;
> + map_idx += 1;
> }
>
> - memset(output, 0, sizeof(*output));
> + list_for_each_entry(item, &configfs->connectors_group.cg_children,
> + ci_entry) {
> + struct vkms_config_connector *config_connector =
> + item_to_config_connector(item);
> + struct drm_connector *connector = vkms_connector_init(vkmsdev);
>
> - return ret;
> -}
> + if (IS_ERR(connector)) {
> + DRM_ERROR("Failed to init connector from config: %s",
> + item->ci_name);
> + return PTR_ERR(connector);
> + }
>
> -int vkms_output_init(struct vkms_device *vkmsdev)
> -{
> - return -ENOTSUPP;
> + for (int j = 0; j < output->num_connectors; j++) {
> + struct encoder_map *encoder = &encoder_map[j];
> +
> + if (is_object_linked(
> + &config_connector->possible_encoders,
> + encoder->config_encoder
> + ->encoder_config_idx)) {
Here encoder->config_encoder for two connectors but a single encoder
will give back a empty encoder.
> + drm_connector_attach_encoder(connector,
> + encoder->encoder);
> + }
> + }
> + }
> +
> + list_for_each_entry(item, &configfs->crtcs_group.cg_children,
> + ci_entry) {
> + struct vkms_config_crtc *config_crtc =
> + item_to_config_crtc(item);
> + struct vkms_crtc *vkms_crtc;
> + struct drm_plane *primary = NULL, *cursor = NULL;
> +
> + for (int j = 0; j < output->num_planes; j++) {
> + struct plane_map *plane_entry = &plane_map[j];
> + struct drm_plane *plane = &plane_entry->plane->base;
> +
> + if (!is_object_linked(
> + &plane_entry->config_plane->possible_crtcs,
> + config_crtc->crtc_config_idx)) {
> + continue;
> + }
> +
> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> + if (primary) {
> + DRM_WARN(
> + "Too many primary planes found for crtc %s.",
> + item->ci_name);
> + return EINVAL;
> + }
> + primary = plane;
> + } else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> + if (cursor) {
> + DRM_WARN(
> + "Too many cursor planes found for crtc %s.",
> + item->ci_name);
> + return EINVAL;
> + }
> + cursor = plane;
> + }
> + }
> +
> + if (!primary) {
> + DRM_WARN("No primary plane configured for crtc %s",
> + item->ci_name);
> + return EINVAL;
> + }
> +
> + vkms_crtc =
> + vkms_crtc_init(vkmsdev, primary, cursor, item->ci_name);
> + if (IS_ERR(vkms_crtc)) {
> + DRM_WARN("Unable to init crtc from config: %s",
> + item->ci_name);
> + return PTR_ERR(vkms_crtc);
> + }
> +
> + for (int j = 0; j < output->num_planes; j++) {
> + struct plane_map *plane_entry = &plane_map[j];
> +
> + if (!plane_entry->plane)
> + break;
> +
> + if (is_object_linked(
> + &plane_entry->config_plane->possible_crtcs,
> + config_crtc->crtc_config_idx)) {
> + plane_entry->plane->base.possible_crtcs |=
> + drm_crtc_mask(&vkms_crtc->base);
> + }
> + }
> +
> + for (int j = 0; j < output->num_encoders; j++) {
> + struct encoder_map *encoder_entry = &encoder_map[j];
> +
> + if (is_object_linked(&encoder_entry->config_encoder
> + ->possible_crtcs,
> + config_crtc->crtc_config_idx)) {
> + encoder_entry->encoder->possible_crtcs |=
> + drm_crtc_mask(&vkms_crtc->base);
> + }
> + }
> +
> + if (vkmsdev->config.writeback) {
> + int ret = vkms_enable_writeback_connector(vkmsdev,
> + vkms_crtc);
> + if (ret)
> + DRM_WARN(
> + "Failed to init writeback connector for config crtc: %s. Error code %d",
> + item->ci_name, ret);
> + }
> + }
I think we might be needing here a test for missing symlinks - invalid
configurations, like unused crtcs, encoders, connectors. I
suppose anything that's not matched with is_object_linked(),
such we avoid invalid configuration found by drm_mode_config_validate()
and inform users about missing them.

An example here would be to create a second encoder, connector, crtc and
not symlink them to their possible_encoders,possible_crtcs mask. An
i-g-t test for this very thing would be needed to stress different kind
of combinations.

> +
> + drm_mode_config_reset(dev);
> +
> + return 0;
> }
> --
> 2.41.0.162.gfafddb0af9-goog
>

Attachments:

(No filename) (13.75 kB)
signature.asc (849.00 B)
Download all attachments

2023-08-20 10:08:04

by Brandon Ross Pollack

[permalink] [raw]

Subject: Re: [PATCH v2 6/6] drm/vkms: Add a module param to enable/disable the default device

On 6/26/23 03:04, Maira Canal wrote:
> Hi Jim,
>
> On 6/23/23 19:23, Jim Shargo wrote:
>> In many testing circumstances, we will want to just create a new device
>> and test against that. If we create a default device, it can be annoying
>> to have to manually select the new device instead of choosing the only
>> one that exists.
>>
>> The param, enable_default, is defaulted to true to maintain backwards
>> compatibility.
>>
>> Signed-off-by: Jim Shargo <[email protected]>
>> ---
>> drivers/gpu/drm/vkms/vkms_drv.c | 44 ++++++++++++++++++++++-----------
>> 1 file changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c
>> b/drivers/gpu/drm/vkms/vkms_drv.c
>> index 314a04659c5f..1cb56c090a65 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>> @@ -42,17 +42,26 @@
>> #define DRIVER_MAJOR    1
>> #define DRIVER_MINOR    0
>> +static bool enable_default_device = true;
>> +module_param_named(enable_default_device, enable_default_device,
>> bool, 0444);
>> +MODULE_PARM_DESC(enable_default_device,
>> +         "Enable/Disable creating the default device");
>
> Wouldn't be better to just call it "enable_default"?

At the risk of being annoyingly pedantic, I actually like the original
name better because it makes it clear it is an entire device and setup,

including the planes/encoders/connectors/crtcs needed and connecting
them as necessary etc. I just feel "device" here makes it clearer what
is the default thing.

>
> Also, could you add this parameter to vkms_config debugfs file?

But of course! This is the last comment before I send out the new series.

Thank you so much for your time, looking forward to the next round of
comments.

>
> Best Regards,
> - Maíra
>
>> +
>> static bool enable_cursor = true;
>> module_param_named(enable_cursor, enable_cursor, bool, 0444);
>> -MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>> +MODULE_PARM_DESC(enable_cursor,
>> +         "Enable/Disable cursor support for the default device");
>> static bool enable_writeback = true;
>> module_param_named(enable_writeback, enable_writeback, bool, 0444);
>> -MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback
>> connector support");
>> +MODULE_PARM_DESC(
>> +    enable_writeback,
>> +    "Enable/Disable writeback connector support for the default
>> device");
>> static bool enable_overlay;
>> module_param_named(enable_overlay, enable_overlay, bool, 0444);
>> -MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
>> +MODULE_PARM_DESC(enable_overlay,
>> +         "Enable/Disable overlay support for the default device");
>> DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>> @@ -278,10 +287,7 @@ void vkms_remove_device(struct vkms_device
>> *vkms_device)
>> static int __init vkms_init(void)
>> {
>>       int ret;
>> -    struct platform_device *pdev;
>> -    struct vkms_device_setup vkms_device_setup = {
>> -        .configfs = NULL,
>> -    };
>> +    struct platform_device *default_pdev = NULL;
>>       ret = platform_driver_register(&vkms_platform_driver);
>>       if (ret) {
>> @@ -289,19 +295,27 @@ static int __init vkms_init(void)
>>           return ret;
>>       }
>> -    pdev = platform_device_register_data(NULL, DRIVER_NAME, 0,
>> -                         &vkms_device_setup,
>> -                         sizeof(vkms_device_setup));
>> -    if (IS_ERR(pdev)) {
>> -        DRM_ERROR("Unable to register default vkms device\n");
>> -        platform_driver_unregister(&vkms_platform_driver);
>> -        return PTR_ERR(pdev);
>> +    if (enable_default_device) {
>> +        struct vkms_device_setup vkms_device_setup = {
>> +            .configfs = NULL,
>> +        };
>> +
>> +        default_pdev = platform_device_register_data(
>> +            NULL, DRIVER_NAME, 0, &vkms_device_setup,
>> +            sizeof(vkms_device_setup));
>> +        if (IS_ERR(default_pdev)) {
>> +            DRM_ERROR("Unable to register default vkms device\n");
>> + platform_driver_unregister(&vkms_platform_driver);
>> +            return PTR_ERR(default_pdev);
>> +        }
>>       }
>>       ret = vkms_init_configfs();
>>       if (ret) {
>>           DRM_ERROR("Unable to initialize configfs\n");
>> -        platform_device_unregister(pdev);
>> +        if (default_pdev)
>> +            platform_device_unregister(default_pdev);
>> +
>>           platform_driver_unregister(&vkms_platform_driver);
>>       }
>
> From mboxrd@z Thu Jan 1 00:00:00 1970
> Return-Path: <[email protected]>
> X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on
>     aws-us-west-2-korg-lkml-1.web.codeaurora.org
> Received: from gabe.freedesktop.org (gabe.freedesktop.org
> [131.252.210.177])
>     (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
> bits))
>     (No client certificate requested)
>     by smtp.lore.kernel.org (Postfix) with ESMTPS id AB561C0015E
>     for <[email protected]>; Sun, 25 Jun 2023 18:04:15
> +0000 (UTC)
> Received: from gabe.freedesktop.org (localhost [127.0.0.1])
>     by gabe.freedesktop.org (Postfix) with ESMTP id D153310E18C;
>     Sun, 25 Jun 2023 18:04:14 +0000 (UTC)
> Received: from mx1.riseup.net (mx1.riseup.net [198.252.153.129])
> by gabe.freedesktop.org (Postfix) with ESMTPS id C6BE910E187
> for <[email protected]>; Sun, 25 Jun 2023 18:04:12 +0000
> (UTC)
> Received: from fews01-sea.riseup.net (fews01-sea-pn.riseup.net
> [10.0.1.109])
> (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
> key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest
> SHA256)
> (No client certificate requested)
> by mx1.riseup.net (Postfix) with ESMTPS id 4QpzPl6CHTzDrNy;
> Sun, 25 Jun 2023 18:04:11 +0000 (UTC)
> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=riseup.net;
> s=squak;
> t=1687716252; bh=3cOd9Tulf+RLZ2R/lGuVfEdERy8xqZ4HHWjkWZv3FAs=;
> h=Date:Subject:To:Cc:References:From:In-Reply-To:From;
> b=lXentHTOo29YwgA/Q5WAgBBEUsl5DTqsmOJd4wsjxBgZocgZ/PG9hmyBm6a2IjvcQ
> jSPI26UWubdYOe7kngWhcU0/oO570ofVUyrxiNgiJQfcscdp5bpwrskBKK4yXdJc0q
> 2YM4+n0xeBBSr2pFLMDwbOsLDvaGUK77nSPcbPXQ=
> X-Riseup-User-ID:
> 7EBE90DED2258EE1CA830B796CBA05FD63338D890F31F4C1BCCCB27A6DA77A56
> Received: from [127.0.0.1] (localhost [127.0.0.1])
> by fews01-sea.riseup.net (Postfix) with ESMTPSA id 4QpzPg4YpbzJntB;
> Sun, 25 Jun 2023 18:04:07 +0000 (UTC)
> Message-ID: <[email protected]>
> Date: Sun, 25 Jun 2023 15:04:05 -0300
> MIME-Version: 1.0
> Subject: Re: [PATCH v2 6/6] drm/vkms: Add a module param to
> enable/disable the
> default device
> Content-Language: en-US
> To: Jim Shargo <[email protected]>, Daniel Vetter <[email protected]>,
> David Airlie <[email protected]>, Haneen Mohammed
> <[email protected]>,
> Jonathan Corbet <[email protected]>,
> Maarten Lankhorst <[email protected]>,
> Maxime Ripard <[email protected]>, Melissa Wen <[email protected]>,
> Rodrigo Siqueira <[email protected]>,
> Thomas Zimmermann <[email protected]>
> References: <[email protected]>
> <[email protected]>
> From: Maira Canal <[email protected]>
> In-Reply-To: <[email protected]>
> Content-Type: text/plain; charset=UTF-8; format=flowed
> Content-Transfer-Encoding: 8bit
> X-BeenThere: [email protected]
> X-Mailman-Version: 2.1.29
> Precedence: list
> List-Id: Direct Rendering Infrastructure - Development
> <dri-devel.lists.freedesktop.org>
> List-Unsubscribe:
> <https://lists.freedesktop.org/mailman/options/dri-devel>,
> <mailto:[email protected]?subject=unsubscribe>
> List-Archive: <https://lists.freedesktop.org/archives/dri-devel>
> List-Post: <mailto:[email protected]>
> List-Help: <mailto:[email protected]?subject=help>
> List-Subscribe:
> <https://lists.freedesktop.org/mailman/listinfo/dri-devel>,
> <mailto:[email protected]?subject=subscribe>
> Cc: [email protected], [email protected],
> [email protected]
> Errors-To: [email protected]
> Sender: "dri-devel" <[email protected]>
>
> Hi Jim,
>
> On 6/23/23 19:23, Jim Shargo wrote:
>> In many testing circumstances, we will want to just create a new device
>> and test against that. If we create a default device, it can be annoying
>> to have to manually select the new device instead of choosing the only
>> one that exists.
>>
>> The param, enable_default, is defaulted to true to maintain backwards
>> compatibility.
>>
>> Signed-off-by: Jim Shargo <[email protected]>
>> ---
>> drivers/gpu/drm/vkms/vkms_drv.c | 44 ++++++++++++++++++++++-----------
>> 1 file changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c
>> b/drivers/gpu/drm/vkms/vkms_drv.c
>> index 314a04659c5f..1cb56c090a65 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>> @@ -42,17 +42,26 @@
>> #define DRIVER_MAJOR    1
>> #define DRIVER_MINOR    0
>> +static bool enable_default_device = true;
>> +module_param_named(enable_default_device, enable_default_device,
>> bool, 0444);
>> +MODULE_PARM_DESC(enable_default_device,
>> +         "Enable/Disable creating the default device");
>
> Wouldn't be better to just call it "enable_default"?
>
> Also, could you add this parameter to vkms_config debugfs file?
>
> Best Regards,
> - Maíra
>
>> +
>> static bool enable_cursor = true;
>> module_param_named(enable_cursor, enable_cursor, bool, 0444);
>> -MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>> +MODULE_PARM_DESC(enable_cursor,
>> +         "Enable/Disable cursor support for the default device");
>> static bool enable_writeback = true;
>> module_param_named(enable_writeback, enable_writeback, bool, 0444);
>> -MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback
>> connector support");
>> +MODULE_PARM_DESC(
>> +    enable_writeback,
>> +    "Enable/Disable writeback connector support for the default
>> device");
>> static bool enable_overlay;
>> module_param_named(enable_overlay, enable_overlay, bool, 0444);
>> -MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
>> +MODULE_PARM_DESC(enable_overlay,
>> +         "Enable/Disable overlay support for the default device");
>> DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>> @@ -278,10 +287,7 @@ void vkms_remove_device(struct vkms_device
>> *vkms_device)
>> static int __init vkms_init(void)
>> {
>>       int ret;
>> -    struct platform_device *pdev;
>> -    struct vkms_device_setup vkms_device_setup = {
>> -        .configfs = NULL,
>> -    };
>> +    struct platform_device *default_pdev = NULL;
>>       ret = platform_driver_register(&vkms_platform_driver);
>>       if (ret) {
>> @@ -289,19 +295,27 @@ static int __init vkms_init(void)
>>           return ret;
>>       }
>> -    pdev = platform_device_register_data(NULL, DRIVER_NAME, 0,
>> -                         &vkms_device_setup,
>> -                         sizeof(vkms_device_setup));
>> -    if (IS_ERR(pdev)) {
>> -        DRM_ERROR("Unable to register default vkms device\n");
>> -        platform_driver_unregister(&vkms_platform_driver);
>> -        return PTR_ERR(pdev);
>> +    if (enable_default_device) {
>> +        struct vkms_device_setup vkms_device_setup = {
>> +            .configfs = NULL,
>> +        };
>> +
>> +        default_pdev = platform_device_register_data(
>> +            NULL, DRIVER_NAME, 0, &vkms_device_setup,
>> +            sizeof(vkms_device_setup));
>> +        if (IS_ERR(default_pdev)) {
>> +            DRM_ERROR("Unable to register default vkms device\n");
>> + platform_driver_unregister(&vkms_platform_driver);
>> +            return PTR_ERR(default_pdev);
>> +        }
>>       }
>>       ret = vkms_init_configfs();
>>       if (ret) {
>>           DRM_ERROR("Unable to initialize configfs\n");
>> -        platform_device_unregister(pdev);
>> +        if (default_pdev)
>> +            platform_device_unregister(default_pdev);
>> +
>>           platform_driver_unregister(&vkms_platform_driver);
>>       }
>