Hi
Am 02.12.19 um 10:27 schrieb Emil Velikov:
> 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().
I wanted to do this later. But since you brought it up and there's also
the issue where I forgot to convert the import_attach code, I'll send
out a series to clean up the damage handling and then get back to the
simple-pipe conversion.
Best regards
Thomas
>
> HTH
> Emil
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
