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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4502 bytes --]

Hi

Am 02.12.19 um 10:27 schrieb Emil Velikov:
> 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().

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel