| Message ID | 20240827-google-vkms-managed-v2-1-f41104553aeb@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Switch all vkms object to DRM managed objects | expand |
Hi, On Tue, Aug 27, 2024 at 11:57:36AM GMT, Louis Chauvet wrote: > The current VKMS driver uses non-managed function to create connectors. It > is not an issue yet, but in order to support multiple devices easily, > convert this code to use drm and device managed helpers. > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > --- > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++---------- > 2 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 5e46ea5b96dc..9a3c6c34d1f6 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -99,7 +99,6 @@ struct vkms_crtc_state { > struct vkms_output { > struct drm_crtc crtc; > struct drm_encoder encoder; > - struct drm_connector connector; > struct drm_writeback_connector wb_connector; > struct hrtimer vblank_hrtimer; > ktime_t period_ns; > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > index 5ce70dd946aa..4fe6b88e8081 100644 > --- a/drivers/gpu/drm/vkms/vkms_output.c > +++ b/drivers/gpu/drm/vkms/vkms_output.c > @@ -3,11 +3,11 @@ > #include "vkms_drv.h" > #include <drm/drm_atomic_helper.h> > #include <drm/drm_edid.h> > +#include <drm/drm_managed.h> > #include <drm/drm_probe_helper.h> > > static const struct drm_connector_funcs vkms_connector_funcs = { > .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > .reset = drm_atomic_helper_connector_reset, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > { > struct vkms_output *output = &vkmsdev->output; > struct drm_device *dev = &vkmsdev->drm; > - struct drm_connector *connector = &output->connector; > + struct drm_connector *connector; > struct drm_encoder *encoder = &output->encoder; > struct drm_crtc *crtc = &output->crtc; > struct vkms_plane *primary, *cursor = NULL; > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > if (ret) > return ret; > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > - DRM_MODE_CONNECTOR_VIRTUAL); > + connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); > + if (!connector) { > + DRM_ERROR("Failed to allocate connector\n"); > + ret = -ENOMEM; > + goto err_connector; > + } > + I think it would be worth explaining why you need to move to a separate allocation for the connector now. Maxime
Le 27/08/24 - 15:15, Maxime Ripard a écrit : > Hi, > > On Tue, Aug 27, 2024 at 11:57:36AM GMT, Louis Chauvet wrote: > > The current VKMS driver uses non-managed function to create connectors. It > > is not an issue yet, but in order to support multiple devices easily, > > convert this code to use drm and device managed helpers. > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > --- > > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++---------- > > 2 files changed, 12 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index 5e46ea5b96dc..9a3c6c34d1f6 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -99,7 +99,6 @@ struct vkms_crtc_state { > > struct vkms_output { > > struct drm_crtc crtc; > > struct drm_encoder encoder; > > - struct drm_connector connector; > > struct drm_writeback_connector wb_connector; > > struct hrtimer vblank_hrtimer; > > ktime_t period_ns; > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > index 5ce70dd946aa..4fe6b88e8081 100644 > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > @@ -3,11 +3,11 @@ > > #include "vkms_drv.h" > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_edid.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_probe_helper.h> > > > > static const struct drm_connector_funcs vkms_connector_funcs = { > > .fill_modes = drm_helper_probe_single_connector_modes, > > - .destroy = drm_connector_cleanup, > > .reset = drm_atomic_helper_connector_reset, > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > { > > struct vkms_output *output = &vkmsdev->output; > > struct drm_device *dev = &vkmsdev->drm; > > - struct drm_connector *connector = &output->connector; > > + struct drm_connector *connector; > > struct drm_encoder *encoder = &output->encoder; > > struct drm_crtc *crtc = &output->crtc; > > struct vkms_plane *primary, *cursor = NULL; > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > if (ret) > > return ret; > > > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > - DRM_MODE_CONNECTOR_VIRTUAL); > > + connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); > > + if (!connector) { > > + DRM_ERROR("Failed to allocate connector\n"); > > + ret = -ENOMEM; > > + goto err_connector; > > + } > > + > > I think it would be worth explaining why you need to move to a separate > allocation for the connector now. > > Maxime Hi, This is in preparation for ConfigFS implementation, as the number of connector/encoders/crtc/planes... will be dynamic, we need to have separate alloaction. If I add this paragraph in the commit message, is it sufficient? A specific allocation for the connector is not strictly necessary at this point, but in order to implement dynamic configuration of VKMS (configFS), it will be easier to have one allocation per connector. (same for encoder & CRTC) Thanks, Louis Chauvet
On Tue, Aug 27, 2024 at 03:24:10PM GMT, Louis Chauvet wrote: > Le 27/08/24 - 15:15, Maxime Ripard a écrit : > > Hi, > > > > On Tue, Aug 27, 2024 at 11:57:36AM GMT, Louis Chauvet wrote: > > > The current VKMS driver uses non-managed function to create connectors. It > > > is not an issue yet, but in order to support multiple devices easily, > > > convert this code to use drm and device managed helpers. > > > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > > --- > > > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > > > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++---------- > > > 2 files changed, 12 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > > index 5e46ea5b96dc..9a3c6c34d1f6 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > > @@ -99,7 +99,6 @@ struct vkms_crtc_state { > > > struct vkms_output { > > > struct drm_crtc crtc; > > > struct drm_encoder encoder; > > > - struct drm_connector connector; > > > struct drm_writeback_connector wb_connector; > > > struct hrtimer vblank_hrtimer; > > > ktime_t period_ns; > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > > index 5ce70dd946aa..4fe6b88e8081 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > > @@ -3,11 +3,11 @@ > > > #include "vkms_drv.h" > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_edid.h> > > > +#include <drm/drm_managed.h> > > > #include <drm/drm_probe_helper.h> > > > > > > static const struct drm_connector_funcs vkms_connector_funcs = { > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > - .destroy = drm_connector_cleanup, > > > .reset = drm_atomic_helper_connector_reset, > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > { > > > struct vkms_output *output = &vkmsdev->output; > > > struct drm_device *dev = &vkmsdev->drm; > > > - struct drm_connector *connector = &output->connector; > > > + struct drm_connector *connector; > > > struct drm_encoder *encoder = &output->encoder; > > > struct drm_crtc *crtc = &output->crtc; > > > struct vkms_plane *primary, *cursor = NULL; > > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > if (ret) > > > return ret; > > > > > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > > - DRM_MODE_CONNECTOR_VIRTUAL); > > > + connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); > > > + if (!connector) { > > > + DRM_ERROR("Failed to allocate connector\n"); > > > + ret = -ENOMEM; > > > + goto err_connector; > > > + } > > > + > > > > I think it would be worth explaining why you need to move to a separate > > allocation for the connector now. > > > > Maxime > > Hi, > > This is in preparation for ConfigFS implementation, as the number of > connector/encoders/crtc/planes... will be dynamic, we need to have > separate alloaction. > > If I add this paragraph in the commit message, is it sufficient? > > A specific allocation for the connector is not strictly necessary > at this point, but in order to implement dynamic configuration of > VKMS (configFS), it will be easier to have one allocation per > connector. > > (same for encoder & CRTC) Yeah, that's a good message, but it probably belongs in a separate patch then. Thanks! Maxime
Le 27/08/24 - 16:39, Maxime Ripard a écrit : > On Tue, Aug 27, 2024 at 03:24:10PM GMT, Louis Chauvet wrote: > > Le 27/08/24 - 15:15, Maxime Ripard a écrit : > > > Hi, > > > > > > On Tue, Aug 27, 2024 at 11:57:36AM GMT, Louis Chauvet wrote: > > > > The current VKMS driver uses non-managed function to create connectors. It > > > > is not an issue yet, but in order to support multiple devices easily, > > > > convert this code to use drm and device managed helpers. > > > > > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > > > --- > > > > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > > > > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++---------- > > > > 2 files changed, 12 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > > > index 5e46ea5b96dc..9a3c6c34d1f6 100644 > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > > > @@ -99,7 +99,6 @@ struct vkms_crtc_state { > > > > struct vkms_output { > > > > struct drm_crtc crtc; > > > > struct drm_encoder encoder; > > > > - struct drm_connector connector; > > > > struct drm_writeback_connector wb_connector; > > > > struct hrtimer vblank_hrtimer; > > > > ktime_t period_ns; > > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > > > index 5ce70dd946aa..4fe6b88e8081 100644 > > > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > > > @@ -3,11 +3,11 @@ > > > > #include "vkms_drv.h" > > > > #include <drm/drm_atomic_helper.h> > > > > #include <drm/drm_edid.h> > > > > +#include <drm/drm_managed.h> > > > > #include <drm/drm_probe_helper.h> > > > > > > > > static const struct drm_connector_funcs vkms_connector_funcs = { > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > - .destroy = drm_connector_cleanup, > > > > .reset = drm_atomic_helper_connector_reset, > > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > > { > > > > struct vkms_output *output = &vkmsdev->output; > > > > struct drm_device *dev = &vkmsdev->drm; > > > > - struct drm_connector *connector = &output->connector; > > > > + struct drm_connector *connector; > > > > struct drm_encoder *encoder = &output->encoder; > > > > struct drm_crtc *crtc = &output->crtc; > > > > struct vkms_plane *primary, *cursor = NULL; > > > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > > if (ret) > > > > return ret; > > > > > > > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > > > - DRM_MODE_CONNECTOR_VIRTUAL); > > > > + connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); > > > > + if (!connector) { > > > > + DRM_ERROR("Failed to allocate connector\n"); > > > > + ret = -ENOMEM; > > > > + goto err_connector; > > > > + } > > > > + > > > > > > I think it would be worth explaining why you need to move to a separate > > > allocation for the connector now. > > > > > > Maxime > > > > Hi, > > > > This is in preparation for ConfigFS implementation, as the number of > > connector/encoders/crtc/planes... will be dynamic, we need to have > > separate alloaction. > > > > If I add this paragraph in the commit message, is it sufficient? > > > > A specific allocation for the connector is not strictly necessary > > at this point, but in order to implement dynamic configuration of > > VKMS (configFS), it will be easier to have one allocation per > > connector. > > > > (same for encoder & CRTC) > > Yeah, that's a good message, but it probably belongs in a separate patch > then. Can you explain what you mean by "in a separate patch"? I wanted to write this paragraph in the commit log. Louis Chauvet
On Tue, Aug 27, 2024 at 05:08:08PM GMT, Louis Chauvet wrote: > Le 27/08/24 - 16:39, Maxime Ripard a écrit : > > On Tue, Aug 27, 2024 at 03:24:10PM GMT, Louis Chauvet wrote: > > > Le 27/08/24 - 15:15, Maxime Ripard a écrit : > > > > Hi, > > > > > > > > On Tue, Aug 27, 2024 at 11:57:36AM GMT, Louis Chauvet wrote: > > > > > The current VKMS driver uses non-managed function to create connectors. It > > > > > is not an issue yet, but in order to support multiple devices easily, > > > > > convert this code to use drm and device managed helpers. > > > > > > > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > > > > --- > > > > > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > > > > > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++---------- > > > > > 2 files changed, 12 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > > > > index 5e46ea5b96dc..9a3c6c34d1f6 100644 > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > > > > @@ -99,7 +99,6 @@ struct vkms_crtc_state { > > > > > struct vkms_output { > > > > > struct drm_crtc crtc; > > > > > struct drm_encoder encoder; > > > > > - struct drm_connector connector; > > > > > struct drm_writeback_connector wb_connector; > > > > > struct hrtimer vblank_hrtimer; > > > > > ktime_t period_ns; > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > > > > index 5ce70dd946aa..4fe6b88e8081 100644 > > > > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > > > > @@ -3,11 +3,11 @@ > > > > > #include "vkms_drv.h" > > > > > #include <drm/drm_atomic_helper.h> > > > > > #include <drm/drm_edid.h> > > > > > +#include <drm/drm_managed.h> > > > > > #include <drm/drm_probe_helper.h> > > > > > > > > > > static const struct drm_connector_funcs vkms_connector_funcs = { > > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > > - .destroy = drm_connector_cleanup, > > > > > .reset = drm_atomic_helper_connector_reset, > > > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > > > { > > > > > struct vkms_output *output = &vkmsdev->output; > > > > > struct drm_device *dev = &vkmsdev->drm; > > > > > - struct drm_connector *connector = &output->connector; > > > > > + struct drm_connector *connector; > > > > > struct drm_encoder *encoder = &output->encoder; > > > > > struct drm_crtc *crtc = &output->crtc; > > > > > struct vkms_plane *primary, *cursor = NULL; > > > > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > > > if (ret) > > > > > return ret; > > > > > > > > > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > > > > - DRM_MODE_CONNECTOR_VIRTUAL); > > > > > + connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); > > > > > + if (!connector) { > > > > > + DRM_ERROR("Failed to allocate connector\n"); > > > > > + ret = -ENOMEM; > > > > > + goto err_connector; > > > > > + } > > > > > + > > > > > > > > I think it would be worth explaining why you need to move to a separate > > > > allocation for the connector now. > > > > > > > > Maxime > > > > > > Hi, > > > > > > This is in preparation for ConfigFS implementation, as the number of > > > connector/encoders/crtc/planes... will be dynamic, we need to have > > > separate alloaction. > > > > > > If I add this paragraph in the commit message, is it sufficient? > > > > > > A specific allocation for the connector is not strictly necessary > > > at this point, but in order to implement dynamic configuration of > > > VKMS (configFS), it will be easier to have one allocation per > > > connector. > > > > > > (same for encoder & CRTC) > > > > Yeah, that's a good message, but it probably belongs in a separate patch > > then. > > Can you explain what you mean by "in a separate patch"? I wanted to write > this paragraph in the commit log. You're doing two things in that patch: converting to drmm like you documented in your original commit log, and switching from having the connector (encoder, and crtc) from vkms_output to being dynamically allocated. Ideally, you should have one patch to switch from being part of vkms_output to a dynamic allocation (with the commit log you suggested above), and one patch to switch to drmm with your original commit log. Maxime
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 5e46ea5b96dc..9a3c6c34d1f6 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -99,7 +99,6 @@ struct vkms_crtc_state { struct vkms_output { struct drm_crtc crtc; struct drm_encoder encoder; - struct drm_connector connector; struct drm_writeback_connector wb_connector; struct hrtimer vblank_hrtimer; ktime_t period_ns; diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index 5ce70dd946aa..4fe6b88e8081 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -3,11 +3,11 @@ #include "vkms_drv.h" #include <drm/drm_atomic_helper.h> #include <drm/drm_edid.h> +#include <drm/drm_managed.h> #include <drm/drm_probe_helper.h> static const struct drm_connector_funcs vkms_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) { struct vkms_output *output = &vkmsdev->output; struct drm_device *dev = &vkmsdev->drm; - struct drm_connector *connector = &output->connector; + struct drm_connector *connector; struct drm_encoder *encoder = &output->encoder; struct drm_crtc *crtc = &output->crtc; struct vkms_plane *primary, *cursor = NULL; @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) if (ret) return ret; - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, - DRM_MODE_CONNECTOR_VIRTUAL); + connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); + if (!connector) { + DRM_ERROR("Failed to allocate connector\n"); + ret = -ENOMEM; + goto err_connector; + } + + ret = drmm_connector_init(dev, connector, &vkms_connector_funcs, + DRM_MODE_CONNECTOR_VIRTUAL, NULL); if (ret) { DRM_ERROR("Failed to init connector\n"); goto err_connector; @@ -93,7 +100,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) { DRM_ERROR("Failed to init encoder\n"); - goto err_encoder; + return ret; } encoder->possible_crtcs = 1; @@ -115,12 +122,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) err_attach: drm_encoder_cleanup(encoder); - -err_encoder: - drm_connector_cleanup(connector); - err_connector: drm_crtc_cleanup(crtc); - return ret; }
The current VKMS driver uses non-managed function to create connectors. It is not an issue yet, but in order to support multiple devices easily, convert this code to use drm and device managed helpers. Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> --- drivers/gpu/drm/vkms/vkms_drv.h | 1 - drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-)