| Message ID | ea7e3a0daa4ee502d8ec67a010120d53f88fa06b.1561491964.git.rodrigosiqueiramelo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Introduces writeback support | expand |
On Tue, 2019-06-25 at 22:38 -0300, Rodrigo Siqueira wrote: > 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 51a270514219..8126aa0f968f 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -6,33 +6,40 @@ > #include <drm/drm_atomic_helper.h> > #include <drm/drm_gem_framebuffer_helper.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); Hi Rodgrigo, Do I understand correctly, that previous version with memset was actually zeroing out the whole fb, except first 24 bytes? On the first iteration bytes 24..32 would be zeroed, on the second 25..33, etc. Should we add more CRC tests to IGT, so we can catch such mistakes? For example, compute CRC, than augment random pixel and assert that CRC changed. Oleg > - 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)); > } > } >
On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote: > 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. Uh why? For writeback we're writing the output too, so we can write whatever we want to into the alpha channel. For writeback we should never accept a pixel format where alpha actually matters, that doesn't make sense. You can't see through a real screen either, they are all opaque :-) -Daniel > > 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 51a270514219..8126aa0f968f 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -6,33 +6,40 @@ > #include <drm/drm_atomic_helper.h> > #include <drm/drm_gem_framebuffer_helper.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)); > } > } > > -- > 2.21.0
On Thursday, July 11, 2019 11:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote: > > > 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. > > Uh why? For writeback we're writing the output too, so we can write > whatever we want to into the alpha channel. For writeback we should never > accept a pixel format where alpha actually matters, that doesn't make > sense. You can't see through a real screen either, they are all opaque :-) I'm not sure about that. See e.g. https://en.wikipedia.org/wiki/See-through_display Many drivers already accept FBs with alpha channels for the primary plane. https://drmdb.emersion.fr/formats?plane=1 Just making sure we aren't painting ourselves into a corner. :P
On Thu, Jul 11, 2019 at 10:28 AM Simon Ser <contact@emersion.fr> wrote: > > On Thursday, July 11, 2019 11:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote: > > > > > 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. > > > > Uh why? For writeback we're writing the output too, so we can write > > whatever we want to into the alpha channel. For writeback we should never > > accept a pixel format where alpha actually matters, that doesn't make > > sense. You can't see through a real screen either, they are all opaque :-) > > I'm not sure about that. See e.g. > https://en.wikipedia.org/wiki/See-through_display They have variable opaqueness, independent of the color value? > Many drivers already accept FBs with alpha channels for the primary > plane. > https://drmdb.emersion.fr/formats?plane=1 If you have a background color (we assume it to be black) that makes sense. Still doesn't mean we render transparent output, we don't. > Just making sure we aren't painting ourselves into a corner. :P You can add ARGB to your writeback format support list, there is no corner here at all to get into (at least in the abstract sense). -Daniel
On 07/11, Daniel Vetter wrote: > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote: > > 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. > > Uh why? For writeback we're writing the output too, so we can write > whatever we want to into the alpha channel. For writeback we should never > accept a pixel format where alpha actually matters, that doesn't make > sense. You can't see through a real screen either, they are all opaque :-) > -Daniel Hmmm, I see your point and I agree, but even though we can write whatever we want in the output, don’t you think that is weird to change the framebuffer value in the compute_crc() function? Thanks > > > > 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 51a270514219..8126aa0f968f 100644 > > --- a/drivers/gpu/drm/vkms/vkms_composer.c > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > > @@ -6,33 +6,40 @@ > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_gem_framebuffer_helper.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)); > > } > > } > > > > -- > > 2.21.0 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jul 12, 2019 at 12:14:49AM -0300, Rodrigo Siqueira wrote: > On 07/11, Daniel Vetter wrote: > > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote: > > > 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. > > > > Uh why? For writeback we're writing the output too, so we can write > > whatever we want to into the alpha channel. For writeback we should never > > accept a pixel format where alpha actually matters, that doesn't make > > sense. You can't see through a real screen either, they are all opaque :-) > > -Daniel > > Hmmm, > > I see your point and I agree, but even though we can write whatever we > want in the output, don’t you think that is weird to change the > framebuffer value in the compute_crc() function? Not sure what you mean here ... ? From a quick look the memset only sets our temporary buffer, so we're not changing the input framebuffer here. And we have to somehow get rid of the X bits, since there's no alpha value. For CRC computation, all we need is some value which is the same for every frame (so that the CRC stays constant for the same visible output). For writeback we could write whatever we want (which includes whatever is there already). But there's no guarantee and definitely no expectation that the X bits survive. Writing 0 is imo the most reasonable thing to do. I'm not even sure whether modern gpus can still do channel masking (i.e. only write out specific channels, instead of the entire color). That was a "feature" of bitop blitters of the 80s/90s :-) -Daniel > > Thanks > > > > > > > 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 51a270514219..8126aa0f968f 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > > > @@ -6,33 +6,40 @@ > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_gem_framebuffer_helper.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)); > > > } > > > } > > > > > > -- > > > 2.21.0 > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Rodrigo Siqueira > https://siqueira.tech > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 07/16, Daniel Vetter wrote: > On Fri, Jul 12, 2019 at 12:14:49AM -0300, Rodrigo Siqueira wrote: > > On 07/11, Daniel Vetter wrote: > > > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote: > > > > 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. > > > > > > Uh why? For writeback we're writing the output too, so we can write > > > whatever we want to into the alpha channel. For writeback we should never > > > accept a pixel format where alpha actually matters, that doesn't make > > > sense. You can't see through a real screen either, they are all opaque :-) > > > -Daniel > > > > Hmmm, > > > > I see your point and I agree, but even though we can write whatever we > > want in the output, don’t you think that is weird to change the > > framebuffer value in the compute_crc() function? > > Not sure what you mean here ... ? From a quick look the memset only sets > our temporary buffer, so we're not changing the input framebuffer here. > And we have to somehow get rid of the X bits, since there's no alpha > value. For CRC computation, all we need is some value which is the same > for every frame (so that the CRC stays constant for the same visible > output). For writeback we could write whatever we want (which includes > whatever is there already). But there's no guarantee and definitely no > expectation that the X bits survive. Writing 0 is imo the most reasonable > thing to do. I'm not even sure whether modern gpus can still do channel > masking (i.e. only write out specific channels, instead of the entire > color). That was a "feature" of bitop blitters of the 80s/90s :-) > -Daniel Ahhh, now I can see that my mindset was in the 90s :) Thanks > > > > Thanks > > > > > > > > > > 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 51a270514219..8126aa0f968f 100644 > > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c > > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > > > > @@ -6,33 +6,40 @@ > > > > #include <drm/drm_atomic_helper.h> > > > > #include <drm/drm_gem_framebuffer_helper.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)); > > > > } > > > > } > > > > > > > > -- > > > > 2.21.0 > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Rodrigo Siqueira > > https://siqueira.tech > > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 51a270514219..8126aa0f968f 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -6,33 +6,40 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.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)); } }
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(-)