|
Hi Angelo,
Thanks for the reviews.
On Wed, 2023-06-14 at 10:43 +0200, AngeloGioacchino Del Regno wrote:
|
|
External email : Please do not click links or open attachments until you have verified the sender or the content.
|
Il 13/06/23 13:32, Jason-JH.Lin ha scritto:
> 1. Add casting before assign to avoid the unintentional integer
> overflow or unintended sign extension.
> 2. Add a int varriable for multiplier calculation instead of calculating
> different types multiplier with dma_addr_t varriable directly.
>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
> ---
> drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 22 +++++++++++++---------
> 2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index a25b28d3ee90..0c7878bc0b37 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -121,7 +121,7 @@ int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> int ret;
>
> args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> -args->size = args->pitch * args->height;
> +args->size = (__u64)args->pitch * args->height;
We could avoid explicit casting here if you do
args->size = args->pitch;
args->size *= args->height;
OK, Thanks for your advice.
I'll take this.
>
> mtk_gem = mtk_drm_gem_create(dev, args->size, false);
> if (IS_ERR(mtk_gem))
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 31f9420aff6f..1cd41454d545 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -145,6 +145,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> dma_addr_t addr;
> dma_addr_t hdr_addr = 0;
> unsigned int hdr_pitch = 0;
> +int offset;
...but offset can never be negative, can it?
in that case, this should be unsigned int.
src.x1 and src.y1 are 'int', so they can be negative.
But I'm not sure does anyone would set the negative to them.
I think using 'unsigned int' for offset would be very big after multiply with negative src.x1 or src.y1.
So I just use 'int' here to avoid that case.
Do you think I should use 'unsigned int' for offset and add the boundary checking for addr?
Regard,
Jason-JH.Lin
>
> gem = fb->obj[0];
> mtk_gem = to_mtk_gem_obj(gem);
> @@ -154,8 +155,10 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> modifier = fb->modifier;
>
> if (modifier == DRM_FORMAT_MOD_LINEAR) {
> -addr += (new_state->src.x1 >> 16) * fb->format->cpp[0];
> -addr += (new_state->src.y1 >> 16) * pitch;
> +offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];
> +addr += offset;
> +offset = (new_state->src.y1 >> 16) * pitch;
> +addr += offset;
> } else {
> int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
> / AFBC_DATA_BLOCK_WIDTH;
> @@ -163,21 +166,22 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> / AFBC_DATA_BLOCK_HEIGHT;
> int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;
> int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;
> -int hdr_size;
> +int hdr_size, hdr_offset;
>
> hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE;
> pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH *
> AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0];
>
> hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);
> +hdr_offset = hdr_pitch * y_offset_in_blocks +
> +AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
> +hdr_addr = addr + hdr_offset;
>
> -hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
> - AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
> /* The data plane is offset by 1 additional block. */
> -addr = addr + hdr_size +
> - pitch * y_offset_in_blocks +
> - AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
> - fb->format->cpp[0] * (x_offset_in_blocks + 1);
> +offset = pitch * y_offset_in_blocks +
> + AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
> + fb->format->cpp[0] * (x_offset_in_blocks + 1);
> +addr = addr + hdr_size + offset;
> }
>
> mtk_plane_state->pending.enable = true;
************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe
that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
| 