[PATCH v2 3/4] drm/mediatek: Add casting before assign

[Jason-JH Lin (林睿祥)]

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 at mediatek.com>

> 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;



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230618/4ec7e613/attachment-0001.htm>