Messages in this thread | | | Date | Fri, 12 Apr 2019 16:55:49 +0100 | From | Liviu Dudau <> | Subject | Re: [PATCH v2 2/2] drm/malidp: Enable writeback scaling |
| |
Hi Ben,
On Fri, Apr 12, 2019 at 03:08:32PM +0100, Ben Davis wrote: > The phase setting part of malidp_crtc_atomic_check_scaling is refactored > to allow use in writeback scaling. > > Also the enable_memwrite function prototype is simplified by directly > passing mw_state. > > Signed-off-by: Ben Davis <ben.davis@arm.com> > --- > drivers/gpu/drm/arm/malidp_crtc.c | 49 ++++++++++++---------- > drivers/gpu/drm/arm/malidp_crtc.h | 12 ++++++ > drivers/gpu/drm/arm/malidp_drv.c | 10 +++-- > drivers/gpu/drm/arm/malidp_hw.c | 45 ++++++++++++++------ > drivers/gpu/drm/arm/malidp_hw.h | 19 +++++++-- > drivers/gpu/drm/arm/malidp_mw.c | 86 ++++++++++++++++++++++++++++++--------- > drivers/gpu/drm/arm/malidp_regs.h | 1 + > 7 files changed, 161 insertions(+), 61 deletions(-) > create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c > index 3f65996..aa66457 100644 > --- a/drivers/gpu/drm/arm/malidp_crtc.c > +++ b/drivers/gpu/drm/arm/malidp_crtc.c > @@ -265,6 +265,31 @@ static int malidp_crtc_atomic_check_ctm(struct drm_crtc *crtc, > return 0; > } > > +void malidp_set_se_config_phase(struct malidp_se_config *s) > +{ > +#define SE_N_PHASE 4 > +#define SE_SHIFT_N_PHASE 12 > + u32 phase; > + > + /* Calculate initial_phase and delta_phase for horizontal. */ > + phase = s->input_w; > + s->h_init_phase = ((phase << SE_N_PHASE) / s->output_w + 1) / 2; > + > + phase = s->input_w; > + phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE); > + s->h_delta_phase = phase / s->output_w; > + > + /* Same for vertical. */ > + phase = s->input_h; > + s->v_init_phase = ((phase << SE_N_PHASE) / s->output_h + 1) / 2; > + > + phase = s->input_h; > + phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE); > + s->v_delta_phase = phase / s->output_h; > +#undef SE_N_PHASE > +#undef SE_SHIFT_N_PHASE > +} > + > static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > @@ -291,7 +316,6 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc, > > drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) { > struct malidp_plane *mp = to_malidp_plane(plane); > - u32 phase; > > if (!(mp->layer->id & scaling)) > continue; > @@ -319,27 +343,8 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc, > s->output_w = pstate->crtc_w; > s->output_h = pstate->crtc_h; > > -#define SE_N_PHASE 4 > -#define SE_SHIFT_N_PHASE 12 > - /* Calculate initial_phase and delta_phase for horizontal. */ > - phase = s->input_w; > - s->h_init_phase = > - ((phase << SE_N_PHASE) / s->output_w + 1) / 2; > - > - phase = s->input_w; > - phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE); > - s->h_delta_phase = phase / s->output_w; > - > - /* Same for vertical. */ > - phase = s->input_h; > - s->v_init_phase = > - ((phase << SE_N_PHASE) / s->output_h + 1) / 2; > - > - phase = s->input_h; > - phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE); > - s->v_delta_phase = phase / s->output_h; > -#undef SE_N_PHASE > -#undef SE_SHIFT_N_PHASE > + > + malidp_set_se_config_phase(s); > s->plane_src_id = mp->layer->id; > } > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.h b/drivers/gpu/drm/arm/malidp_crtc.h > new file mode 100644 > index 0000000..0f056f8 > --- /dev/null > +++ b/drivers/gpu/drm/arm/malidp_crtc.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * (C) COPYRIGHT 2019 ARM Limited. All rights reserved. > + * > + */ > + > +#include "malidp_hw.h" > + > +#ifndef __MALIDP_CRTC_H__ > +#define __MALIDP_CRTC_H__ > +void malidp_set_se_config_phase(struct malidp_se_config *s); > +#endif > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > index d37ff9d..e2465e9 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -138,7 +138,7 @@ static void malidp_atomic_commit_se_config(struct drm_crtc *crtc, > u32 val; > > /* Set SE_CONTROL */ > - if (!s->scale_enable) { > + if (!s->scale_enable && !s->wb_scale_enable) { > val = malidp_hw_read(hwdev, se_control); > val &= ~MALIDP_SE_SCALING_EN; > malidp_hw_write(hwdev, val, se_control); > @@ -147,12 +147,16 @@ static void malidp_atomic_commit_se_config(struct drm_crtc *crtc, > > hwdev->hw->se_set_scaling_coeffs(hwdev, s, old_s); > val = malidp_hw_read(hwdev, se_control); > - val |= MALIDP_SE_SCALING_EN | MALIDP_SE_ALPHA_EN; > + val |= MALIDP_SE_SCALING_EN; > > val &= ~MALIDP_SE_ENH(MALIDP_SE_ENH_MASK); > val |= s->enhancer_enable ? MALIDP_SE_ENH(3) : 0; > > - val |= MALIDP_SE_RGBO_IF_EN; > + if (s->scale_enable) > + val |= (MALIDP_SE_RGBO_IF_EN | MALIDP_SE_ALPHA_EN); > + else > + val &= ~((MALIDP_SE_RGBO_IF_EN | MALIDP_SE_ALPHA_EN)); > + > malidp_hw_write(hwdev, val, se_control); > > /* Set IN_SIZE & OUT_SIZE. */ > diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c > index 1a36718..6f59c0e 100644 > --- a/drivers/gpu/drm/arm/malidp_hw.c > +++ b/drivers/gpu/drm/arm/malidp_hw.c > @@ -497,13 +497,18 @@ static long malidp500_se_calc_mclk(struct malidp_hw_device *hwdev, > return ret; > } > > -static int malidp500_enable_memwrite(struct malidp_hw_device *hwdev, > - dma_addr_t *addrs, s32 *pitches, > - int num_planes, u16 w, u16 h, u32 fmt_id, > - const s16 *rgb2yuv_coeffs) > +static int malidp500_enable_memwrite(struct malidp_hw_device *hwdev, u16 w, u16 h, > + struct malidp_mw_connector_state *mw_state) > { > + > + int num_planes = mw_state->n_planes; > + dma_addr_t *addrs = mw_state->addrs; > + s32 *pitches = mw_state->pitches; > + u32 fmt_id = mw_state->format; > u32 base = MALIDP500_SE_MEMWRITE_BASE; > u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK); > + const s16 *rgb2yuv_coeffs = !mw_state->rgb2yuv_initialized ? > + mw_state->rgb2yuv_coeffs : NULL; > > /* enable the scaling engine block */ > malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC); > @@ -847,13 +852,18 @@ static long malidp550_se_calc_mclk(struct malidp_hw_device *hwdev, > return ret; > } > > -static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev, > - dma_addr_t *addrs, s32 *pitches, > - int num_planes, u16 w, u16 h, u32 fmt_id, > - const s16 *rgb2yuv_coeffs) > +static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev, u16 w, u16 h, > + struct malidp_mw_connector_state *mw_state) > { > + int num_planes = mw_state->n_planes; > + bool scaling = mw_state->wb_scale_enable; > + dma_addr_t *addrs = mw_state->addrs; > + s32 *pitches = mw_state->pitches; > + u32 fmt_id = mw_state->format; > u32 base = MALIDP550_SE_MEMWRITE_BASE; > u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK); > + const s16 *rgb2yuv_coeffs = !mw_state->rgb2yuv_initialized ? > + mw_state->rgb2yuv_coeffs : NULL; > > /* enable the scaling engine block */ > malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC); > @@ -876,10 +886,18 @@ static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev, > WARN(1, "Invalid number of planes"); > } > > - malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h), > - MALIDP550_SE_MEMWRITE_OUT_SIZE); > - malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN, > - MALIDP550_SE_CONTROL); > + malidp_hw_clearbits(hwdev, MALIDP_SE_MEMWRITE_EN | MALIDP_SE_MEMWRITE_SCL_EN, > + MALIDP550_SE_CONTROL); > + > + if (scaling) { > + malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_SCL_EN, > + MALIDP550_SE_CONTROL); > + } else { > + malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h), > + MALIDP550_SE_MEMWRITE_OUT_SIZE); > + malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN, > + MALIDP550_SE_CONTROL); > + } > > if (rgb2yuv_coeffs) { > int i; > @@ -897,7 +915,8 @@ static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev) > { > u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK); > > - malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN, > + malidp_hw_clearbits(hwdev, > + MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN | MALIDP_SE_MEMWRITE_SCL_EN, > MALIDP550_SE_CONTROL); > malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC); > } > diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h > index 8352a2e..61ddcb5 100644 > --- a/drivers/gpu/drm/arm/malidp_hw.h > +++ b/drivers/gpu/drm/arm/malidp_hw.h > @@ -85,6 +85,7 @@ enum malidp_scaling_coeff_set { > struct malidp_se_config { > u8 scale_enable : 1; > u8 enhancer_enable : 1; > + u8 wb_scale_enable : 1; > u8 hcoeff : 3; > u8 vcoeff : 3; > u8 plane_src_id; > @@ -94,6 +95,19 @@ struct malidp_se_config { > u32 v_init_phase, v_delta_phase; > }; > > +#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state) > + > +struct malidp_mw_connector_state { > + struct drm_connector_state base; > + dma_addr_t addrs[2]; > + s32 pitches[2]; > + u8 format; > + u8 n_planes; > + bool rgb2yuv_initialized; > + const s16 *rgb2yuv_coeffs; > + bool wb_scale_enable; > +}; > + > /* regmap features */ > #define MALIDP_REGMAP_HAS_CLEARIRQ BIT(0) > #define MALIDP_DEVICE_AFBC_SUPPORT_SPLIT BIT(1) > @@ -206,9 +220,8 @@ struct malidp_hw { > * @param h - height of the output frame > * @param fmt_id - internal format ID of output buffer > */ > - int (*enable_memwrite)(struct malidp_hw_device *hwdev, dma_addr_t *addrs, > - s32 *pitches, int num_planes, u16 w, u16 h, u32 fmt_id, > - const s16 *rgb2yuv_coeffs); > + int (*enable_memwrite)(struct malidp_hw_device *hwdev, u16 w, u16 h, > + struct malidp_mw_connector_state *mw_state); > > /* > * Disable the writing to memory of the next frame's content. > diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c > index 2865f7a..2120ef1 100644 > --- a/drivers/gpu/drm/arm/malidp_mw.c > +++ b/drivers/gpu/drm/arm/malidp_mw.c > @@ -13,23 +13,13 @@ > #include <drm/drm_gem_cma_helper.h> > #include <drm/drmP.h> > #include <drm/drm_writeback.h> > +#include <video/videomode.h> > > +#include "malidp_crtc.h" > #include "malidp_drv.h" > #include "malidp_hw.h" > #include "malidp_mw.h" > > -#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state) > - > -struct malidp_mw_connector_state { > - struct drm_connector_state base; > - dma_addr_t addrs[2]; > - s32 pitches[2]; > - u8 format; > - u8 n_planes; > - bool rgb2yuv_initialized; > - const s16 *rgb2yuv_coeffs; > -}; > - > static int malidp_mw_connector_get_modes(struct drm_connector *connector) > { > struct drm_device *dev = connector->dev; > @@ -126,16 +116,76 @@ malidp_mw_encoder_atomic_check(struct drm_encoder *encoder, > struct drm_connector_state *conn_state) > { > struct malidp_mw_connector_state *mw_state = to_mw_state(conn_state); > + struct malidp_crtc_state *cs = to_malidp_crtc_state(crtc_state); > + struct malidp_se_config *s = &cs->scaler_config; > struct malidp_drm *malidp = encoder->dev->dev_private; > struct drm_framebuffer *fb; > - int i, n_planes; > + int i, n_planes, ret; > + struct malidp_hw_device *hwdev = malidp->dev; > + struct videomode vm; > + u32 h_upscale_factor = 0; /* U16.16 */ > + u32 v_upscale_factor = 0; /* U16.16 */ > + > + s->wb_scale_enable = 0; > > if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > return 0; > > + > + if ((conn_state->writeback_w != crtc_state->mode.hdisplay) || > + (conn_state->writeback_h != crtc_state->mode.vdisplay)) {
Now that the default (zero) for writeback_w and writeback_h means "not set by userspace, don't scale" I think you need to revisit this check. Otherwise the writeback scaling gets enabled all the time.
Best regards, Liviu
> + s->wb_scale_enable = 1; > + mw_state->wb_scale_enable = true; > + } > + > fb = conn_state->writeback_job->fb; > - if ((fb->width != crtc_state->mode.hdisplay) || > - (fb->height != crtc_state->mode.vdisplay)) { > + if (s->wb_scale_enable) { > + if (s->scale_enable) { > + DRM_DEBUG_KMS("Attempting to scale on writeback while scaling a layer\n"); > + return -EINVAL; > + } > + > + s->input_w = crtc_state->mode.hdisplay; > + s->input_h = crtc_state->mode.vdisplay; > + s->output_w = conn_state->writeback_w; > + s->output_h = conn_state->writeback_h; > + > + if (fb->width < s->output_w || fb->height < s->output_h) { > + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", > + fb->width, fb->height); > + return -EINVAL; > + } > + > + if ((s->output_w > s->input_w) || (s->output_h > s->input_h)) { > + DRM_DEBUG_KMS("Upscaling on writeback is not supported\n"); > + return -EINVAL; > + } > + > + /* > + * Convert crtc_[w|h] to U32.32, then divide by U16.16 src_[w|h] > + * to get the U16.16 result. > + */ > + h_upscale_factor = div_u64((u64)s->output_w << 32, > + (u32)s->input_w << 16); > + v_upscale_factor = div_u64((u64)s->output_h << 32, > + (u32)s->input_h << 16); > + > + /* enhancer is for upscaling */ > + s->enhancer_enable = 0; > + > + malidp_set_se_config_phase(s); > + > + s->hcoeff = malidp_se_select_coeffs(h_upscale_factor); > + s->vcoeff = malidp_se_select_coeffs(v_upscale_factor); > + > + drm_display_mode_to_videomode(&crtc_state->adjusted_mode, &vm); > + ret = hwdev->hw->se_calc_mclk(hwdev, s, &vm); > + if (ret < 0) > + return -EINVAL; > + } > + > + if (!s->wb_scale_enable && ((fb->width != crtc_state->mode.hdisplay) || > + (fb->height != crtc_state->mode.vdisplay))) { > DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", > fb->width, fb->height); > return -EINVAL; > @@ -259,11 +309,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm, > > drm_writeback_queue_job(mw_conn, conn_state->writeback_job); > conn_state->writeback_job = NULL; > - hwdev->hw->enable_memwrite(hwdev, mw_state->addrs, > - mw_state->pitches, mw_state->n_planes, > - fb->width, fb->height, mw_state->format, > - !mw_state->rgb2yuv_initialized ? > - mw_state->rgb2yuv_coeffs : NULL); > + hwdev->hw->enable_memwrite(hwdev, fb->width, fb->height, mw_state); > mw_state->rgb2yuv_initialized = !!mw_state->rgb2yuv_coeffs; > } else { > DRM_DEV_DEBUG_DRIVER(drm->dev, "Disable memwrite\n"); > diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h > index a3363f3..254aa98 100644 > --- a/drivers/gpu/drm/arm/malidp_regs.h > +++ b/drivers/gpu/drm/arm/malidp_regs.h > @@ -73,6 +73,7 @@ > #define MALIDP_DISP_FUNC_CADJ (1 << 4) > #define MALIDP_DISP_FUNC_ILACED (1 << 8) > #define MALIDP_SCALE_ENGINE_EN (1 << 16) > +#define MALIDP_SE_MEMWRITE_SCL_EN (1 << 5) > #define MALIDP_SE_MEMWRITE_EN (2 << 5) > > /* register offsets for IRQ management */ > -- > 2.7.4 >
-- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯
| |