Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version

From: Sean Paul
Date: Thu Jul 13 2017 - 16:33:56 EST

Next message: Gwendal Grignou: "Re: [PATCH v3 1/4] mfd: cros_ec: Get rid of cros_ec_check_features from cros_ec_dev."
Previous message: Andrea Arcangeli: "Re: [RFC PATCH 1/1] mm/mremap: add MREMAP_MIRROR flag for existing mirroring functionality"
In reply to: Mark yao: "Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version"
Next in thread: Mark Yao: "[PATCH v2 1/5] drm/rockchip: vop: get rid of register init table"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jul 13, 2017 at 09:45:12AM +0800, Mark yao wrote:
> On 2017å07æ13æ 01:53, Sean Paul wrote:
> > On Wed, Jul 12, 2017 at 10:03:38AM +0800, Mark Yao wrote:
> >
> > Please add a commit message describing *what* and *why* you are making the
> > change.
>
> Got it, I will fix it at next version.
>
> Thanks.
> >
> > > Signed-off-by: Mark Yao <mark.yao@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 66 +++++++++++++++++++++--------
> > > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 18 ++++++--
> > > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 20 ++++++---
> > > 3 files changed, 77 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > index 7a5f809..a9180fd 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > @@ -42,33 +42,60 @@
> > > #include "rockchip_drm_psr.h"
> > > #include "rockchip_drm_vop.h"
> > > -#define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
> > > - vop_mask_write(x, off, mask, shift, v, write_mask, true)
> > > +#define VOP_REG_SUPPORT(vop, reg) \
> > > + (!reg.major || (reg.major == VOP_MAJOR(vop->data->version) && \
> > > + reg.begin_minor <= VOP_MINOR(vop->data->version) && \
> > > + reg.end_minor >= VOP_MINOR(vop->data->version) && \
> > > + reg.mask))
> > This would be better suited as a static inline function. As would many of the
> > macros below.
>
> Got it, will fix it at next version.
> >
> > > -#define __REG_SET_NORMAL(x, off, mask, shift, v, write_mask) \
> > > - vop_mask_write(x, off, mask, shift, v, write_mask, false)
> > > +#define VOP_WIN_SUPPORT(vop, win, name) \
> > > + VOP_REG_SUPPORT(vop, win->phy->name)
> > > -#define REG_SET(x, base, reg, v, mode) \
> > > - __REG_SET_##mode(x, base + reg.offset, \
> > > - reg.mask, reg.shift, v, reg.write_mask)
> > > -#define REG_SET_MASK(x, base, reg, mask, v, mode) \
> > > - __REG_SET_##mode(x, base + reg.offset, \
> > > - mask, reg.shift, v, reg.write_mask)
> > > +#define VOP_CTRL_SUPPORT(vop, name) \
> > > + VOP_REG_SUPPORT(vop, vop->data->ctrl->name)
> > > +
> > > +#define VOP_INTR_SUPPORT(vop, name) \
> > > + VOP_REG_SUPPORT(vop, vop->data->intr->name)
> > > +
> > > +#define __REG_SET(x, off, mask, shift, v, write_mask, relaxed) \
> > > + vop_mask_write(x, off, mask, shift, v, write_mask, relaxed)
> > There's really no point to this, just call vop_mask_write directly.
>
> Got it, will fix it at next version.
> >
> > > +
> > > +#define _REG_SET(vop, name, off, reg, mask, v, relaxed) \
> > > + do { \
> > > + if (VOP_REG_SUPPORT(vop, reg)) \
> > > + __REG_SET(vop, off + reg.offset, mask, reg.shift, \
> > s/mask/reg.mask & mask/
>
> Got it, will fix it at next version.
> >
> > > + v, reg.write_mask, relaxed); \
> > > + else \
> > > + dev_dbg(vop->dev, "Warning: not support "#name"\n"); \
> > > + } while (0)
> >
> > Also better as static inline, IMO.
>
> Good idea, I will try it.
>
> >
> > > +
> > > +#define REG_SET(x, name, off, reg, v, relaxed) \
> > > + _REG_SET(x, name, off, reg, reg.mask, v, relaxed)
> > s/reg.mask/~0/
>
> Got it, will fix it at next version.
> >
> > > +#define REG_SET_MASK(x, name, off, reg, mask, v, relaxed) \
> > > + _REG_SET(x, name, off, reg, reg.mask & mask, v, relaxed)
> > s/reg.mask &//
> >
> > Also, these can become static inline functions as well.
>
> Got it, will fix it at next version.
> >
> > > #define VOP_WIN_SET(x, win, name, v) \
> > > - REG_SET(x, win->base, win->phy->name, v, RELAXED)
> > > + REG_SET(x, name, win->base, win->phy->name, v, true)
> > > +#define VOP_WIN_SET_EXT(x, win, ext, name, v) \
> > > + REG_SET(x, name, 0, win->ext->name, v, true)
> > > #define VOP_SCL_SET(x, win, name, v) \
> > > - REG_SET(x, win->base, win->phy->scl->name, v, RELAXED)
> > > + REG_SET(x, name, win->base, win->phy->scl->name, v, true)
> > > #define VOP_SCL_SET_EXT(x, win, name, v) \
> > > - REG_SET(x, win->base, win->phy->scl->ext->name, v, RELAXED)
> > > + REG_SET(x, name, win->base, win->phy->scl->ext->name, v, true)
> > > +
> > > #define VOP_CTRL_SET(x, name, v) \
> > > - REG_SET(x, 0, (x)->data->ctrl->name, v, NORMAL)
> > > + REG_SET(x, name, 0, (x)->data->ctrl->name, v, false)
> > > #define VOP_INTR_GET(vop, name) \
> > > vop_read_reg(vop, 0, &vop->data->ctrl->name)
> > > -#define VOP_INTR_SET(vop, name, mask, v) \
> > > - REG_SET_MASK(vop, 0, vop->data->intr->name, mask, v, NORMAL)
> > > +#define VOP_INTR_SET(vop, name, v) \
> > > + REG_SET(vop, name, 0, vop->data->intr->name, \
> > > + v, false)
> > > +#define VOP_INTR_SET_MASK(vop, name, mask, v) \
> > > + REG_SET_MASK(vop, name, 0, vop->data->intr->name, \
> > > + mask, v, false)
> > > +
> > > #define VOP_INTR_SET_TYPE(vop, name, type, v) \
> > > do { \
> > > int i, reg = 0, mask = 0; \
> > > @@ -78,13 +105,16 @@
> > > mask |= 1 << i; \
> > > } \
> > > } \
> > > - VOP_INTR_SET(vop, name, mask, reg); \
> > > + VOP_INTR_SET_MASK(vop, name, mask, reg); \
> > > } while (0)
> > > #define VOP_INTR_GET_TYPE(vop, name, type) \
> > > vop_get_intr_type(vop, &vop->data->intr->name, type)
> > > +#define VOP_CTRL_GET(x, name) \
> > > + vop_read_reg(x, 0, &vop->data->ctrl->name)
> > > +
> > > #define VOP_WIN_GET(x, win, name) \
> > > - vop_read_reg(x, win->base, &win->phy->name)
> > > + vop_read_reg(x, win->offset, win->phy->name)
> > > #define VOP_WIN_GET_YRGBADDR(vop, win) \
> > > vop_readl(vop, win->base + win->phy->yrgb_mst.offset)
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> > > index 084d3b2..e4de890 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> > > @@ -15,6 +15,14 @@
> > > #ifndef _ROCKCHIP_DRM_VOP_H
> > > #define _ROCKCHIP_DRM_VOP_H
> > > +/*
> > > + * major: IP major vertion, used for IP structure
> > s/vertion/version
>
> Got it, will fix it at next version.
> Thanks.
>
> >
> > > + * minor: big feature change under same structure
> > > + */
> > > +#define VOP_VERSION(major, minor) ((major) << 8 | (minor))
> > > +#define VOP_MAJOR(version) ((version) >> 8)
> > > +#define VOP_MINOR(version) ((version) & 0xff)
> > > +
> > > enum vop_data_format {
> > > VOP_FMT_ARGB8888 = 0,
> > > VOP_FMT_RGB888,
> > > @@ -25,10 +33,13 @@ enum vop_data_format {
> > > };
> > > struct vop_reg {
> > > - uint32_t offset;
> > > - uint32_t shift;
> > > uint32_t mask;
> > > - bool write_mask;
> > > + uint32_t offset:12;
> > > + uint32_t shift:5;
> > > + uint32_t begin_minor:4;
> > > + uint32_t end_minor:4;
> > > + uint32_t major:3;
> > > + uint32_t write_mask:1;
> > Why are you packing this?
>
> make struct vop_reg not too big, jus u64 size, struct vop_reg use a lot on register define, packing it would reduce register table size.
>

Ehhh, let's not get too fancy. If you want to save space, you can use uint8_t
and uint16_t. Packing like this makes it look like you're sending the struct
elsewhere, which is just confusing.

Sean

> >
> > > };
> > > struct vop_ctrl {
> > > @@ -134,6 +145,7 @@ struct vop_win_data {
> > > };
> > > struct vop_data {
> > > + uint32_t version;
> > > const struct vop_ctrl *ctrl;
> > > const struct vop_intr *intr;
> > > const struct vop_win_data *win;
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> > > index 00e9d79..7744603 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> > > @@ -20,17 +20,25 @@
> > > #include "rockchip_drm_vop.h"
> > > #include "rockchip_vop_reg.h"
> > > -#define VOP_REG(off, _mask, s) \
> > > +#define VOP_REG_VER_MASK(off, _mask, s, _write_mask, _major, \
> > > + _begin_minor, _end_minor) \
> > > {.offset = off, \
> > > .mask = _mask, \
> > > .shift = s, \
> > > - .write_mask = false,}
> > > + .write_mask = _write_mask, \
> > > + .major = _major, \
> > > + .begin_minor = _begin_minor, \
> > > + .end_minor = _end_minor,}
> > > +
> > > +#define VOP_REG_VER(off, _mask, s, _major, _begin_minor, _end_minor) \
> > > + VOP_REG_VER_MASK(off, _mask, s, false, \
> > > + _major, _begin_minor, _end_minor)
> > > +
> > > +#define VOP_REG(off, _mask, s) \
> > > + VOP_REG_VER(off, _mask, s, 0, 0, -1)
> > > #define VOP_REG_MASK(off, _mask, s) \
> > > - {.offset = off, \
> > > - .mask = _mask, \
> > > - .shift = s, \
> > > - .write_mask = true,}
> > > + VOP_REG_VER_MASK(off, _mask, s, true, 0, 0, -1)
> > > static const uint32_t formats_win_full[] = {
> > > DRM_FORMAT_XRGB8888,
> > > --
> > > 1.9.1
> > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> --
> ïark Yao
>
>

--
Sean Paul, Software Engineer, Google / Chromium OS

Next message: Gwendal Grignou: "Re: [PATCH v3 1/4] mfd: cros_ec: Get rid of cros_ec_check_features from cros_ec_dev."
Previous message: Andrea Arcangeli: "Re: [RFC PATCH 1/1] mm/mremap: add MREMAP_MIRROR flag for existing mirroring functionality"
In reply to: Mark yao: "Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version"
Next in thread: Mark Yao: "[PATCH v2 1/5] drm/rockchip: vop: get rid of register init table"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]