On Tue, 12 May 2020 at 12:34, Emil Velikov emil.l.velikov@gmail.com wrote:

Hi Rodrigo,

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

...

From: Rodrigo Siqueira rodrigosiqueiramelo@gmail.com

The compute_crc() function is responsible for calculating the framebuffer CRC value; due to the XRGB format, this function has to ignore the alpha channel during the CRC computation. Therefore, compute_crc() set zero to the alpha channel directly in the input framebuffer, which is not a problem since this function receives a copy of the original buffer. However, if we want to use this function in a context without a buffer copy, it will change the initial value. This patch makes compute_crc() calculate the CRC value without modifying the input framebuffer.

Signed-off-by: Rodrigo Siqueira rodrigosiqueiramelo@gmail.com

drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++----------- 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 258e659ecfba..686d25e7b01d 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -9,33 +9,40 @@

#include "vkms_drv.h"

+static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,

•                            const struct vkms_composer *composer)
  

+{

•   int src_offset = composer->offset + (y * composer->pitch)
  

•                                     + (x * composer->cpp);
  

•   return *(u32 *)&buffer[src_offset];
  

+}

/**

• compute_crc - Compute CRC value on output frame

• • @vaddr_out: address to final framebuffer

• • @vaddr: address to final framebuffer
  • @composer: framebuffer's metadata
  • returns CRC value computed using crc32 on the visible portion of
  • the final framebuffer at vaddr_out
  */

-static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer) +static uint32_t compute_crc(const u8 *vaddr,

•                       const struct vkms_composer *composer)
  

{

•   int i, j, src_offset;
  

•   int x, y;
    int x_src = composer->src.x1 >> 16;
    int y_src = composer->src.y1 >> 16;
    int h_src = drm_rect_height(&composer->src) >> 16;
    int w_src = drm_rect_width(&composer->src) >> 16;
  

•   u32 crc = 0;
  

•   u32 crc = 0, pixel = 0;
  
  

•   for (i = y_src; i < y_src + h_src; ++i) {
  

•           for (j = x_src; j < x_src + w_src; ++j) {
  

•                   src_offset = composer->offset
  

•                                + (i * composer->pitch)
  

•                                + (j * composer->cpp);
  

•   for (y = y_src; y < y_src + h_src; ++y) {
  

•           for (x = x_src; x < x_src + w_src; ++x) {
                    /* XRGB format ignores Alpha channel */
  

•                   memset(vaddr_out + src_offset + 24, 0,  8);
  

•                   crc = crc32_le(crc, vaddr_out + src_offset,
  

•                                  sizeof(u32));
  

•                   pixel = get_pixel_from_buffer(x, y, vaddr, composer);
  

•                   bitmap_clear((void *)&pixel, 0, 8);
  

•                   crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
            }
    }
  
  

IMHO using something like the following makes the code far simpler and clearer.

offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp);

for (i = 0; i < h_src; i++, offset += composer->pitch) { for (j = 0; j < w_src; j++, offset += composer->cpp) { pixel = get_pixel_from_buffer(vaddr, offset); crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed } }

With the bitmap_clear() and related comment moved into get_pixel_from_buffer().

If you fold the bitmap_clear() in get_pixel_from_buffer(), and drop the cast (unless I'm missing something and it's really needed) for crc32_le() this patch is:

Reviewed-by: Emil Velikov emil.l.velikov@gmail.com

I would suggest (but it's not a requirement) that you simplify the loop/offset calculation as separate patch in v5.

-Emil

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