| Message ID | 20181022122718.girdp4ybkw3wrsmn@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/vkms: Add writeback support | expand |
On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote: > Hi, > > I started to work for add the writeback feature into the VKMS, and this > RFC represents my first draft. As a result, I have some questions which > I list below. > > 1. In this patch, I replaced the virtual connector by the writeback > connector. Is it a good idea? Or should I keep both connectors? If I > keep both connectors, should I add a module parameter to enable the > writeback? Yeah separate connector is imo better, with module config knob. Eventually we can move that into configfs or similar. > 2. When I added the writeback connector (in > drm_writeback_connector_init()), I noticed that I had to indicate the > struct drm_encoder_helper_funcs. However, I think that VKMS does not > need anything sophisticated related to encoders at the writeback > perspective. Am I right? Or did I missed something? Yeah, you don't need anything fancy in the drm_encoder. I think you could reuse the same dummy drm_encoder as we use for the normal connector. > 3. I just discovered that the writeback connectors are not exposed as a > connector for the userspace, as a result, IGT tests do not work and > Wayland is unable to run as well. Simon Ser and Daniel Stone, explained > to me that I have to update the compositor (in my case Weston) to set > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do > not have experience with userspace, anyone can point me out another > aspect that I have to take a look to make the writeback work in the user > space? Also, Is it possible to update the IGT test or do I need to > create a new set of tests? Yeah, writeback connector doesn't work like a normal connector, so it's hidden by default. There should be special writeback connector tests in igt though, but I guess those haven't been merged yet. Ask Liviu and Brian from arm where they're stuck. Without those it's going to be rather hard to make this work correctly. Cheers, Daniel > > Thanks! > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > --- > drivers/gpu/drm/vkms/vkms_crc.c | 9 ++- > drivers/gpu/drm/vkms/vkms_crtc.c | 2 + > drivers/gpu/drm/vkms/vkms_drv.h | 3 +- > drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++-------- > 4 files changed, 89 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > index 9d9e8146db90..5d9ac45e00f2 100644 > --- a/drivers/gpu/drm/vkms/vkms_crc.c > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > @@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc, > } > > static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > - struct vkms_crc_data *cursor_crc) > + struct vkms_crc_data *cursor_crc, > + struct vkms_output *out) > { > struct drm_framebuffer *fb = &primary_crc->fb; > struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); > @@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > } > > memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size); > + // TODO: Remove this copy from crc, and make this sort of operation > + // generic since we can use in different places > + memcpy(out->connector.base.state->writeback_job->fb, > + vkms_obj->vaddr, vkms_obj->gem.size); > mutex_unlock(&vkms_obj->pages_lock); > > if (cursor_crc) > @@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work) > } > > if (primary_crc) > - crc32 = _vkms_get_crc(primary_crc, cursor_crc); > + crc32 = _vkms_get_crc(primary_crc, cursor_crc, out); > > frame_end = drm_crtc_accurate_vblank_count(crtc); > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > index 177bbcb38306..d33ddcc40a29 100644 > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > @@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > _vblank_handle(output); > > + drm_writeback_signal_completion(&output->connector, 0); > + > ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, > output->period_ns); > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 1c93990693e3..1e8184ad872f 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -5,6 +5,7 @@ > #include <drm/drm.h> > #include <drm/drm_gem.h> > #include <drm/drm_encoder.h> > +#include <drm/drm_writeback.h> > #include <linux/hrtimer.h> > > #define XRES_MIN 20 > @@ -61,7 +62,7 @@ struct vkms_crtc_state { > struct vkms_output { > struct drm_crtc crtc; > struct drm_encoder encoder; > - struct drm_connector connector; > + struct drm_writeback_connector connector; > struct hrtimer vblank_hrtimer; > ktime_t period_ns; > struct drm_pending_vblank_event *event; > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > index 271a0eb9042c..46622d012107 100644 > --- a/drivers/gpu/drm/vkms/vkms_output.c > +++ b/drivers/gpu/drm/vkms/vkms_output.c > @@ -9,6 +9,13 @@ > #include "vkms_drv.h" > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_writeback.h> > + > +#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state) > + > +struct vkms_writeback_connector_state { > + struct drm_connector_state base; > +}; > > static void vkms_connector_destroy(struct drm_connector *connector) > { > @@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector) > drm_connector_cleanup(connector); > } > > +static void vkms_connector_reset(struct drm_connector *connector) > +{ > + struct vkms_writeback_connector_state *wb_state = > + kzalloc(sizeof(*wb_state), GFP_KERNEL); > + > + if (connector->state) > + __drm_atomic_helper_connector_destroy_state(connector->state); > + > + kfree(connector->state); > + __drm_atomic_helper_connector_reset(connector, &wb_state->base); > +} > + > +static struct drm_connector_state * > +vkms_connector_duplicate_state(struct drm_connector *connector) > +{ > + struct vkms_writeback_connector_state *wb_state; > + > + if (WARN_ON(!connector->state)) > + return NULL; > + > + wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL); > + if (!wb_state) > + return NULL; > + > + __drm_atomic_helper_connector_duplicate_state(connector, > + &wb_state->base); > + > + return &wb_state->base; > +} > + > +static enum drm_connector_status > +vkms_connector_detect(struct drm_connector *connector, bool force) > +{ > + return connector_status_connected; > +} > + > static const struct drm_connector_funcs vkms_connector_funcs = { > + .reset = vkms_connector_reset, > + .detect = vkms_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > .destroy = vkms_connector_destroy, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_duplicate_state = vkms_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > @@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { > .get_modes = vkms_conn_get_modes, > }; > > +static int vkms_encoder_atomic_check(struct drm_encoder *encoder, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct drm_framebuffer *fb; > + > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + return 0; > + > + fb = conn_state->writeback_job->fb; > + if (fb->width != crtc_state->mode.hdisplay || > + fb->height != crtc_state->mode.vdisplay) { > + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", > + fb->width, fb->height); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = { > + .atomic_check = vkms_encoder_atomic_check, > +}; > + > int vkms_output_init(struct vkms_device *vkmsdev) > { > struct vkms_output *output = &vkmsdev->output; > struct drm_device *dev = &vkmsdev->drm; > - struct drm_connector *connector = &output->connector; > - struct drm_encoder *encoder = &output->encoder; > + struct drm_writeback_connector *connector = &output->connector; > struct drm_crtc *crtc = &output->crtc; > struct drm_plane *primary, *cursor = NULL; > int ret; > @@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev) > if (ret) > goto err_crtc; > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > - DRM_MODE_CONNECTOR_VIRTUAL); > + ret = drm_writeback_connector_init(dev, connector, > + &vkms_connector_funcs, > + &vkms_encoder_helper_funcs, > + vkms_formats, 1); > if (ret) { > DRM_ERROR("Failed to init connector\n"); > goto err_connector; > } > > - drm_connector_helper_add(connector, &vkms_conn_helper_funcs); > + drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs); > > - ret = drm_connector_register(connector); > + ret = drm_connector_register(&connector->base); > if (ret) { > DRM_ERROR("Failed to register connector\n"); > goto err_connector_register; > } > > - ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs, > - DRM_MODE_ENCODER_VIRTUAL, NULL); > - if (ret) { > - DRM_ERROR("Failed to init encoder\n"); > - goto err_encoder; > - } > - encoder->possible_crtcs = 1; > - > - ret = drm_connector_attach_encoder(connector, encoder); > - if (ret) { > - DRM_ERROR("Failed to attach connector to encoder\n"); > - goto err_attach; > - } > - > drm_mode_config_reset(dev); > > return 0; > > -err_attach: > - drm_encoder_cleanup(encoder); > - > -err_encoder: > - drm_connector_unregister(connector); > - > err_connector_register: > - drm_connector_cleanup(connector); > + drm_connector_cleanup(&connector->base); > > err_connector: > drm_crtc_cleanup(crtc); > -- > 2.19.1 > > > -- > Rodrigo Siqueira > https://siqueira.tech > https://twitter.com/siqueirajordao > Graduate Student > Department of Computer Science > University of São Paulo
On 10/22, Daniel Vetter wrote: > On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote: > > Hi, > > > > I started to work for add the writeback feature into the VKMS, and this > > RFC represents my first draft. As a result, I have some questions which > > I list below. > > > > 1. In this patch, I replaced the virtual connector by the writeback > > connector. Is it a good idea? Or should I keep both connectors? If I > > keep both connectors, should I add a module parameter to enable the > > writeback? > > Yeah separate connector is imo better, with module config knob. Eventually > we can move that into configfs or similar. > > > 2. When I added the writeback connector (in > > drm_writeback_connector_init()), I noticed that I had to indicate the > > struct drm_encoder_helper_funcs. However, I think that VKMS does not > > need anything sophisticated related to encoders at the writeback > > perspective. Am I right? Or did I missed something? > > Yeah, you don't need anything fancy in the drm_encoder. I think you could > reuse the same dummy drm_encoder as we use for the normal connector. > > > 3. I just discovered that the writeback connectors are not exposed as a > > connector for the userspace, as a result, IGT tests do not work and > > Wayland is unable to run as well. Simon Ser and Daniel Stone, explained > > to me that I have to update the compositor (in my case Weston) to set > > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do > > not have experience with userspace, anyone can point me out another > > aspect that I have to take a look to make the writeback work in the user > > space? Also, Is it possible to update the IGT test or do I need to > > create a new set of tests? > > Yeah, writeback connector doesn't work like a normal connector, so it's > hidden by default. There should be special writeback connector tests in > igt though, but I guess those haven't been merged yet. Ask Liviu and Brian > from arm where they're stuck. Without those it's going to be rather hard > to make this work correctly. Hi, thanks for the feedbacks I will improve the code based on your comments, and for the testing mechanism, I want to try to make rootston run on top of VKMS. Best Regards > Cheers, Daniel > > > > > Thanks! > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > --- > > drivers/gpu/drm/vkms/vkms_crc.c | 9 ++- > > drivers/gpu/drm/vkms/vkms_crtc.c | 2 + > > drivers/gpu/drm/vkms/vkms_drv.h | 3 +- > > drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++-------- > > 4 files changed, 89 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > > index 9d9e8146db90..5d9ac45e00f2 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > > @@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc, > > } > > > > static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > > - struct vkms_crc_data *cursor_crc) > > + struct vkms_crc_data *cursor_crc, > > + struct vkms_output *out) > > { > > struct drm_framebuffer *fb = &primary_crc->fb; > > struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); > > @@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > > } > > > > memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size); > > + // TODO: Remove this copy from crc, and make this sort of operation > > + // generic since we can use in different places > > + memcpy(out->connector.base.state->writeback_job->fb, > > + vkms_obj->vaddr, vkms_obj->gem.size); > > mutex_unlock(&vkms_obj->pages_lock); > > > > if (cursor_crc) > > @@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work) > > } > > > > if (primary_crc) > > - crc32 = _vkms_get_crc(primary_crc, cursor_crc); > > + crc32 = _vkms_get_crc(primary_crc, cursor_crc, out); > > > > frame_end = drm_crtc_accurate_vblank_count(crtc); > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > > index 177bbcb38306..d33ddcc40a29 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > > @@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > > > _vblank_handle(output); > > > > + drm_writeback_signal_completion(&output->connector, 0); > > + > > ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, > > output->period_ns); > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index 1c93990693e3..1e8184ad872f 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -5,6 +5,7 @@ > > #include <drm/drm.h> > > #include <drm/drm_gem.h> > > #include <drm/drm_encoder.h> > > +#include <drm/drm_writeback.h> > > #include <linux/hrtimer.h> > > > > #define XRES_MIN 20 > > @@ -61,7 +62,7 @@ struct vkms_crtc_state { > > struct vkms_output { > > struct drm_crtc crtc; > > struct drm_encoder encoder; > > - struct drm_connector connector; > > + struct drm_writeback_connector connector; > > struct hrtimer vblank_hrtimer; > > ktime_t period_ns; > > struct drm_pending_vblank_event *event; > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > index 271a0eb9042c..46622d012107 100644 > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > @@ -9,6 +9,13 @@ > > #include "vkms_drv.h" > > #include <drm/drm_crtc_helper.h> > > #include <drm/drm_atomic_helper.h> > > +#include <drm/drm_writeback.h> > > + > > +#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state) > > + > > +struct vkms_writeback_connector_state { > > + struct drm_connector_state base; > > +}; > > > > static void vkms_connector_destroy(struct drm_connector *connector) > > { > > @@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector) > > drm_connector_cleanup(connector); > > } > > > > +static void vkms_connector_reset(struct drm_connector *connector) > > +{ > > + struct vkms_writeback_connector_state *wb_state = > > + kzalloc(sizeof(*wb_state), GFP_KERNEL); > > + > > + if (connector->state) > > + __drm_atomic_helper_connector_destroy_state(connector->state); > > + > > + kfree(connector->state); > > + __drm_atomic_helper_connector_reset(connector, &wb_state->base); > > +} > > + > > +static struct drm_connector_state * > > +vkms_connector_duplicate_state(struct drm_connector *connector) > > +{ > > + struct vkms_writeback_connector_state *wb_state; > > + > > + if (WARN_ON(!connector->state)) > > + return NULL; > > + > > + wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL); > > + if (!wb_state) > > + return NULL; > > + > > + __drm_atomic_helper_connector_duplicate_state(connector, > > + &wb_state->base); > > + > > + return &wb_state->base; > > +} > > + > > +static enum drm_connector_status > > +vkms_connector_detect(struct drm_connector *connector, bool force) > > +{ > > + return connector_status_connected; > > +} > > + > > static const struct drm_connector_funcs vkms_connector_funcs = { > > + .reset = vkms_connector_reset, > > + .detect = vkms_connector_detect, > > .fill_modes = drm_helper_probe_single_connector_modes, > > .destroy = vkms_connector_destroy, > > - .reset = drm_atomic_helper_connector_reset, > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > + .atomic_duplicate_state = vkms_connector_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > }; > > > > @@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { > > .get_modes = vkms_conn_get_modes, > > }; > > > > +static int vkms_encoder_atomic_check(struct drm_encoder *encoder, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) > > +{ > > + struct drm_framebuffer *fb; > > + > > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > > + return 0; > > + > > + fb = conn_state->writeback_job->fb; > > + if (fb->width != crtc_state->mode.hdisplay || > > + fb->height != crtc_state->mode.vdisplay) { > > + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", > > + fb->width, fb->height); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = { > > + .atomic_check = vkms_encoder_atomic_check, > > +}; > > + > > int vkms_output_init(struct vkms_device *vkmsdev) > > { > > struct vkms_output *output = &vkmsdev->output; > > struct drm_device *dev = &vkmsdev->drm; > > - struct drm_connector *connector = &output->connector; > > - struct drm_encoder *encoder = &output->encoder; > > + struct drm_writeback_connector *connector = &output->connector; > > struct drm_crtc *crtc = &output->crtc; > > struct drm_plane *primary, *cursor = NULL; > > int ret; > > @@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev) > > if (ret) > > goto err_crtc; > > > > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > - DRM_MODE_CONNECTOR_VIRTUAL); > > + ret = drm_writeback_connector_init(dev, connector, > > + &vkms_connector_funcs, > > + &vkms_encoder_helper_funcs, > > + vkms_formats, 1); > > if (ret) { > > DRM_ERROR("Failed to init connector\n"); > > goto err_connector; > > } > > > > - drm_connector_helper_add(connector, &vkms_conn_helper_funcs); > > + drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs); > > > > - ret = drm_connector_register(connector); > > + ret = drm_connector_register(&connector->base); > > if (ret) { > > DRM_ERROR("Failed to register connector\n"); > > goto err_connector_register; > > } > > > > - ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs, > > - DRM_MODE_ENCODER_VIRTUAL, NULL); > > - if (ret) { > > - DRM_ERROR("Failed to init encoder\n"); > > - goto err_encoder; > > - } > > - encoder->possible_crtcs = 1; > > - > > - ret = drm_connector_attach_encoder(connector, encoder); > > - if (ret) { > > - DRM_ERROR("Failed to attach connector to encoder\n"); > > - goto err_attach; > > - } > > - > > drm_mode_config_reset(dev); > > > > return 0; > > > > -err_attach: > > - drm_encoder_cleanup(encoder); > > - > > -err_encoder: > > - drm_connector_unregister(connector); > > - > > err_connector_register: > > - drm_connector_cleanup(connector); > > + drm_connector_cleanup(&connector->base); > > > > err_connector: > > drm_crtc_cleanup(crtc); > > -- > > 2.19.1 > > > > > > -- > > Rodrigo Siqueira > > https://siqueira.tech > > https://twitter.com/siqueirajordao > > Graduate Student > > Department of Computer Science > > University of São Paulo > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Hi Rodrigo, On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote: >Hi, > >I started to work for add the writeback feature into the VKMS, and this >RFC represents my first draft. As a result, I have some questions which >I list below. > >1. In this patch, I replaced the virtual connector by the writeback >connector. Is it a good idea? Or should I keep both connectors? If I >keep both connectors, should I add a module parameter to enable the >writeback? > >2. When I added the writeback connector (in >drm_writeback_connector_init()), I noticed that I had to indicate the >struct drm_encoder_helper_funcs. However, I think that VKMS does not >need anything sophisticated related to encoders at the writeback >perspective. Am I right? Or did I missed something? > >3. I just discovered that the writeback connectors are not exposed as a >connector for the userspace, as a result, IGT tests do not work and >Wayland is unable to run as well. Simon Ser and Daniel Stone, explained >to me that I have to update the compositor (in my case Weston) to set >DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do >not have experience with userspace, anyone can point me out another >aspect that I have to take a look to make the writeback work in the user >space? Also, Is it possible to update the IGT test or do I need to >create a new set of tests? Alex added support for writeback connectors into drm_hwcomposer - you can see that here[1] There's also some igt tests like Daniel mentioned - I think Liviu already pointed you at those. Some more review below, Cheers! -Brian [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 > >Thanks! > >Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> >--- > drivers/gpu/drm/vkms/vkms_crc.c | 9 ++- > drivers/gpu/drm/vkms/vkms_crtc.c | 2 + > drivers/gpu/drm/vkms/vkms_drv.h | 3 +- > drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++-------- > 4 files changed, 89 insertions(+), 32 deletions(-) > >diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c >index 9d9e8146db90..5d9ac45e00f2 100644 >--- a/drivers/gpu/drm/vkms/vkms_crc.c >+++ b/drivers/gpu/drm/vkms/vkms_crc.c >@@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc, > } > > static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, >- struct vkms_crc_data *cursor_crc) >+ struct vkms_crc_data *cursor_crc, >+ struct vkms_output *out) > { > struct drm_framebuffer *fb = &primary_crc->fb; > struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); >@@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > } > > memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size); >+ // TODO: Remove this copy from crc, and make this sort of operation >+ // generic since we can use in different places >+ memcpy(out->connector.base.state->writeback_job->fb, >+ vkms_obj->vaddr, vkms_obj->gem.size); This looks like it would crash terribly, you're copying the pixel data (from vkms_obj->vaddr) into a "struct drm_framebuffer *" which is a pointer to the struct, rather than somewhere to put the actual pixels. The same way you get the vaddr for the input framebuffer, you need to get the vaddr for the output framebuffer too - via drm_gem_fb_get_obj() and then converting that to a vkms_gem_object. Then use that vaddr as the destination of the memcpy. However, see my comments at the bottom about drm_writeback_queue_job(). You might be better getting and storing the vaddr during atomic_commit(), rather than in this function. In order for this to be a straight memcpy, you need to make sure that the input and output buffers have the same size, pixel format, same stride etc. as well so you'll need to check that in atomic_check(). Once it's working you can relax those restrictions by copying a line at a time or doing pixel format conversion - but I don't think it's necessary to start with. There might be an opportunity to skip a copy here too - if you have a framebuffer attached to the writeback connector, you can skip allocating "vaddr_out" and just compose the cursor directly into the output framebuffer, and calculate the CRC on that. That's definitely an optimisation rather than a requirement though. > mutex_unlock(&vkms_obj->pages_lock); > > if (cursor_crc) >@@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work) > } > > if (primary_crc) >- crc32 = _vkms_get_crc(primary_crc, cursor_crc); >+ crc32 = _vkms_get_crc(primary_crc, cursor_crc, out); > > frame_end = drm_crtc_accurate_vblank_count(crtc); > >diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c >index 177bbcb38306..d33ddcc40a29 100644 >--- a/drivers/gpu/drm/vkms/vkms_crtc.c >+++ b/drivers/gpu/drm/vkms/vkms_crtc.c >@@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > _vblank_handle(output); > >+ drm_writeback_signal_completion(&output->connector, 0); >+ > ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, > output->period_ns); > >diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h >index 1c93990693e3..1e8184ad872f 100644 >--- a/drivers/gpu/drm/vkms/vkms_drv.h >+++ b/drivers/gpu/drm/vkms/vkms_drv.h >@@ -5,6 +5,7 @@ > #include <drm/drm.h> > #include <drm/drm_gem.h> > #include <drm/drm_encoder.h> >+#include <drm/drm_writeback.h> > #include <linux/hrtimer.h> > > #define XRES_MIN 20 >@@ -61,7 +62,7 @@ struct vkms_crtc_state { > struct vkms_output { > struct drm_crtc crtc; > struct drm_encoder encoder; >- struct drm_connector connector; >+ struct drm_writeback_connector connector; > struct hrtimer vblank_hrtimer; > ktime_t period_ns; > struct drm_pending_vblank_event *event; >diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c >index 271a0eb9042c..46622d012107 100644 >--- a/drivers/gpu/drm/vkms/vkms_output.c >+++ b/drivers/gpu/drm/vkms/vkms_output.c >@@ -9,6 +9,13 @@ > #include "vkms_drv.h" > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> >+#include <drm/drm_writeback.h> >+ >+#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state) >+ >+struct vkms_writeback_connector_state { >+ struct drm_connector_state base; >+}; > > static void vkms_connector_destroy(struct drm_connector *connector) > { >@@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector) > drm_connector_cleanup(connector); > } > >+static void vkms_connector_reset(struct drm_connector *connector) >+{ >+ struct vkms_writeback_connector_state *wb_state = >+ kzalloc(sizeof(*wb_state), GFP_KERNEL); >+ >+ if (connector->state) >+ __drm_atomic_helper_connector_destroy_state(connector->state); >+ >+ kfree(connector->state); >+ __drm_atomic_helper_connector_reset(connector, &wb_state->base); >+} >+ >+static struct drm_connector_state * >+vkms_connector_duplicate_state(struct drm_connector *connector) >+{ >+ struct vkms_writeback_connector_state *wb_state; >+ >+ if (WARN_ON(!connector->state)) >+ return NULL; >+ >+ wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL); >+ if (!wb_state) >+ return NULL; >+ >+ __drm_atomic_helper_connector_duplicate_state(connector, >+ &wb_state->base); >+ >+ return &wb_state->base; >+} >+ >+static enum drm_connector_status >+vkms_connector_detect(struct drm_connector *connector, bool force) >+{ >+ return connector_status_connected; >+} >+ > static const struct drm_connector_funcs vkms_connector_funcs = { >+ .reset = vkms_connector_reset, >+ .detect = vkms_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > .destroy = vkms_connector_destroy, >- .reset = drm_atomic_helper_connector_reset, >- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >+ .atomic_duplicate_state = vkms_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; You're missing the call to drm_writeback_queue_job(), which is required. Your connector needs to have an atomic_commit callback in its drm_connector_helper_funcs, and in there you need to "queue" the job, something like: struct drm_writeback_connector *wb_conn = &out->connector; drm_writeback_queue_job(wb_conn, wb_conn->base.state->writeback_job); wb_conn->base.state->writeback_job = NULL; Because you need to set the job pointer to NULL there, before that you'll need to stash the destination somewhere else, to be used as the destination for the memcpy() in _vkms_get_crc(). The simplest thing to do is probably to just add a new void *writeback_vaddr to vkms_writeback_connector_state, and set it to the output framebuffer vaddr before you call _queue_job(). The core will make sure the framebuffer is properly refcounted until you call drm_writeback_signal_completion(), so as long as you only call that after you've finished copying the data, you're safe. In a "real" HW device, we write that pointer directly to a HW register, so we don't need to track it in our connector state. But you don't have a HW register to put it in, so you'll need to track it yourself :-) > >@@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { > .get_modes = vkms_conn_get_modes, > }; > >+static int vkms_encoder_atomic_check(struct drm_encoder *encoder, >+ struct drm_crtc_state *crtc_state, >+ struct drm_connector_state *conn_state) >+{ >+ struct drm_framebuffer *fb; >+ >+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) >+ return 0; >+ >+ fb = conn_state->writeback_job->fb; >+ if (fb->width != crtc_state->mode.hdisplay || >+ fb->height != crtc_state->mode.vdisplay) { >+ DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", >+ fb->width, fb->height); >+ return -EINVAL; >+ } >+ >+ return 0; >+} >+ >+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = { >+ .atomic_check = vkms_encoder_atomic_check, >+}; >+ > int vkms_output_init(struct vkms_device *vkmsdev) > { > struct vkms_output *output = &vkmsdev->output; > struct drm_device *dev = &vkmsdev->drm; >- struct drm_connector *connector = &output->connector; >- struct drm_encoder *encoder = &output->encoder; >+ struct drm_writeback_connector *connector = &output->connector; > struct drm_crtc *crtc = &output->crtc; > struct drm_plane *primary, *cursor = NULL; > int ret; >@@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev) > if (ret) > goto err_crtc; > >- ret = drm_connector_init(dev, connector, &vkms_connector_funcs, >- DRM_MODE_CONNECTOR_VIRTUAL); >+ ret = drm_writeback_connector_init(dev, connector, >+ &vkms_connector_funcs, >+ &vkms_encoder_helper_funcs, >+ vkms_formats, 1); > if (ret) { > DRM_ERROR("Failed to init connector\n"); > goto err_connector; > } > >- drm_connector_helper_add(connector, &vkms_conn_helper_funcs); >+ drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs); > >- ret = drm_connector_register(connector); >+ ret = drm_connector_register(&connector->base); > if (ret) { > DRM_ERROR("Failed to register connector\n"); > goto err_connector_register; > } > >- ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs, >- DRM_MODE_ENCODER_VIRTUAL, NULL); >- if (ret) { >- DRM_ERROR("Failed to init encoder\n"); >- goto err_encoder; >- } >- encoder->possible_crtcs = 1; >- >- ret = drm_connector_attach_encoder(connector, encoder); >- if (ret) { >- DRM_ERROR("Failed to attach connector to encoder\n"); >- goto err_attach; >- } >- > drm_mode_config_reset(dev); > > return 0; > >-err_attach: >- drm_encoder_cleanup(encoder); >- >-err_encoder: >- drm_connector_unregister(connector); >- > err_connector_register: >- drm_connector_cleanup(connector); >+ drm_connector_cleanup(&connector->base); > > err_connector: > drm_crtc_cleanup(crtc); >-- >2.19.1 > > >-- >Rodrigo Siqueira >https://siqueira.tech >https://twitter.com/siqueirajordao >Graduate Student >Department of Computer Science >University of São Paulo
On 10/24, Brian Starkey wrote: > Hi Rodrigo, Hi Brian, Thanks a lot for your feedback! I learned a lot with all of them :) I will work for make a real PATCH soon. Again, thanks a lot. Best Regards > On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote: > >Hi, > > > >I started to work for add the writeback feature into the VKMS, and this > >RFC represents my first draft. As a result, I have some questions which > >I list below. > > > >1. In this patch, I replaced the virtual connector by the writeback > >connector. Is it a good idea? Or should I keep both connectors? If I > >keep both connectors, should I add a module parameter to enable the > >writeback? > > > >2. When I added the writeback connector (in > >drm_writeback_connector_init()), I noticed that I had to indicate the > >struct drm_encoder_helper_funcs. However, I think that VKMS does not > >need anything sophisticated related to encoders at the writeback > >perspective. Am I right? Or did I missed something? > > > >3. I just discovered that the writeback connectors are not exposed as a > >connector for the userspace, as a result, IGT tests do not work and > >Wayland is unable to run as well. Simon Ser and Daniel Stone, explained > >to me that I have to update the compositor (in my case Weston) to set > >DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do > >not have experience with userspace, anyone can point me out another > >aspect that I have to take a look to make the writeback work in the user > >space? Also, Is it possible to update the IGT test or do I need to > >create a new set of tests? > > Alex added support for writeback connectors into drm_hwcomposer - you > can see that here[1] > > There's also some igt tests like Daniel mentioned - I think Liviu > already pointed you at those. > > Some more review below, > > Cheers! > -Brian > > [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 > > > > >Thanks! > > > >Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > >--- > > drivers/gpu/drm/vkms/vkms_crc.c | 9 ++- > > drivers/gpu/drm/vkms/vkms_crtc.c | 2 + > > drivers/gpu/drm/vkms/vkms_drv.h | 3 +- > > drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++-------- > > 4 files changed, 89 insertions(+), 32 deletions(-) > > > >diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > >index 9d9e8146db90..5d9ac45e00f2 100644 > >--- a/drivers/gpu/drm/vkms/vkms_crc.c > >+++ b/drivers/gpu/drm/vkms/vkms_crc.c > >@@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc, > > } > > > > static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > >- struct vkms_crc_data *cursor_crc) > >+ struct vkms_crc_data *cursor_crc, > >+ struct vkms_output *out) > > { > > struct drm_framebuffer *fb = &primary_crc->fb; > > struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); > >@@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > > } > > > > memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size); > >+ // TODO: Remove this copy from crc, and make this sort of operation > >+ // generic since we can use in different places > >+ memcpy(out->connector.base.state->writeback_job->fb, > >+ vkms_obj->vaddr, vkms_obj->gem.size); > > This looks like it would crash terribly, you're copying the pixel data > (from vkms_obj->vaddr) into a "struct drm_framebuffer *" which is a > pointer to the struct, rather than somewhere to put the actual > pixels. > > The same way you get the vaddr for the input framebuffer, you need to > get the vaddr for the output framebuffer too - via > drm_gem_fb_get_obj() and then converting that to a vkms_gem_object. > Then use that vaddr as the destination of the memcpy. However, see my > comments at the bottom about drm_writeback_queue_job(). You might be > better getting and storing the vaddr during atomic_commit(), rather > than in this function. > > In order for this to be a straight memcpy, you need to make sure that > the input and output buffers have the same size, pixel format, same > stride etc. as well so you'll need to check that in atomic_check(). > Once it's working you can relax those restrictions by copying a line > at a time or doing pixel format conversion - but I don't think it's > necessary to start with. > > There might be an opportunity to skip a copy here too - if you have a > framebuffer attached to the writeback connector, you can skip > allocating "vaddr_out" and just compose the cursor directly into the > output framebuffer, and calculate the CRC on that. That's definitely > an optimisation rather than a requirement though. > > > mutex_unlock(&vkms_obj->pages_lock); > > > > if (cursor_crc) > >@@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work) > > } > > > > if (primary_crc) > >- crc32 = _vkms_get_crc(primary_crc, cursor_crc); > >+ crc32 = _vkms_get_crc(primary_crc, cursor_crc, out); > > > > frame_end = drm_crtc_accurate_vblank_count(crtc); > > > >diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > >index 177bbcb38306..d33ddcc40a29 100644 > >--- a/drivers/gpu/drm/vkms/vkms_crtc.c > >+++ b/drivers/gpu/drm/vkms/vkms_crtc.c > >@@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > > > _vblank_handle(output); > > > >+ drm_writeback_signal_completion(&output->connector, 0); > >+ > > ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, > > output->period_ns); > > > >diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > >index 1c93990693e3..1e8184ad872f 100644 > >--- a/drivers/gpu/drm/vkms/vkms_drv.h > >+++ b/drivers/gpu/drm/vkms/vkms_drv.h > >@@ -5,6 +5,7 @@ > > #include <drm/drm.h> > > #include <drm/drm_gem.h> > > #include <drm/drm_encoder.h> > >+#include <drm/drm_writeback.h> > > #include <linux/hrtimer.h> > > > > #define XRES_MIN 20 > >@@ -61,7 +62,7 @@ struct vkms_crtc_state { > > struct vkms_output { > > struct drm_crtc crtc; > > struct drm_encoder encoder; > >- struct drm_connector connector; > >+ struct drm_writeback_connector connector; > > struct hrtimer vblank_hrtimer; > > ktime_t period_ns; > > struct drm_pending_vblank_event *event; > >diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > >index 271a0eb9042c..46622d012107 100644 > >--- a/drivers/gpu/drm/vkms/vkms_output.c > >+++ b/drivers/gpu/drm/vkms/vkms_output.c > >@@ -9,6 +9,13 @@ > > #include "vkms_drv.h" > > #include <drm/drm_crtc_helper.h> > > #include <drm/drm_atomic_helper.h> > >+#include <drm/drm_writeback.h> > >+ > >+#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state) > >+ > >+struct vkms_writeback_connector_state { > >+ struct drm_connector_state base; > >+}; > > > > static void vkms_connector_destroy(struct drm_connector *connector) > > { > >@@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector) > > drm_connector_cleanup(connector); > > } > > > >+static void vkms_connector_reset(struct drm_connector *connector) > >+{ > >+ struct vkms_writeback_connector_state *wb_state = > >+ kzalloc(sizeof(*wb_state), GFP_KERNEL); > >+ > >+ if (connector->state) > >+ __drm_atomic_helper_connector_destroy_state(connector->state); > >+ > >+ kfree(connector->state); > >+ __drm_atomic_helper_connector_reset(connector, &wb_state->base); > >+} > >+ > >+static struct drm_connector_state * > >+vkms_connector_duplicate_state(struct drm_connector *connector) > >+{ > >+ struct vkms_writeback_connector_state *wb_state; > >+ > >+ if (WARN_ON(!connector->state)) > >+ return NULL; > >+ > >+ wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL); > >+ if (!wb_state) > >+ return NULL; > >+ > >+ __drm_atomic_helper_connector_duplicate_state(connector, > >+ &wb_state->base); > >+ > >+ return &wb_state->base; > >+} > >+ > >+static enum drm_connector_status > >+vkms_connector_detect(struct drm_connector *connector, bool force) > >+{ > >+ return connector_status_connected; > >+} > >+ > > static const struct drm_connector_funcs vkms_connector_funcs = { > >+ .reset = vkms_connector_reset, > >+ .detect = vkms_connector_detect, > > .fill_modes = drm_helper_probe_single_connector_modes, > > .destroy = vkms_connector_destroy, > >- .reset = drm_atomic_helper_connector_reset, > >- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > >+ .atomic_duplicate_state = vkms_connector_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > }; > > You're missing the call to drm_writeback_queue_job(), which is > required. Your connector needs to have an atomic_commit callback in > its drm_connector_helper_funcs, and in there you need to "queue" the > job, something like: > > struct drm_writeback_connector *wb_conn = &out->connector; > drm_writeback_queue_job(wb_conn, wb_conn->base.state->writeback_job); > wb_conn->base.state->writeback_job = NULL; > > Because you need to set the job pointer to NULL there, before that > you'll need to stash the destination somewhere else, to be used as the > destination for the memcpy() in _vkms_get_crc(). The simplest thing to > do is probably to just add a new void *writeback_vaddr to > vkms_writeback_connector_state, and set it to the output framebuffer > vaddr before you call _queue_job(). The core will make sure the > framebuffer is properly refcounted until you call > drm_writeback_signal_completion(), so as long as you only call that > after you've finished copying the data, you're safe. > > In a "real" HW device, we write that pointer directly to a HW > register, so we don't need to track it in our connector state. But you > don't have a HW register to put it in, so you'll need to track it > yourself :-) > > > > >@@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { > > .get_modes = vkms_conn_get_modes, > > }; > > > >+static int vkms_encoder_atomic_check(struct drm_encoder *encoder, > >+ struct drm_crtc_state *crtc_state, > >+ struct drm_connector_state *conn_state) > >+{ > >+ struct drm_framebuffer *fb; > >+ > >+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > >+ return 0; > >+ > >+ fb = conn_state->writeback_job->fb; > >+ if (fb->width != crtc_state->mode.hdisplay || > >+ fb->height != crtc_state->mode.vdisplay) { > >+ DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", > >+ fb->width, fb->height); > >+ return -EINVAL; > >+ } > >+ > >+ return 0; > >+} > >+ > >+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = { > >+ .atomic_check = vkms_encoder_atomic_check, > >+}; > >+ > > int vkms_output_init(struct vkms_device *vkmsdev) > > { > > struct vkms_output *output = &vkmsdev->output; > > struct drm_device *dev = &vkmsdev->drm; > >- struct drm_connector *connector = &output->connector; > >- struct drm_encoder *encoder = &output->encoder; > >+ struct drm_writeback_connector *connector = &output->connector; > > struct drm_crtc *crtc = &output->crtc; > > struct drm_plane *primary, *cursor = NULL; > > int ret; > >@@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev) > > if (ret) > > goto err_crtc; > > > >- ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > >- DRM_MODE_CONNECTOR_VIRTUAL); > >+ ret = drm_writeback_connector_init(dev, connector, > >+ &vkms_connector_funcs, > >+ &vkms_encoder_helper_funcs, > >+ vkms_formats, 1); > > if (ret) { > > DRM_ERROR("Failed to init connector\n"); > > goto err_connector; > > } > > > >- drm_connector_helper_add(connector, &vkms_conn_helper_funcs); > >+ drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs); > > > >- ret = drm_connector_register(connector); > >+ ret = drm_connector_register(&connector->base); > > if (ret) { > > DRM_ERROR("Failed to register connector\n"); > > goto err_connector_register; > > } > > > >- ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs, > >- DRM_MODE_ENCODER_VIRTUAL, NULL); > >- if (ret) { > >- DRM_ERROR("Failed to init encoder\n"); > >- goto err_encoder; > >- } > >- encoder->possible_crtcs = 1; > >- > >- ret = drm_connector_attach_encoder(connector, encoder); > >- if (ret) { > >- DRM_ERROR("Failed to attach connector to encoder\n"); > >- goto err_attach; > >- } > >- > > drm_mode_config_reset(dev); > > > > return 0; > > > >-err_attach: > >- drm_encoder_cleanup(encoder); > >- > >-err_encoder: > >- drm_connector_unregister(connector); > >- > > err_connector_register: > >- drm_connector_cleanup(connector); > >+ drm_connector_cleanup(&connector->base); > > > > err_connector: > > drm_crtc_cleanup(crtc); > >-- > >2.19.1 > > > > > >-- > >Rodrigo Siqueira > >https://siqueira.tech > >https://twitter.com/siqueirajordao > >Graduate Student > >Department of Computer Science > >University of São Paulo
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c index 9d9e8146db90..5d9ac45e00f2 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc, } static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, - struct vkms_crc_data *cursor_crc) + struct vkms_crc_data *cursor_crc, + struct vkms_output *out) { struct drm_framebuffer *fb = &primary_crc->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); @@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, } memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size); + // TODO: Remove this copy from crc, and make this sort of operation + // generic since we can use in different places + memcpy(out->connector.base.state->writeback_job->fb, + vkms_obj->vaddr, vkms_obj->gem.size); mutex_unlock(&vkms_obj->pages_lock); if (cursor_crc) @@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work) } if (primary_crc) - crc32 = _vkms_get_crc(primary_crc, cursor_crc); + crc32 = _vkms_get_crc(primary_crc, cursor_crc, out); frame_end = drm_crtc_accurate_vblank_count(crtc); diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 177bbcb38306..d33ddcc40a29 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) _vblank_handle(output); + drm_writeback_signal_completion(&output->connector, 0); + ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, output->period_ns); diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 1c93990693e3..1e8184ad872f 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -5,6 +5,7 @@ #include <drm/drm.h> #include <drm/drm_gem.h> #include <drm/drm_encoder.h> +#include <drm/drm_writeback.h> #include <linux/hrtimer.h> #define XRES_MIN 20 @@ -61,7 +62,7 @@ struct vkms_crtc_state { struct vkms_output { struct drm_crtc crtc; struct drm_encoder encoder; - struct drm_connector connector; + struct drm_writeback_connector connector; struct hrtimer vblank_hrtimer; ktime_t period_ns; struct drm_pending_vblank_event *event; diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index 271a0eb9042c..46622d012107 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -9,6 +9,13 @@ #include "vkms_drv.h" #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_writeback.h> + +#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state) + +struct vkms_writeback_connector_state { + struct drm_connector_state base; +}; static void vkms_connector_destroy(struct drm_connector *connector) { @@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector) drm_connector_cleanup(connector); } +static void vkms_connector_reset(struct drm_connector *connector) +{ + struct vkms_writeback_connector_state *wb_state = + kzalloc(sizeof(*wb_state), GFP_KERNEL); + + if (connector->state) + __drm_atomic_helper_connector_destroy_state(connector->state); + + kfree(connector->state); + __drm_atomic_helper_connector_reset(connector, &wb_state->base); +} + +static struct drm_connector_state * +vkms_connector_duplicate_state(struct drm_connector *connector) +{ + struct vkms_writeback_connector_state *wb_state; + + if (WARN_ON(!connector->state)) + return NULL; + + wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL); + if (!wb_state) + return NULL; + + __drm_atomic_helper_connector_duplicate_state(connector, + &wb_state->base); + + return &wb_state->base; +} + +static enum drm_connector_status +vkms_connector_detect(struct drm_connector *connector, bool force) +{ + return connector_status_connected; +} + static const struct drm_connector_funcs vkms_connector_funcs = { + .reset = vkms_connector_reset, + .detect = vkms_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = vkms_connector_destroy, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_duplicate_state = vkms_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; @@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { .get_modes = vkms_conn_get_modes, }; +static int vkms_encoder_atomic_check(struct drm_encoder *encoder, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct drm_framebuffer *fb; + + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) + return 0; + + fb = conn_state->writeback_job->fb; + if (fb->width != crtc_state->mode.hdisplay || + fb->height != crtc_state->mode.vdisplay) { + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", + fb->width, fb->height); + return -EINVAL; + } + + return 0; +} + +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = { + .atomic_check = vkms_encoder_atomic_check, +}; + int vkms_output_init(struct vkms_device *vkmsdev) { struct vkms_output *output = &vkmsdev->output; struct drm_device *dev = &vkmsdev->drm; - struct drm_connector *connector = &output->connector; - struct drm_encoder *encoder = &output->encoder; + struct drm_writeback_connector *connector = &output->connector; struct drm_crtc *crtc = &output->crtc; struct drm_plane *primary, *cursor = NULL; int ret; @@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev) if (ret) goto err_crtc; - ret = drm_connector_init(dev, connector, &vkms_connector_funcs, - DRM_MODE_CONNECTOR_VIRTUAL); + ret = drm_writeback_connector_init(dev, connector, + &vkms_connector_funcs, + &vkms_encoder_helper_funcs, + vkms_formats, 1); if (ret) { DRM_ERROR("Failed to init connector\n"); goto err_connector; } - drm_connector_helper_add(connector, &vkms_conn_helper_funcs); + drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs); - ret = drm_connector_register(connector); + ret = drm_connector_register(&connector->base); if (ret) { DRM_ERROR("Failed to register connector\n"); goto err_connector_register; } - ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs, - DRM_MODE_ENCODER_VIRTUAL, NULL); - if (ret) { - DRM_ERROR("Failed to init encoder\n"); - goto err_encoder; - } - encoder->possible_crtcs = 1; - - ret = drm_connector_attach_encoder(connector, encoder); - if (ret) { - DRM_ERROR("Failed to attach connector to encoder\n"); - goto err_attach; - } - drm_mode_config_reset(dev); return 0; -err_attach: - drm_encoder_cleanup(encoder); - -err_encoder: - drm_connector_unregister(connector); - err_connector_register: - drm_connector_cleanup(connector); + drm_connector_cleanup(&connector->base); err_connector: drm_crtc_cleanup(crtc);
Hi, I started to work for add the writeback feature into the VKMS, and this RFC represents my first draft. As a result, I have some questions which I list below. 1. In this patch, I replaced the virtual connector by the writeback connector. Is it a good idea? Or should I keep both connectors? If I keep both connectors, should I add a module parameter to enable the writeback? 2. When I added the writeback connector (in drm_writeback_connector_init()), I noticed that I had to indicate the struct drm_encoder_helper_funcs. However, I think that VKMS does not need anything sophisticated related to encoders at the writeback perspective. Am I right? Or did I missed something? 3. I just discovered that the writeback connectors are not exposed as a connector for the userspace, as a result, IGT tests do not work and Wayland is unable to run as well. Simon Ser and Daniel Stone, explained to me that I have to update the compositor (in my case Weston) to set DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do not have experience with userspace, anyone can point me out another aspect that I have to take a look to make the writeback work in the user space? Also, Is it possible to update the IGT test or do I need to create a new set of tests? Thanks! Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> --- drivers/gpu/drm/vkms/vkms_crc.c | 9 ++- drivers/gpu/drm/vkms/vkms_crtc.c | 2 + drivers/gpu/drm/vkms/vkms_drv.h | 3 +- drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++-------- 4 files changed, 89 insertions(+), 32 deletions(-)