On Fri, Jul 12, 2019 at 12:37:22AM -0300, Rodrigo Siqueira wrote:
> On 07/11, Daniel Vetter wrote:
> > On Tue, Jun 25, 2019 at 10:39:03PM -0300, Rodrigo Siqueira wrote:
> > > This patch implements the necessary functions to add writeback support
> > > for vkms. This feature is useful for testing compositors if you don't
> > > have hardware with writeback support.
> > >
> > > Change in V3 (Daniel):
> > > - If writeback is enabled, compose everything into the writeback buffer
> > > instead of CRC private buffer.
> > > - Guarantees that the CRC will match exactly what we have in the
> > > writeback buffer.
> > >
> > > Change in V2:
> > > - Rework signal completion (Brian)
> > > - Integrates writeback with active_planes (Daniel)
> > > - Compose cursor (Daniel)
> > >
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/vkms/Makefile | 9 +-
> > > drivers/gpu/drm/vkms/vkms_composer.c | 16 ++-
> > > drivers/gpu/drm/vkms/vkms_drv.c | 4 +
> > > drivers/gpu/drm/vkms/vkms_drv.h | 8 ++
> > > drivers/gpu/drm/vkms/vkms_output.c | 10 ++
> > > drivers/gpu/drm/vkms/vkms_writeback.c | 141 ++++++++++++++++++++++++++
> > > 6 files changed, 185 insertions(+), 3 deletions(-)
> > > create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > >
> > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > index 0b767d7efa24..333d3cead0e3 100644
> > > --- a/drivers/gpu/drm/vkms/Makefile
> > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > @@ -1,4 +1,11 @@
> > > # SPDX-License-Identifier: GPL-2.0-only
> > > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
> > > +vkms-y := \
> > > + vkms_drv.o \
> > > + vkms_plane.o \
> > > + vkms_output.o \
> > > + vkms_crtc.o \
> > > + vkms_gem.o \
> > > + vkms_composer.o \
> > > + vkms_writeback.o
> > >
> > > obj-$(CONFIG_DRM_VKMS) += vkms.o
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index 8126aa0f968f..2317803e7320 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -157,16 +157,17 @@ void vkms_composer_worker(struct work_struct *work)
> > > struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > > struct vkms_composer *primary_composer = NULL;
> > > struct vkms_composer *cursor_composer = NULL;
> > > + bool crc_pending, wb_pending;
> >
> > I'm not seeing how you schedule this work for writeback if there's no crc
> > enabled. Does that work?
>
> If we enable writeback, we set true for the flag composer_enabled (see
> vkms_output.c in this patch) which also make vkms_vblank_simulate able
> to handle crc. Since writeback is oneshot, we only enable it on
> vkms_wb_atomic_commit (crtc_state->wb_pending = true); in its turn
> vkms_composer_worker() use writeback buffer instead of creating a new
> one for computing crc. Finally, at the end of vkms_composer_worker() we
> disable the writeback.
Yeah I got all that. But who's doing the queue_work? That only happens
if crc_enabled is set, and I'm not seeing how that's changed. Might be in
one of the earlier patches ...
-Daniel
>
> > > void *vaddr_out = NULL;
> > > u32 crc32 = 0;
> > > u64 frame_start, frame_end;
> > > - bool crc_pending;
> > > int ret;
> > >
> > > spin_lock_irq(&out->composer_lock);
> > > frame_start = crtc_state->frame_start;
> > > frame_end = crtc_state->frame_end;
> > > crc_pending = crtc_state->crc_pending;
> > > + wb_pending = crtc_state->wb_pending;
> > > crtc_state->frame_start = 0;
> > > crtc_state->frame_end = 0;
> > > crtc_state->crc_pending = false;
> > > @@ -188,9 +189,12 @@ void vkms_composer_worker(struct work_struct *work)
> > > if (!primary_composer)
> > > return;
> > >
> > > + if (wb_pending)
> > > + vaddr_out = crtc_state->active_writeback;
> > > +
> > > ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> > > if (ret) {
> > > - if (ret == -EINVAL)
> > > + if (ret == -EINVAL && !wb_pending)
> > > kfree(vaddr_out);
> > > return;
> > > }
> > > @@ -203,6 +207,14 @@ void vkms_composer_worker(struct work_struct *work)
> > > while (frame_start <= frame_end)
> > > drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > >
> > > + if (wb_pending) {
> > > + drm_writeback_signal_completion(&out->wb_connector, 0);
> > > + spin_lock_irq(&out->composer_lock);
> > > + crtc_state->wb_pending = false;
> > > + spin_unlock_irq(&out->composer_lock);
> > > + return;
> > > + }
> > > +
> > > kfree(vaddr_out);
> > > }
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index ac790b6527e4..152d7de24a76 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -30,6 +30,10 @@ bool enable_cursor;
> > > module_param_named(enable_cursor, enable_cursor, bool, 0444);
> > > MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> > >
> > > +bool enable_writeback;
> > > +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> > > +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> > > +
> > > static const struct file_operations vkms_driver_fops = {
> > > .owner = THIS_MODULE,
> > > .open = drm_open,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index fc6cda164336..9ff2cd4ebf81 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -7,6 +7,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
> > > @@ -19,6 +20,7 @@
> > > #define YRES_MAX 8192
> > >
> > > extern bool enable_cursor;
> > > +extern bool enable_writeback;
> > >
> > > struct vkms_composer {
> > > struct drm_framebuffer fb;
> > > @@ -52,9 +54,11 @@ struct vkms_crtc_state {
> > > int num_active_planes;
> > > /* stack of active planes for crc computation, should be in z order */
> > > struct vkms_plane_state **active_planes;
> > > + void *active_writeback;
> > >
> > > /* below three are protected by vkms_output.composer_lock */
> > > bool crc_pending;
> > > + bool wb_pending;
> > > u64 frame_start;
> > > u64 frame_end;
> > > };
> > > @@ -63,6 +67,7 @@ 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;
> > > struct drm_pending_vblank_event *event;
> > > @@ -147,4 +152,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > > /* Composer Support */
> > > void vkms_composer_worker(struct work_struct *work);
> > >
> > > +/* Writeback */
> > > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > > +
> > > #endif /* _VKMS_DRV_H_ */
> > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > index fb1941a6522c..aea1d4410864 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > @@ -84,6 +84,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > goto err_attach;
> > > }
> > >
> > > + if (enable_writeback) {
> > > + ret = enable_writeback_connector(vkmsdev);
> > > + if (!ret) {
> > > + output->composer_enabled = true;
> > > + DRM_INFO("Writeback connector enabled");
> > > + } else {
> > > + DRM_ERROR("Failed to init writeback connector\n");
> > > + }
> > > + }
> > > +
> > > drm_mode_config_reset(dev);
> > >
> > > return 0;
> > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > new file mode 100644
> > > index 000000000000..34dad37a0236
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > @@ -0,0 +1,141 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +#include "vkms_drv.h"
> > > +#include <drm/drm_writeback.h>
> > > +#include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_gem_framebuffer_helper.h>
> > > +
> > > +static const u32 vkms_wb_formats[] = {
> > > + DRM_FORMAT_XRGB8888,
> > > +};
> > > +
> > > +static const struct drm_connector_funcs vkms_wb_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,
> > > +};
> > > +
> > > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > > + struct drm_crtc_state *crtc_state,
> > > + struct drm_connector_state *conn_state)
> > > +{
> > > + struct drm_framebuffer *fb;
> > > + const struct drm_display_mode *mode = &crtc_state->mode;
> > > +
> > > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > > + return 0;
> > > +
> > > + fb = conn_state->writeback_job->fb;
> > > + if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> > > + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> > > + fb->width, fb->height);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (fb->format->format != DRM_FORMAT_XRGB8888) {
> > > + struct drm_format_name_buf format_name;
> > > +
> > > + DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > > + drm_get_format_name(fb->format->format,
> > > + &format_name));
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > > + .atomic_check = vkms_wb_encoder_atomic_check,
> > > +};
> > > +
> > > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > > +{
> > > + struct drm_device *dev = connector->dev;
> > > +
> > > + return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > > + dev->mode_config.max_height);
> > > +}
> >
> > Do we need a mode for writeb connector? Is that used by userspace to
> > figure out the max size for writeback?
>
> Yes, we need it. You're right about the userspace utilizing it to figure
> out the max size writeback, kms_writeback strongly rely on this.
>
> > > +
> > > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > > + struct drm_writeback_job *job)
> > > +{
> > > + struct vkms_gem_object *vkms_obj;
> > > + struct drm_gem_object *gem_obj;
> > > + int ret;
> > > +
> > > + if (!job->fb)
> > > + return 0;
> >
> > Can this happen?
>
> Yes, here is the path until this function is invoked:
>
> drm_atomic_helper_prepare_planes()
>
> Inside the above function we have:
>
> for_each_new_connector_in_state(state, connector, new_conn_state, i)
>
> which in turn invoke:
>
> drm_writeback_prepare_job(new_conn_state->writeback_job);
>
> FWIU, new_conn_state->writeback_job could be NULL. Anyway, I also tested
> it, and I notice this condition can be reached.
>
> > > +
> > > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > + ret = vkms_gem_vmap(gem_obj);
> > > + if (ret) {
> > > + DRM_ERROR("vmap failed: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > + job->priv = vkms_obj->vaddr;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > > + struct drm_writeback_job *job)
> > > +{
> > > + struct drm_gem_object *gem_obj;
> > > +
> > > + if (!job->fb)
> > > + return;
> >
> > Same here?
> >
> > > +
> > > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > + vkms_gem_vunmap(gem_obj);
> > > +}
> > > +
> > > +static void vkms_wb_atomic_commit(struct drm_connector *conn,
> > > + struct drm_connector_state *state)
> > > +{
> > > + struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> > > + struct vkms_output *output = &vkmsdev->output;
> > > + struct drm_writeback_connector *wb_conn = &output->wb_connector;
> > > + struct drm_connector_state *conn_state = wb_conn->base.state;
> > > + struct vkms_crtc_state *crtc_state = output->composer_state;
> > > +
> > > + if (!conn_state)
> > > + return;
> > > +
> > > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > > + DRM_DEBUG_DRIVER("Disable writeback\n");
> > > + return;
> > > + }
> >
> > All three checks above sound like helpers should take care of this?
>
> Maybe I missed something, but I did not find such helper. I also noticed
> that vc4, rcar-du, and mali does the same check.
>
> Thanks
>
> > > +
> > > + spin_lock_irq(&output->composer_lock);
> > > + crtc_state->active_writeback = conn_state->writeback_job->priv;
> > > + crtc_state->wb_pending = true;
> > > + spin_unlock_irq(&output->composer_lock);
> > > + drm_writeback_queue_job(wb_conn, state);
> > > +}
> > > +
> > > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > > + .get_modes = vkms_wb_connector_get_modes,
> > > + .prepare_writeback_job = vkms_wb_prepare_job,
> > > + .cleanup_writeback_job = vkms_wb_cleanup_job,
> > > + .atomic_commit = vkms_wb_atomic_commit,
> > > +};
> > > +
> > > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > > +{
> > > + struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > > +
> > > + vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > > + drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > > +
> > > + return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > > + &vkms_wb_connector_funcs,
> > > + &vkms_wb_encoder_helper_funcs,
> > > + vkms_wb_formats,
> > > + ARRAY_SIZE(vkms_wb_formats));
> > > +}
> >
> > Overall looks like good approach.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Rodrigo Siqueira
> https://siqueira.tech
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
