| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 9 Apr 2019 11:17:30 +0200
From: Neil Armstrong <narmstrong@...libre.com>
To: Jerome Brunet <jbrunet@...libre.com>,
dri-devel@...ts.freedesktop.org
Cc: linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/11] drm/meson: Add G12A support for plane handling in
CRTC driver
On 09/04/2019 10:43, Jerome Brunet wrote:
> On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote:
>> This patch adds support for the new OSD+VD Plane blending module
>> in the CRTC code by adding the G12A code to manage the blending
>> module and setting the right OSD1 & VD1 plane registers.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
>> ---
>> drivers/gpu/drm/meson/meson_crtc.c | 269 +++++++++++++++++++++++------
>> drivers/gpu/drm/meson/meson_drv.h | 4 +
>> 2 files changed, 221 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
>> index 6d9311e254ef..5579f8ac3e3f 100644
>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>> @@ -39,12 +39,17 @@
>> #include "meson_viu.h"
>> #include "meson_registers.h"
>>
>> +#define MESON_G12A_VIU_OFFSET 0x5ec0
>> +
>> /* CRTC definition */
>>
>> struct meson_crtc {
>> struct drm_crtc base;
>> struct drm_pending_vblank_event *event;
>> struct meson_drm *priv;
>> + void (*enable_osd1)(struct meson_drm *priv);
>> + void (*enable_vd1)(struct meson_drm *priv);
>> + unsigned int viu_offset;
>> };
>> #define to_meson_crtc(x) container_of(x, struct meson_crtc, base)
>>
>> @@ -80,6 +85,44 @@ static const struct drm_crtc_funcs meson_crtc_funcs = {
>>
>> };
>>
>> +static void meson_g12a_crtc_atomic_enable(struct drm_crtc *crtc,
>> + struct drm_crtc_state *old_state)
>> +{
>> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
>> + struct drm_crtc_state *crtc_state = crtc->state;
>> + struct meson_drm *priv = meson_crtc->priv;
>> +
>> + DRM_DEBUG_DRIVER("\n");
>> +
>> + if (!crtc_state) {
>> + DRM_ERROR("Invalid crtc_state\n");
>> + return;
>> + }
>> +
>> + /* VD1 Preblend vertical start/end */
>> + writel(FIELD_PREP(GENMASK(11, 0), 2303),
>
> Could could define this mask somewhere
> I don't know if the 2303 can be explained. I'd nice if it could
It's a width, I'll add a comment.
>
>> + priv->io_base + _REG(VPP_PREBLEND_VD1_V_START_END));
>> +
>> + /* Setup Blender */
>> + writel(crtc_state->mode.hdisplay |
>> + crtc_state->mode.vdisplay << 16,
>> + priv->io_base + _REG(VPP_POSTBLEND_H_SIZE));
>> +
>> + writel_relaxed(0 << 16 |
>
> this 16 shift seems to be used for hdisplay or something. Could
> define/document it a bit
Ack
>
>> + (crtc_state->mode.hdisplay - 1),
>> + priv->io_base + _REG(VPP_OSD1_BLD_H_SCOPE));
>> + writel_relaxed(0 << 16 |
>> + (crtc_state->mode.vdisplay - 1),
>> + priv->io_base + _REG(VPP_OSD1_BLD_V_SCOPE));
>> + writel_relaxed(crtc_state->mode.hdisplay << 16 |
>> + crtc_state->mode.vdisplay,
>> + priv->io_base + _REG(VPP_OUT_H_V_SIZE));
>> +
>> + drm_crtc_vblank_on(crtc);
>> +
>> + priv->viu.osd1_enabled = true;
>> +}
>> +
>> static void meson_crtc_atomic_enable(struct drm_crtc *crtc,
>> struct drm_crtc_state *old_state)
>> {
>> @@ -110,6 +153,31 @@ static void meson_crtc_atomic_enable(struct drm_crtc *crtc,
>> priv->viu.osd1_enabled = true;
>> }
>>
>> +static void meson_g12a_crtc_atomic_disable(struct drm_crtc *crtc,
>> + struct drm_crtc_state *old_state)
>> +{
>> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
>> + struct meson_drm *priv = meson_crtc->priv;
>> +
>> + DRM_DEBUG_DRIVER("\n");
>> +
>> + drm_crtc_vblank_off(crtc);
>> +
>> + priv->viu.osd1_enabled = false;
>> + priv->viu.osd1_commit = false;
>> +
>> + priv->viu.vd1_enabled = false;
>> + priv->viu.vd1_commit = false;
>> +
>> + if (crtc->state->event && !crtc->state->active) {
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> + spin_unlock_irq(&crtc->dev->event_lock);
>> +
>> + crtc->state->event = NULL;
>> + }
>> +}
>> +
>> static void meson_crtc_atomic_disable(struct drm_crtc *crtc,
>> struct drm_crtc_state *old_state)
>> {
>> @@ -173,6 +241,53 @@ static const struct drm_crtc_helper_funcs meson_crtc_helper_funcs = {
>> .atomic_disable = meson_crtc_atomic_disable,
>> };
>>
>> +static const struct drm_crtc_helper_funcs meson_g12a_crtc_helper_funcs = {
>> + .atomic_begin = meson_crtc_atomic_begin,
>> + .atomic_flush = meson_crtc_atomic_flush,
>> + .atomic_enable = meson_g12a_crtc_atomic_enable,
>> + .atomic_disable = meson_g12a_crtc_atomic_disable,
>> +};
>> +
>> +static void meson_crtc_enable_osd1(struct meson_drm *priv)
>> +{
>> + writel_bits_relaxed(VPP_OSD1_POSTBLEND, VPP_OSD1_POSTBLEND,
>> + priv->io_base + _REG(VPP_MISC));
>> +}
>> +
>> +static void meson_g12a_crtc_enable_osd1(struct meson_drm *priv)
>> +{
>> + writel_relaxed(priv->viu.osd_blend_din0_scope_h,
>> + priv->io_base +
>> + _REG(VIU_OSD_BLEND_DIN0_SCOPE_H));
>> + writel_relaxed(priv->viu.osd_blend_din0_scope_v,
>> + priv->io_base +
>> + _REG(VIU_OSD_BLEND_DIN0_SCOPE_V));
>> + writel_relaxed(priv->viu.osb_blend0_size,
>> + priv->io_base +
>> + _REG(VIU_OSD_BLEND_BLEND0_SIZE));
>> + writel_relaxed(priv->viu.osb_blend1_size,
>> + priv->io_base +
>> + _REG(VIU_OSD_BLEND_BLEND1_SIZE));
>> +}
>> +
>> +static void meson_crtc_enable_vd1(struct meson_drm *priv)
>> +{
>> + writel_bits_relaxed(VPP_VD1_PREBLEND | VPP_VD1_POSTBLEND |
>> + VPP_COLOR_MNG_ENABLE,
>> + VPP_VD1_PREBLEND | VPP_VD1_POSTBLEND |
>> + VPP_COLOR_MNG_ENABLE,
>> + priv->io_base + _REG(VPP_MISC));
>> +}
>> +
>> +static void meson_g12a_crtc_enable_vd1(struct meson_drm *priv)
>> +{
>> + writel_relaxed(((1 << 16) | /* post bld premult*/
>> + (1 << 8) | /* post src */
>> + (1 << 4) | /* pre bld premult*/
>> + (1 << 0)),
>
> Could you use the BIT() macro here and possibly some defines ?
Ack
>
>> + priv->io_base + _REG(VD1_BLEND_SRC_CTRL));
>> +}
>> +
>> void meson_crtc_irq(struct meson_drm *priv)
>> {
>> struct meson_crtc *meson_crtc = to_meson_crtc(priv->crtc);
>> @@ -219,8 +334,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>> MESON_CANVAS_BLKMODE_LINEAR, 0);
>>
>> /* Enable OSD1 */
>> - writel_bits_relaxed(VPP_OSD1_POSTBLEND, VPP_OSD1_POSTBLEND,
>> - priv->io_base + _REG(VPP_MISC));
>> + if (meson_crtc->enable_osd1)
>> + meson_crtc->enable_osd1(priv);
>>
>> priv->viu.osd1_commit = false;
>> }
>> @@ -261,89 +376,133 @@ void meson_crtc_irq(struct meson_drm *priv)
>> };
>>
>> writel_relaxed(priv->viu.vd1_if0_gen_reg,
>> - priv->io_base + _REG(VD1_IF0_GEN_REG));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_GEN_REG));
>
> Could you make a local variable for priv->io_base + meson_crtc->viu_offset ?
> It would be a little easier to read
Indeed
>
>> writel_relaxed(priv->viu.vd1_if0_gen_reg,
>> - priv->io_base + _REG(VD2_IF0_GEN_REG));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_GEN_REG));
>> writel_relaxed(priv->viu.vd1_if0_gen_reg2,
>> - priv->io_base + _REG(VD1_IF0_GEN_REG2));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_GEN_REG2));
>> writel_relaxed(priv->viu.viu_vd1_fmt_ctrl,
>> - priv->io_base + _REG(VIU_VD1_FMT_CTRL));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VIU_VD1_FMT_CTRL));
>> writel_relaxed(priv->viu.viu_vd1_fmt_ctrl,
>> - priv->io_base + _REG(VIU_VD2_FMT_CTRL));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VIU_VD2_FMT_CTRL));
>> writel_relaxed(priv->viu.viu_vd1_fmt_w,
>> - priv->io_base + _REG(VIU_VD1_FMT_W));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VIU_VD1_FMT_W));
>> writel_relaxed(priv->viu.viu_vd1_fmt_w,
>> - priv->io_base + _REG(VIU_VD2_FMT_W));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VIU_VD2_FMT_W));
>> writel_relaxed(priv->viu.vd1_if0_canvas0,
>> - priv->io_base + _REG(VD1_IF0_CANVAS0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CANVAS0));
>> writel_relaxed(priv->viu.vd1_if0_canvas0,
>> - priv->io_base + _REG(VD1_IF0_CANVAS1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CANVAS1));
>> writel_relaxed(priv->viu.vd1_if0_canvas0,
>> - priv->io_base + _REG(VD2_IF0_CANVAS0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CANVAS0));
>> writel_relaxed(priv->viu.vd1_if0_canvas0,
>> - priv->io_base + _REG(VD2_IF0_CANVAS1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CANVAS1));
>> writel_relaxed(priv->viu.vd1_if0_luma_x0,
>> - priv->io_base + _REG(VD1_IF0_LUMA_X0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA_X0));
>> writel_relaxed(priv->viu.vd1_if0_luma_x0,
>> - priv->io_base + _REG(VD1_IF0_LUMA_X1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA_X1));
>> writel_relaxed(priv->viu.vd1_if0_luma_x0,
>> - priv->io_base + _REG(VD2_IF0_LUMA_X0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA_X0));
>> writel_relaxed(priv->viu.vd1_if0_luma_x0,
>> - priv->io_base + _REG(VD2_IF0_LUMA_X1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA_X1));
>> writel_relaxed(priv->viu.vd1_if0_luma_y0,
>> - priv->io_base + _REG(VD1_IF0_LUMA_Y0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA_Y0));
>> writel_relaxed(priv->viu.vd1_if0_luma_y0,
>> - priv->io_base + _REG(VD1_IF0_LUMA_Y1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA_Y1));
>> writel_relaxed(priv->viu.vd1_if0_luma_y0,
>> - priv->io_base + _REG(VD2_IF0_LUMA_Y0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA_Y0));
>> writel_relaxed(priv->viu.vd1_if0_luma_y0,
>> - priv->io_base + _REG(VD2_IF0_LUMA_Y1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA_Y1));
>> writel_relaxed(priv->viu.vd1_if0_chroma_x0,
>> - priv->io_base + _REG(VD1_IF0_CHROMA_X0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA_X0));
>> writel_relaxed(priv->viu.vd1_if0_chroma_x0,
>> - priv->io_base + _REG(VD1_IF0_CHROMA_X1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA_X1));
>> writel_relaxed(priv->viu.vd1_if0_chroma_x0,
>> - priv->io_base + _REG(VD2_IF0_CHROMA_X0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA_X0));
>> writel_relaxed(priv->viu.vd1_if0_chroma_x0,
>> - priv->io_base + _REG(VD2_IF0_CHROMA_X1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA_X1));
>> writel_relaxed(priv->viu.vd1_if0_chroma_y0,
>> - priv->io_base + _REG(VD1_IF0_CHROMA_Y0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA_Y0));
>> writel_relaxed(priv->viu.vd1_if0_chroma_y0,
>> - priv->io_base + _REG(VD1_IF0_CHROMA_Y1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA_Y1));
>> writel_relaxed(priv->viu.vd1_if0_chroma_y0,
>> - priv->io_base + _REG(VD2_IF0_CHROMA_Y0));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA_Y0));
>> writel_relaxed(priv->viu.vd1_if0_chroma_y0,
>> - priv->io_base + _REG(VD2_IF0_CHROMA_Y1));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA_Y1));
>> writel_relaxed(priv->viu.vd1_if0_repeat_loop,
>> - priv->io_base + _REG(VD1_IF0_RPT_LOOP));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_RPT_LOOP));
>> writel_relaxed(priv->viu.vd1_if0_repeat_loop,
>> - priv->io_base + _REG(VD2_IF0_RPT_LOOP));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_RPT_LOOP));
>> writel_relaxed(priv->viu.vd1_if0_luma0_rpt_pat,
>> - priv->io_base + _REG(VD1_IF0_LUMA0_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA0_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_luma0_rpt_pat,
>> - priv->io_base + _REG(VD2_IF0_LUMA0_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA0_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_luma0_rpt_pat,
>> - priv->io_base + _REG(VD1_IF0_LUMA1_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA1_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_luma0_rpt_pat,
>> - priv->io_base + _REG(VD2_IF0_LUMA1_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA1_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_chroma0_rpt_pat,
>> - priv->io_base + _REG(VD1_IF0_CHROMA0_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA0_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_chroma0_rpt_pat,
>> - priv->io_base + _REG(VD2_IF0_CHROMA0_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA0_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_chroma0_rpt_pat,
>> - priv->io_base + _REG(VD1_IF0_CHROMA1_RPT_PAT));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA1_RPT_PAT));
>> writel_relaxed(priv->viu.vd1_if0_chroma0_rpt_pat,
>> - priv->io_base + _REG(VD2_IF0_CHROMA1_RPT_PAT));
>> - writel_relaxed(0, priv->io_base + _REG(VD1_IF0_LUMA_PSEL));
>> - writel_relaxed(0, priv->io_base + _REG(VD1_IF0_CHROMA_PSEL));
>> - writel_relaxed(0, priv->io_base + _REG(VD2_IF0_LUMA_PSEL));
>> - writel_relaxed(0, priv->io_base + _REG(VD2_IF0_CHROMA_PSEL));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA1_RPT_PAT));
>> + writel_relaxed(0, priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_LUMA_PSEL));
>> + writel_relaxed(0, priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_CHROMA_PSEL));
>> + writel_relaxed(0, priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_LUMA_PSEL));
>> + writel_relaxed(0, priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD2_IF0_CHROMA_PSEL));
>> writel_relaxed(priv->viu.vd1_range_map_y,
>> - priv->io_base + _REG(VD1_IF0_RANGE_MAP_Y));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_RANGE_MAP_Y));
>> writel_relaxed(priv->viu.vd1_range_map_cb,
>> - priv->io_base + _REG(VD1_IF0_RANGE_MAP_CB));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_RANGE_MAP_CB));
>> writel_relaxed(priv->viu.vd1_range_map_cr,
>> - priv->io_base + _REG(VD1_IF0_RANGE_MAP_CR));
>> + priv->io_base + meson_crtc->viu_offset +
>> + _REG(VD1_IF0_RANGE_MAP_CR));
>> writel_relaxed(0x78404,
>> priv->io_base + _REG(VPP_SC_MISC));
>> writel_relaxed(priv->viu.vpp_pic_in_height,
>> @@ -389,11 +548,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>> writel_relaxed(0x42, priv->io_base + _REG(VPP_SCALE_COEF_IDX));
>>
>> /* Enable VD1 */
>> - writel_bits_relaxed(VPP_VD1_PREBLEND | VPP_VD1_POSTBLEND |
>> - VPP_COLOR_MNG_ENABLE,
>> - VPP_VD1_PREBLEND | VPP_VD1_POSTBLEND |
>> - VPP_COLOR_MNG_ENABLE,
>> - priv->io_base + _REG(VPP_MISC));
>> + if (meson_crtc->enable_vd1)
>> + meson_crtc->enable_vd1(priv);
>>
>> priv->viu.vd1_commit = false;
>> }
>> @@ -430,7 +586,16 @@ int meson_crtc_create(struct meson_drm *priv)
>> return ret;
>> }
>>
>> - drm_crtc_helper_add(crtc, &meson_crtc_helper_funcs);
>> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
>> + meson_crtc->enable_osd1 = meson_g12a_crtc_enable_osd1;
>> + meson_crtc->enable_vd1 = meson_g12a_crtc_enable_vd1;
>> + meson_crtc->viu_offset = MESON_G12A_VIU_OFFSET;
>> + drm_crtc_helper_add(crtc, &meson_g12a_crtc_helper_funcs);
>> + } else {
>> + meson_crtc->enable_osd1 = meson_crtc_enable_osd1;
>> + meson_crtc->enable_vd1 = meson_crtc_enable_vd1;
>> + drm_crtc_helper_add(crtc, &meson_crtc_helper_funcs);
>> + }
>>
>> priv->crtc = crtc;
>>
>> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
>> index 214a7cb18ce2..9614baa836b9 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.h
>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>> @@ -62,6 +62,10 @@ struct meson_drm {
>> uint32_t osd_sc_h_phase_step;
>> uint32_t osd_sc_h_ctrl0;
>> uint32_t osd_sc_v_ctrl0;
>> + uint32_t osd_blend_din0_scope_h;
>> + uint32_t osd_blend_din0_scope_v;
>> + uint32_t osb_blend0_size;
>> + uint32_t osb_blend1_size;
>>
>> bool vd1_enabled;
>> bool vd1_commit;
>
>
Will fix in a follow-up patch.
Neil
Powered by blists - more mailing lists