Re: [PATCH v2 3/5] drm/rockchip: vop: introduce VOP_REG_MASK

[Tomasz Figa <tfiga@chromium.org>]

Hi Mark,

Mark Yao <mark.yao <at> rock-chips.com> writes:

> 
> Some new vop register support mask, bit[16-31] is mask,
> bit[0-15] is value, the mask is correspond to the value.

Please see my comments inline.

> 
> Signed-off-by: Mark Yao <mark.yao <at> rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   45 ++++++++++++++-------
------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |    1 +
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |    9 +++++-
>  3 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 28596e7..59f24cd 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
[snip]
> -static inline void vop_mask_write_relaxed(struct vop *vop, uint32_t 
offset,
> -					  uint32_t mask, uint32_t v)
> -{
> -	if (mask) {
> +	if (write_mask) {
> +		v = (v << shift) | (mask << (shift + 16));

If the caller gives "v" too big, it might get over the bit 16 and affect
the mask. IMHO it would be much safer to mask (v << shift) with 0xffff
just in case.

> +	} else {
>  		uint32_t cached_val = vop->regsbak[offset >> 2];
> 
> -		cached_val = (cached_val & ~mask) | v;
> -		writel_relaxed(cached_val, vop->regs + offset);
> -		vop->regsbak[offset >> 2] = cached_val;
> +		v = (cached_val & ~(mask << shift)) | (v << shift);

Should we also mask "v" with "mask" for better safety?

Best regards,
Tomasz


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