On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
