From: Mark yao <mark.yao@rock-chips.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
linux-rockchip@lists.infradead.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version
Date: Thu, 13 Jul 2017 09:45:12 +0800 [thread overview]
Message-ID: <5966D0A8.6060003@rock-chips.com> (raw)
In-Reply-To: <20170712175305.n7kk7dzvfs2vywz3@art_vandelay>
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@rock-chips.com>
>> ---
>> 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.
>
>> };
>>
>> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Mark Yao
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-07-13 1:45 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-12 2:03 [PATCH v2 0/5] drm/rockchip: add all full framework vop support Mark Yao
2017-07-12 2:03 ` [PATCH v2 1/5] drm/rockchip: vop: get rid of register init table Mark Yao
2017-07-12 16:47 ` Sean Paul
2017-07-13 1:33 ` Mark yao
2017-07-13 20:29 ` Sean Paul
2017-07-17 2:10 ` Mark yao
2017-07-12 2:03 ` [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version Mark Yao
2017-07-12 17:53 ` Sean Paul
2017-07-13 1:45 ` Mark yao [this message]
2017-07-13 20:33 ` Sean Paul
2017-07-12 2:03 ` [PATCH v2 3/5] drm/rockchip: vop: move line_flag_num to interrupt registers Mark Yao
2017-07-12 17:54 ` Sean Paul
2017-07-13 1:46 ` Mark yao
2017-07-12 2:03 ` [PATCH v2 4/5] drm/rockchip: vop: add a series of vop support Mark Yao
2017-07-12 18:33 ` Sean Paul
2017-07-13 1:31 ` Mark yao
2017-07-13 20:28 ` Sean Paul
2017-07-12 2:04 ` [PATCH v2 5/5] dt-bindings: display: fill Documents for series of vop Mark Yao
[not found] ` <1499825042-7652-1-git-send-email-mark.yao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2017-07-17 17:24 ` Rob Herring
2017-07-20 2:10 ` Mark yao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5966D0A8.6060003@rock-chips.com \
--to=mark.yao@rock-chips.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=seanpaul@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).