Re: [PATCH 7/7] drm/udl: Move udl_handle_damage() into udl_modeset.c

On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann tzimmermann@suse.de wrote:

The only caller of udl_handle_damage() in the plane-update function in udl_modeset.c. Move udl_handle_damage() there, make it static, and remove several left-over macros.

Personally I would have left the mechanic code motion from the dead code removal. Not a big deal though: Reviewed-by: Emil Velikov emil.l.velikov@gmail.com

There's few comments for follow-up work below.

+static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,

•                        int width, int height)
  

+{

•   struct drm_device *dev = fb->dev;
  

•   struct udl_device *udl = to_udl(dev);
  

•   int i, ret;
  

•   char *cmd;
  

•   cycles_t start_cycles, end_cycles;
  

•   int bytes_sent = 0;
  

•   int bytes_identical = 0;
  

•   struct urb *urb;
  

•   int aligned_x;
  

•   int log_bpp;
  

•   void *vaddr;
  

•   if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
  

•           return -EINVAL;
  

•   log_bpp = __ffs(fb->format->cpp[0]);
  

•   vaddr = drm_gem_shmem_vmap(fb->obj[0]);
  

•   if (IS_ERR(vaddr)) {
  

•           DRM_ERROR("failed to vmap fb\n");
  

•           return 0;
  

•   }
  

Might as well move this hunk ...

•   aligned_x = ALIGN_DOWN(x, sizeof(unsigned long));
  

•   width = ALIGN(width + (x-aligned_x), sizeof(unsigned long));
  

•   x = aligned_x;
  

•   if ((width <= 0) ||
  

•       (x + width > fb->width) ||
  

•       (y + height > fb->height)) {
  

•           ret = -EINVAL;
  

•           goto err_drm_gem_shmem_vunmap;
  

•   }
  

•   start_cycles = get_cycles();
  

•   urb = udl_get_urb(dev);
  

•   if (!urb)
  

•           goto out;
  

•   cmd = urb->transfer_buffer;
  

... here

•   for (i = y; i < y + height ; i++) {
  

•           const int line_offset = fb->pitches[0] * i;
  

•           const int byte_offset = line_offset + (x << log_bpp);
  

•           const int dev_byte_offset = (fb->width * i + x) << log_bpp;
  

•           if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
  

•                                &cmd, byte_offset, dev_byte_offset,
  

•                                width << log_bpp,
  

•                                &bytes_identical, &bytes_sent))
  

udl_render_hline() - drop the unused args bytes_identical and bytes_sent?

•                   goto error;
  

•   }
  

•   if (cmd > (char *) urb->transfer_buffer) {
  

•           /* Send partial buffer remaining before exiting */
  

•           int len;
  

•           if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
  

•                   *cmd++ = 0xAF;
  

•           len = cmd - (char *) urb->transfer_buffer;
  

•           ret = udl_submit_urb(dev, urb, len);
  

•           bytes_sent += len;
  

Nit:

int len = cmd - (char *) urb->transfer_buffer; if (len > 0) { /* Send partial buffer remaining before exiting */ if (len < urb->transfer_buffer_length) ...

•   } else
  

•           udl_urb_completion(urb);
  

+error:

•   atomic_add(bytes_sent, &udl->bytes_sent);
  

•   atomic_add(bytes_identical, &udl->bytes_identical);
  

•   atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
  

•   end_cycles = get_cycles();
  

•   atomic_add(((unsigned int) ((end_cycles - start_cycles)
  

•               >> 10)), /* Kcycles */
  

•              &udl->cpu_kcycles_used);
  

These atomics (+ lost_pixels) seem to be set-but-unused since day one. We might as well kill them, alongside the associated get_cycles().

HTH Emil