| Message ID | 20200511115524.22602-3-Rodrigo.Siqueira@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Introduces writeback support | expand |
Hi Rodrigo, On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote: > > From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > The compute_crc() function is responsible for calculating the > framebuffer CRC value; due to the XRGB format, this function has to > ignore the alpha channel during the CRC computation. Therefore, > compute_crc() set zero to the alpha channel directly in the input > framebuffer, which is not a problem since this function receives a copy > of the original buffer. However, if we want to use this function in a > context without a buffer copy, it will change the initial value. This > patch makes compute_crc() calculate the CRC value without modifying the > input framebuffer. > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > --- > drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++----------- > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index 258e659ecfba..686d25e7b01d 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -9,33 +9,40 @@ > > #include "vkms_drv.h" > > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer, > + const struct vkms_composer *composer) > +{ > + int src_offset = composer->offset + (y * composer->pitch) > + + (x * composer->cpp); > + > + return *(u32 *)&buffer[src_offset]; > +} > + > /** > * compute_crc - Compute CRC value on output frame > * > - * @vaddr_out: address to final framebuffer > + * @vaddr: address to final framebuffer > * @composer: framebuffer's metadata > * > * returns CRC value computed using crc32 on the visible portion of > * the final framebuffer at vaddr_out > */ > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer) > +static uint32_t compute_crc(const u8 *vaddr, > + const struct vkms_composer *composer) > { > - int i, j, src_offset; > + int x, y; > int x_src = composer->src.x1 >> 16; > int y_src = composer->src.y1 >> 16; > int h_src = drm_rect_height(&composer->src) >> 16; > int w_src = drm_rect_width(&composer->src) >> 16; > - u32 crc = 0; > + u32 crc = 0, pixel = 0; > > - for (i = y_src; i < y_src + h_src; ++i) { > - for (j = x_src; j < x_src + w_src; ++j) { > - src_offset = composer->offset > - + (i * composer->pitch) > - + (j * composer->cpp); > + for (y = y_src; y < y_src + h_src; ++y) { > + for (x = x_src; x < x_src + w_src; ++x) { > /* XRGB format ignores Alpha channel */ > - memset(vaddr_out + src_offset + 24, 0, 8); > - crc = crc32_le(crc, vaddr_out + src_offset, > - sizeof(u32)); > + pixel = get_pixel_from_buffer(x, y, vaddr, composer); > + bitmap_clear((void *)&pixel, 0, 8); > + crc = crc32_le(crc, (void *)&pixel, sizeof(u32)); > } > } > IMHO using something like the following makes the code far simpler and clearer. offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp); for (i = 0; i < h_src; i++, offset += composer->pitch) { for (j = 0; j < w_src; j++, offset += composer->cpp) { pixel = get_pixel_from_buffer(vaddr, offset); crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed } } With the bitmap_clear() and related comment moved into get_pixel_from_buffer(). -Emil
On Tue, 12 May 2020 at 12:34, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > Hi Rodrigo, > > On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote: > > > > From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > > > The compute_crc() function is responsible for calculating the > > framebuffer CRC value; due to the XRGB format, this function has to > > ignore the alpha channel during the CRC computation. Therefore, > > compute_crc() set zero to the alpha channel directly in the input > > framebuffer, which is not a problem since this function receives a copy > > of the original buffer. However, if we want to use this function in a > > context without a buffer copy, it will change the initial value. This > > patch makes compute_crc() calculate the CRC value without modifying the > > input framebuffer. > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > --- > > drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++----------- > > 1 file changed, 19 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > > index 258e659ecfba..686d25e7b01d 100644 > > --- a/drivers/gpu/drm/vkms/vkms_composer.c > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > > @@ -9,33 +9,40 @@ > > > > #include "vkms_drv.h" > > > > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer, > > + const struct vkms_composer *composer) > > +{ > > + int src_offset = composer->offset + (y * composer->pitch) > > + + (x * composer->cpp); > > + > > + return *(u32 *)&buffer[src_offset]; > > +} > > + > > /** > > * compute_crc - Compute CRC value on output frame > > * > > - * @vaddr_out: address to final framebuffer > > + * @vaddr: address to final framebuffer > > * @composer: framebuffer's metadata > > * > > * returns CRC value computed using crc32 on the visible portion of > > * the final framebuffer at vaddr_out > > */ > > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer) > > +static uint32_t compute_crc(const u8 *vaddr, > > + const struct vkms_composer *composer) > > { > > - int i, j, src_offset; > > + int x, y; > > int x_src = composer->src.x1 >> 16; > > int y_src = composer->src.y1 >> 16; > > int h_src = drm_rect_height(&composer->src) >> 16; > > int w_src = drm_rect_width(&composer->src) >> 16; > > - u32 crc = 0; > > + u32 crc = 0, pixel = 0; > > > > - for (i = y_src; i < y_src + h_src; ++i) { > > - for (j = x_src; j < x_src + w_src; ++j) { > > - src_offset = composer->offset > > - + (i * composer->pitch) > > - + (j * composer->cpp); > > + for (y = y_src; y < y_src + h_src; ++y) { > > + for (x = x_src; x < x_src + w_src; ++x) { > > /* XRGB format ignores Alpha channel */ > > - memset(vaddr_out + src_offset + 24, 0, 8); > > - crc = crc32_le(crc, vaddr_out + src_offset, > > - sizeof(u32)); > > + pixel = get_pixel_from_buffer(x, y, vaddr, composer); > > + bitmap_clear((void *)&pixel, 0, 8); > > + crc = crc32_le(crc, (void *)&pixel, sizeof(u32)); > > } > > } > > > IMHO using something like the following makes the code far simpler and clearer. > > offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp); > > for (i = 0; i < h_src; i++, offset += composer->pitch) { > for (j = 0; j < w_src; j++, offset += composer->cpp) { > pixel = get_pixel_from_buffer(vaddr, offset); > crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed > } > } > > With the bitmap_clear() and related comment moved into get_pixel_from_buffer(). > If you fold the bitmap_clear() in get_pixel_from_buffer(), and drop the cast (unless I'm missing something and it's really needed) for crc32_le() this patch is: Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> I would suggest (but it's not a requirement) that you simplify the loop/offset calculation as separate patch in v5. -Emil
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 258e659ecfba..686d25e7b01d 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -9,33 +9,40 @@ #include "vkms_drv.h" +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer, + const struct vkms_composer *composer) +{ + int src_offset = composer->offset + (y * composer->pitch) + + (x * composer->cpp); + + return *(u32 *)&buffer[src_offset]; +} + /** * compute_crc - Compute CRC value on output frame * - * @vaddr_out: address to final framebuffer + * @vaddr: address to final framebuffer * @composer: framebuffer's metadata * * returns CRC value computed using crc32 on the visible portion of * the final framebuffer at vaddr_out */ -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer) +static uint32_t compute_crc(const u8 *vaddr, + const struct vkms_composer *composer) { - int i, j, src_offset; + int x, y; int x_src = composer->src.x1 >> 16; int y_src = composer->src.y1 >> 16; int h_src = drm_rect_height(&composer->src) >> 16; int w_src = drm_rect_width(&composer->src) >> 16; - u32 crc = 0; + u32 crc = 0, pixel = 0; - for (i = y_src; i < y_src + h_src; ++i) { - for (j = x_src; j < x_src + w_src; ++j) { - src_offset = composer->offset - + (i * composer->pitch) - + (j * composer->cpp); + for (y = y_src; y < y_src + h_src; ++y) { + for (x = x_src; x < x_src + w_src; ++x) { /* XRGB format ignores Alpha channel */ - memset(vaddr_out + src_offset + 24, 0, 8); - crc = crc32_le(crc, vaddr_out + src_offset, - sizeof(u32)); + pixel = get_pixel_from_buffer(x, y, vaddr, composer); + bitmap_clear((void *)&pixel, 0, 8); + crc = crc32_le(crc, (void *)&pixel, sizeof(u32)); } }