Re: [PATCH V4 3/3] drm/vkms: Add support for writeback

[Emil Velikov <emil.l.velikov@gmail.com>]

On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:

>  # 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
>
Nit: sort alphabetically


> @@ -191,9 +192,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);
Forgot to mention earlier - with the allocation happening in the
caller, compose_planes() can take void *vaddr_out.

>         if (ret) {
> -               if (ret == -EINVAL)
> +               if (ret == -EINVAL && !wb_pending)
>                         kfree(vaddr_out);
>                 return;
>         }
> @@ -206,6 +210,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;
> +       }
> +
Just like the free() move this above the drm_crtc_add_crc_entry()

if (wb_pending) {
  signal()
  ...
} else {
  free()
}

> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..34dd74dc8eb0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -39,6 +39,10 @@ bool enable_cursor = true;
>  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");
> +
Why is this a module parameter? Let's always enable it and leave it to
userspace whether to use the wb or not.

>  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 f4036bb0b9a8..35f0b71413c9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
>  #include <drm/drm.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_writeback.h>
>
>  #define XRES_MIN    20
>  #define YRES_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 */
Nit: s/below three/the following four/

>         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;
> @@ -144,4 +149,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);

Don't forget appropriate name spacing - prefix the function with vkms.

> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..19ffc67affec 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>                 goto err_attach;
>         }
>
> +       if (enable_writeback) {
> +               ret = enable_writeback_connector(vkmsdev);

With wb connector always enabled, you can kill off the
vkms_output::composer_enabled all together. Plus the info/error
warnings (below) can go as well.

> +               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..868f0d15ca9f
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_fourcc.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>
> +
Nit: sort includes alphabetically.

> +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)
Drop the fb check.

> +               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) {
Use the vkms_wb_formats[], regardless if it's one entry or not.

> +               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;

Thinking out loud:
This function should be a helper that drivers can reuse and build
upon. The format might be tricky.
It's already in drm_writeback_connector::pixel_formats_blob_ptr, while
the function takes drm_writeback::encoder as argument.

But that for another patch series.


> +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) {
Drop the fb check.

-Emil