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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
