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

[Emil Velikov]

On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann at 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 at 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