| Message ID | 20240814-google-vkms-managed-v1-1-7ab8b8921103@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Switch all vkms object to DRM managed objects | expand |
Hi Louis, Thanks for these patches. Easy review as my configfs series included similar patches :) I think that this series could be easily rebased on drm-misc-next, but I don't know if that'd add a lot of work rebasing other series. I'd be nice to get these 4 merged. > 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> Reviewed-by: José Expósito <jose.exposito89@gmail.com> > --- > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++------------ > 2 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index f74a5c2045f9..cea7b2640c5d 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -43,7 +43,6 @@ > 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 20073a00b200..4767838c3a73 100644 > --- a/drivers/gpu/drm/vkms/vkms_output.c > +++ b/drivers/gpu/drm/vkms/vkms_output.c > @@ -3,6 +3,7 @@ > #include <drm/drm_atomic_helper.h> > #include <drm/drm_edid.h> > #include <drm/drm_probe_helper.h> > +#include <drm/drm_managed.h> Nit: Keep includes sorted alphabetically if possible > > #include "vkms_writeback.h" > #include "vkms_plane.h" > @@ -10,7 +11,6 @@ > > 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, > @@ -54,7 +54,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_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; > @@ -90,11 +90,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) > if (ret) > return ret; > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > - DRM_MODE_CONNECTOR_VIRTUAL); > + connector = drmm_kzalloc(&vkmsdev->drm, sizeof(*connector), GFP_KERNEL); "&vkmsdev->drm" can be replaced with "dev". > + if (!connector) For consistency with the init function, it'd be nice to log this error as well: DRM_ERROR("Failed to allocate connector\n"); > + return -ENOMEM; > + > + 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; > + return ret; > } > > drm_connector_helper_add(connector, &vkms_conn_helper_funcs); > @@ -103,7 +107,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) > DRM_MODE_ENCODER_VIRTUAL, NULL); > if (ret) { > DRM_ERROR("Failed to init encoder\n"); > - goto err_encoder; > + return ret; > } > /* > * This is an hardcoded value to select crtc for the encoder. > @@ -130,11 +134,5 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) > err_attach: > drm_encoder_cleanup(encoder); > > -err_encoder: > - drm_connector_cleanup(connector); > - > -err_connector: > - drm_crtc_cleanup(crtc); > - I think that, technically, err_encoder should call drm_crtc_cleanup() in this patch. However, since a future patch will remove this code I don't find it that relevant. > return ret; > }
Hi Louis, On 8/20/24 06:09, José Expósito wrote: > Hi Louis, > > Thanks for these patches. Easy review as my configfs series included similar > patches :) I think that this series could be easily rebased on drm-misc-next, > but I don't know if that'd add a lot of work rebasing other series. I'd be nice > to get these 4 merged. > I'm with José on this one. If you rebase it on top of drm-misc-next, I can review faster and merge this patches right away. Best Regards, - Maíra >> 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> > > Reviewed-by: José Expósito <jose.exposito89@gmail.com> > >> --- >> drivers/gpu/drm/vkms/vkms_drv.h | 1 - >> drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++------------ >> 2 files changed, 10 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h >> index f74a5c2045f9..cea7b2640c5d 100644 >> --- a/drivers/gpu/drm/vkms/vkms_drv.h >> +++ b/drivers/gpu/drm/vkms/vkms_drv.h >> @@ -43,7 +43,6 @@ >> 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 20073a00b200..4767838c3a73 100644 >> --- a/drivers/gpu/drm/vkms/vkms_output.c >> +++ b/drivers/gpu/drm/vkms/vkms_output.c >> @@ -3,6 +3,7 @@ >> #include <drm/drm_atomic_helper.h> >> #include <drm/drm_edid.h> >> #include <drm/drm_probe_helper.h> >> +#include <drm/drm_managed.h> > > Nit: Keep includes sorted alphabetically if possible > >> >> #include "vkms_writeback.h" >> #include "vkms_plane.h" >> @@ -10,7 +11,6 @@ >> >> 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, >> @@ -54,7 +54,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_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; >> @@ -90,11 +90,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) >> if (ret) >> return ret; >> >> - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, >> - DRM_MODE_CONNECTOR_VIRTUAL); >> + connector = drmm_kzalloc(&vkmsdev->drm, sizeof(*connector), GFP_KERNEL); > > "&vkmsdev->drm" can be replaced with "dev". > >> + if (!connector) > > For consistency with the init function, it'd be nice to log this error as well: > > DRM_ERROR("Failed to allocate connector\n"); > >> + return -ENOMEM; >> + >> + 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; >> + return ret; >> } >> >> drm_connector_helper_add(connector, &vkms_conn_helper_funcs); >> @@ -103,7 +107,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) >> DRM_MODE_ENCODER_VIRTUAL, NULL); >> if (ret) { >> DRM_ERROR("Failed to init encoder\n"); >> - goto err_encoder; >> + return ret; >> } >> /* >> * This is an hardcoded value to select crtc for the encoder. >> @@ -130,11 +134,5 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) >> err_attach: >> drm_encoder_cleanup(encoder); >> >> -err_encoder: >> - drm_connector_cleanup(connector); >> - >> -err_connector: >> - drm_crtc_cleanup(crtc); >> - > > I think that, technically, err_encoder should call drm_crtc_cleanup() in this > patch. However, since a future patch will remove this code I don't find it that > relevant. > >> return ret; >> }
Le 20/08/24 - 11:09, José Expósito a écrit : > Hi Louis, > > Thanks for these patches. Easy review as my configfs series included similar > patches :) I think that this series could be easily rebased on drm-misc-next, > but I don't know if that'd add a lot of work rebasing other series. I'd be nice > to get these 4 merged. It is not trivial to rebase before the split-header series. I will see if I can rebase this+split header easly, but I fear few conflicts. > > 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> > > Reviewed-by: José Expósito <jose.exposito89@gmail.com> Thanks! I will test my v2 and send it tomorrow! > > --- > > drivers/gpu/drm/vkms/vkms_drv.h | 1 - > > drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++------------ > > 2 files changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index f74a5c2045f9..cea7b2640c5d 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -43,7 +43,6 @@ > > 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 20073a00b200..4767838c3a73 100644 > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > @@ -3,6 +3,7 @@ > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_edid.h> > > #include <drm/drm_probe_helper.h> > > +#include <drm/drm_managed.h> > > Nit: Keep includes sorted alphabetically if possible > > > > > #include "vkms_writeback.h" > > #include "vkms_plane.h" > > @@ -10,7 +11,6 @@ > > > > 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, > > @@ -54,7 +54,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_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; > > @@ -90,11 +90,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) > > if (ret) > > return ret; > > > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > - DRM_MODE_CONNECTOR_VIRTUAL); > > + connector = drmm_kzalloc(&vkmsdev->drm, sizeof(*connector), GFP_KERNEL); > > "&vkmsdev->drm" can be replaced with "dev". > > > + if (!connector) > > For consistency with the init function, it'd be nice to log this error as well: > > DRM_ERROR("Failed to allocate connector\n"); > > > + return -ENOMEM; > > + > > + 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; > > + return ret; > > } > > > > drm_connector_helper_add(connector, &vkms_conn_helper_funcs); > > @@ -103,7 +107,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) > > DRM_MODE_ENCODER_VIRTUAL, NULL); > > if (ret) { > > DRM_ERROR("Failed to init encoder\n"); > > - goto err_encoder; > > + return ret; > > } > > /* > > * This is an hardcoded value to select crtc for the encoder. > > @@ -130,11 +134,5 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) > > err_attach: > > drm_encoder_cleanup(encoder); > > > > -err_encoder: > > - drm_connector_cleanup(connector); > > - > > -err_connector: > > - drm_crtc_cleanup(crtc); > > - > > I think that, technically, err_encoder should call drm_crtc_cleanup() in this > patch. However, since a future patch will remove this code I don't find it that > relevant. True, I will keep it to avoid issues. I believe it is a remnant of my many rebases. > > return ret; > > }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index f74a5c2045f9..cea7b2640c5d 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -43,7 +43,6 @@ 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 20073a00b200..4767838c3a73 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -3,6 +3,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_edid.h> #include <drm/drm_probe_helper.h> +#include <drm/drm_managed.h> #include "vkms_writeback.h" #include "vkms_plane.h" @@ -10,7 +11,6 @@ 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, @@ -54,7 +54,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_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; @@ -90,11 +90,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) if (ret) return ret; - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, - DRM_MODE_CONNECTOR_VIRTUAL); + connector = drmm_kzalloc(&vkmsdev->drm, sizeof(*connector), GFP_KERNEL); + if (!connector) + return -ENOMEM; + + 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; + return ret; } drm_connector_helper_add(connector, &vkms_conn_helper_funcs); @@ -103,7 +107,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_index) DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) { DRM_ERROR("Failed to init encoder\n"); - goto err_encoder; + return ret; } /* * This is an hardcoded value to select crtc for the encoder. @@ -130,11 +134,5 @@ int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc_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, 10 insertions(+), 13 deletions(-)