| Message ID | 1474889789-26721-1-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: > Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once > in mixer_cfg_layer(). > Trigger this via atomic flush. > > Changes in v2: > - issue mixer_cfg_layer() in mixer_disable() > - rename fields as suggested by Andrzej > - added docu to mixer context struct > - simplify mixer_win_reset() as well > > Changes in v3: > - simplify some conditions as suggested by Inki > - add docu to mixer_cfg_layer() > - fold switch statements into a single one > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ > drivers/gpu/drm/exynos/regs-mixer.h | 2 + > 2 files changed, 92 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 1e78d57..4f06f4d 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { > DRM_FORMAT_NV21, > }; > > +/* > + * Mixer context structure. > + * > + * @crtc: The HDMI CRTC attached to the mixer. > + * @planes: Array of plane objects for each of the mixer windows. > + * @active_windows: Cache of the mixer's hardware state. > + * Tracks which mixer windows are active/inactive. > + * @pipe: The CRTC index. > + * @flags: Bitfield build from the mixer_flag_bits enumerator. > + * @mixer_resources: A struct containing registers, clocks, etc. > + * @mxr_ver: The hardware revision/version of the mixer. > + */ > struct mixer_context { > struct platform_device *pdev; > struct device *dev; > struct drm_device *drm_dev; > struct exynos_drm_crtc *crtc; > struct exynos_drm_plane planes[MIXER_WIN_NR]; > + unsigned long active_windows; > int pipe; > unsigned long flags; > > @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); > } > > -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, > - unsigned int priority, bool enable) > +/** > + * mixer_cfg_layer - apply layer configuration to hardware > + * @ctx: mixer context > + * > + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers > + * using the 'active_windows' field of the the mixer content, and > + * the pixel format of the framebuffers associated with the enabled > + * windows. > + * > + * Has to be called under mixer lock. > + */ > +static void mixer_cfg_layer(struct mixer_context *ctx) > { > struct mixer_resources *res = &ctx->mixer_res; > - u32 val = enable ? ~0 : 0; > - > - switch (win) { > - case 0: > - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_GRP0_VAL(priority), > - MXR_LAYER_CFG_GRP0_MASK); > - break; > - case 1: > - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_GRP1_VAL(priority), > - MXR_LAYER_CFG_GRP1_MASK); > + unsigned int win; > > - break; > - case VP_DEFAULT_WIN: > - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { > - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); > - mixer_reg_writemask(res, MXR_CFG, val, > - MXR_CFG_VP_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_VP_VAL(priority), > - MXR_LAYER_CFG_VP_MASK); > + struct exynos_drm_plane_state *state; > + struct drm_framebuffer *fb; > + unsigned int priority; > + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; > + bool enable; > + > + for (win = 0; win < MIXER_WIN_NR; ++win) { > + state = to_exynos_plane_state(ctx->planes[win].base.state); > + fb = state->fb; > + > + priority = state->base.normalized_zpos + 1; > + enable = test_bit(win, &ctx->active_windows); > + > + if (!enable) > + continue; > + > + BUG_ON(!fb); Not good. Even through some problem happens at mixer driver, other KMS drivers should work.
Inki Dae wrote: > > > 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >> in mixer_cfg_layer(). >> Trigger this via atomic flush. >> >> Changes in v2: >> - issue mixer_cfg_layer() in mixer_disable() >> - rename fields as suggested by Andrzej >> - added docu to mixer context struct >> - simplify mixer_win_reset() as well >> >> Changes in v3: >> - simplify some conditions as suggested by Inki >> - add docu to mixer_cfg_layer() >> - fold switch statements into a single one >> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> --- >> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >> 2 files changed, 92 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >> index 1e78d57..4f06f4d 100644 >> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >> DRM_FORMAT_NV21, >> }; >> >> +/* >> + * Mixer context structure. >> + * >> + * @crtc: The HDMI CRTC attached to the mixer. >> + * @planes: Array of plane objects for each of the mixer windows. >> + * @active_windows: Cache of the mixer's hardware state. >> + * Tracks which mixer windows are active/inactive. >> + * @pipe: The CRTC index. >> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >> + * @mixer_resources: A struct containing registers, clocks, etc. >> + * @mxr_ver: The hardware revision/version of the mixer. >> + */ >> struct mixer_context { >> struct platform_device *pdev; >> struct device *dev; >> struct drm_device *drm_dev; >> struct exynos_drm_crtc *crtc; >> struct exynos_drm_plane planes[MIXER_WIN_NR]; >> + unsigned long active_windows; >> int pipe; >> unsigned long flags; >> >> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >> } >> >> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >> - unsigned int priority, bool enable) >> +/** >> + * mixer_cfg_layer - apply layer configuration to hardware >> + * @ctx: mixer context >> + * >> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >> + * using the 'active_windows' field of the the mixer content, and >> + * the pixel format of the framebuffers associated with the enabled >> + * windows. >> + * >> + * Has to be called under mixer lock. >> + */ >> +static void mixer_cfg_layer(struct mixer_context *ctx) >> { >> struct mixer_resources *res = &ctx->mixer_res; >> - u32 val = enable ? ~0 : 0; >> - >> - switch (win) { >> - case 0: >> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >> - mixer_reg_writemask(res, MXR_LAYER_CFG, >> - MXR_LAYER_CFG_GRP0_VAL(priority), >> - MXR_LAYER_CFG_GRP0_MASK); >> - break; >> - case 1: >> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >> - mixer_reg_writemask(res, MXR_LAYER_CFG, >> - MXR_LAYER_CFG_GRP1_VAL(priority), >> - MXR_LAYER_CFG_GRP1_MASK); >> + unsigned int win; >> >> - break; >> - case VP_DEFAULT_WIN: >> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >> - mixer_reg_writemask(res, MXR_CFG, val, >> - MXR_CFG_VP_ENABLE); >> - mixer_reg_writemask(res, MXR_LAYER_CFG, >> - MXR_LAYER_CFG_VP_VAL(priority), >> - MXR_LAYER_CFG_VP_MASK); >> + struct exynos_drm_plane_state *state; >> + struct drm_framebuffer *fb; >> + unsigned int priority; >> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >> + bool enable; >> + >> + for (win = 0; win < MIXER_WIN_NR; ++win) { >> + state = to_exynos_plane_state(ctx->planes[win].base.state); >> + fb = state->fb; >> + >> + priority = state->base.normalized_zpos + 1; >> + enable = test_bit(win, &ctx->active_windows); >> + >> + if (!enable) >> + continue; >> + >> + BUG_ON(!fb); > > Not good. Even through some problem happens at mixer driver, other KMS drivers should work. That would be a issue at core level. And in this case, BUG_ON() should be triggered. - Tobias >
2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: > Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once > in mixer_cfg_layer(). > Trigger this via atomic flush. > > Changes in v2: > - issue mixer_cfg_layer() in mixer_disable() > - rename fields as suggested by Andrzej > - added docu to mixer context struct > - simplify mixer_win_reset() as well > > Changes in v3: > - simplify some conditions as suggested by Inki > - add docu to mixer_cfg_layer() > - fold switch statements into a single one > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ > drivers/gpu/drm/exynos/regs-mixer.h | 2 + > 2 files changed, 92 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 1e78d57..4f06f4d 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { > DRM_FORMAT_NV21, > }; > > +/* > + * Mixer context structure. > + * > + * @crtc: The HDMI CRTC attached to the mixer. > + * @planes: Array of plane objects for each of the mixer windows. > + * @active_windows: Cache of the mixer's hardware state. > + * Tracks which mixer windows are active/inactive. > + * @pipe: The CRTC index. > + * @flags: Bitfield build from the mixer_flag_bits enumerator. > + * @mixer_resources: A struct containing registers, clocks, etc. > + * @mxr_ver: The hardware revision/version of the mixer. > + */ > struct mixer_context { > struct platform_device *pdev; > struct device *dev; > struct drm_device *drm_dev; > struct exynos_drm_crtc *crtc; > struct exynos_drm_plane planes[MIXER_WIN_NR]; > + unsigned long active_windows; > int pipe; > unsigned long flags; > > @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); > } > > -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, > - unsigned int priority, bool enable) > +/** > + * mixer_cfg_layer - apply layer configuration to hardware > + * @ctx: mixer context > + * > + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers > + * using the 'active_windows' field of the the mixer content, and > + * the pixel format of the framebuffers associated with the enabled > + * windows. > + * > + * Has to be called under mixer lock. > + */ > +static void mixer_cfg_layer(struct mixer_context *ctx) > { > struct mixer_resources *res = &ctx->mixer_res; > - u32 val = enable ? ~0 : 0; > - > - switch (win) { > - case 0: > - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_GRP0_VAL(priority), > - MXR_LAYER_CFG_GRP0_MASK); > - break; > - case 1: > - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_GRP1_VAL(priority), > - MXR_LAYER_CFG_GRP1_MASK); > + unsigned int win; > > - break; > - case VP_DEFAULT_WIN: > - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { > - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); > - mixer_reg_writemask(res, MXR_CFG, val, > - MXR_CFG_VP_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_VP_VAL(priority), > - MXR_LAYER_CFG_VP_MASK); > + struct exynos_drm_plane_state *state; > + struct drm_framebuffer *fb; > + unsigned int priority; > + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; > + bool enable; > + > + for (win = 0; win < MIXER_WIN_NR; ++win) { > + state = to_exynos_plane_state(ctx->planes[win].base.state); > + fb = state->fb; > + > + priority = state->base.normalized_zpos + 1; > + enable = test_bit(win, &ctx->active_windows); > + > + if (!enable) > + continue; > + > + BUG_ON(!fb); > + > + /* > + * TODO: Don't enable alpha blending for the bottom window. > + */ > + switch (win) { > + case 0: > + mxr_cfg |= MXR_CFG_GRP0_ENABLE; > + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); > + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); > + break; > + > + case 1: > + mxr_cfg |= MXR_CFG_GRP1_ENABLE; > + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); > + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); > + break; > + > + case VP_DEFAULT_WIN: > + vp_enable = VP_ENABLE_ON; > + mxr_cfg |= MXR_CFG_VP_ENABLE; > + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); > + mixer_cfg_vp_blend(ctx); > + break; > } > - break; > } > + > + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) > + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); > + > + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); > + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); > } > > static void mixer_run(struct mixer_context *ctx) > @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, > struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; > struct mixer_resources *res = &ctx->mixer_res; > struct drm_framebuffer *fb = state->fb; > - unsigned int priority = state->base.normalized_zpos + 1; > unsigned long flags; > dma_addr_t luma_addr[2], chroma_addr[2]; > bool tiled_mode = false; > @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, > > mixer_cfg_scan(ctx, mode->vdisplay); > mixer_cfg_rgb_fmt(ctx, mode->vdisplay); > - mixer_cfg_layer(ctx, plane->index, priority, true); > - mixer_cfg_vp_blend(ctx); You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? > mixer_run(ctx); > > spin_unlock_irqrestore(&res->reg_slock, flags); > @@ -588,7 +631,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; > struct mixer_resources *res = &ctx->mixer_res; > struct drm_framebuffer *fb = state->fb; > - unsigned int priority = state->base.normalized_zpos + 1; > unsigned long flags; > unsigned int win = plane->index; > unsigned int x_ratio = 0, y_ratio = 0; > @@ -680,8 +722,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > > mixer_cfg_scan(ctx, mode->vdisplay); > mixer_cfg_rgb_fmt(ctx, mode->vdisplay); > - mixer_cfg_layer(ctx, win, priority, true); > - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); ditto. > > /* layer update mandatory for mixer 16.0.33.0 */ > if (ctx->mxr_ver == MXR_VER_16_0_33_0 || > @@ -726,9 +766,6 @@ static void mixer_win_reset(struct mixer_context *ctx) > mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST, > MXR_STATUS_BURST_MASK); > > - /* reset default layer priority */ > - mixer_reg_write(res, MXR_LAYER_CFG, 0); > - > /* setting background color */ > mixer_reg_write(res, MXR_BG_COLOR0, 0x008080); > mixer_reg_write(res, MXR_BG_COLOR1, 0x008080); > @@ -740,11 +777,9 @@ static void mixer_win_reset(struct mixer_context *ctx) > vp_default_filter(res); > } > > - /* disable all layers */ > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); > - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); > + /* disable all layers and reset layer priority to default */ > + ctx->active_windows = 0; > + mixer_cfg_layer(ctx); > > spin_unlock_irqrestore(&res->reg_slock, flags); > } > @@ -994,32 +1029,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, > vp_video_buffer(mixer_ctx, plane); > else > mixer_graph_buffer(mixer_ctx, plane); > + > + __set_bit(plane->index, &mixer_ctx->active_windows); > } > > static void mixer_disable_plane(struct exynos_drm_crtc *crtc, > struct exynos_drm_plane *plane) > { > struct mixer_context *mixer_ctx = crtc->ctx; > - struct mixer_resources *res = &mixer_ctx->mixer_res; > - unsigned long flags; > > DRM_DEBUG_KMS("win: %d\n", plane->index); > > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > return; > > - spin_lock_irqsave(&res->reg_slock, flags); > - mixer_cfg_layer(mixer_ctx, plane->index, 0, false); > - spin_unlock_irqrestore(&res->reg_slock, flags); > + __clear_bit(plane->index, &mixer_ctx->active_windows); > } > > static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) > { > struct mixer_context *mixer_ctx = crtc->ctx; > + struct mixer_resources *res = &mixer_ctx->mixer_res; > + unsigned long flags; > > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > return; > > + spin_lock_irqsave(&res->reg_slock, flags); > + mixer_cfg_layer(mixer_ctx); atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable.
2016년 09월 27일 14:40에 Tobias Jakobi 이(가) 쓴 글: > Inki Dae wrote: >> >> >> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>> in mixer_cfg_layer(). >>> Trigger this via atomic flush. >>> >>> Changes in v2: >>> - issue mixer_cfg_layer() in mixer_disable() >>> - rename fields as suggested by Andrzej >>> - added docu to mixer context struct >>> - simplify mixer_win_reset() as well >>> >>> Changes in v3: >>> - simplify some conditions as suggested by Inki >>> - add docu to mixer_cfg_layer() >>> - fold switch statements into a single one >>> >>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>> --- >>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>> 2 files changed, 92 insertions(+), 45 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>> index 1e78d57..4f06f4d 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>> DRM_FORMAT_NV21, >>> }; >>> >>> +/* >>> + * Mixer context structure. >>> + * >>> + * @crtc: The HDMI CRTC attached to the mixer. >>> + * @planes: Array of plane objects for each of the mixer windows. >>> + * @active_windows: Cache of the mixer's hardware state. >>> + * Tracks which mixer windows are active/inactive. >>> + * @pipe: The CRTC index. >>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>> + * @mixer_resources: A struct containing registers, clocks, etc. >>> + * @mxr_ver: The hardware revision/version of the mixer. >>> + */ >>> struct mixer_context { >>> struct platform_device *pdev; >>> struct device *dev; >>> struct drm_device *drm_dev; >>> struct exynos_drm_crtc *crtc; >>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>> + unsigned long active_windows; >>> int pipe; >>> unsigned long flags; >>> >>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>> } >>> >>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>> - unsigned int priority, bool enable) >>> +/** >>> + * mixer_cfg_layer - apply layer configuration to hardware >>> + * @ctx: mixer context >>> + * >>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>> + * using the 'active_windows' field of the the mixer content, and >>> + * the pixel format of the framebuffers associated with the enabled >>> + * windows. >>> + * >>> + * Has to be called under mixer lock. >>> + */ >>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>> { >>> struct mixer_resources *res = &ctx->mixer_res; >>> - u32 val = enable ? ~0 : 0; >>> - >>> - switch (win) { >>> - case 0: >>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>> - MXR_LAYER_CFG_GRP0_MASK); >>> - break; >>> - case 1: >>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>> - MXR_LAYER_CFG_GRP1_MASK); >>> + unsigned int win; >>> >>> - break; >>> - case VP_DEFAULT_WIN: >>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>> - mixer_reg_writemask(res, MXR_CFG, val, >>> - MXR_CFG_VP_ENABLE); >>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>> - MXR_LAYER_CFG_VP_VAL(priority), >>> - MXR_LAYER_CFG_VP_MASK); >>> + struct exynos_drm_plane_state *state; >>> + struct drm_framebuffer *fb; >>> + unsigned int priority; >>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>> + bool enable; >>> + >>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>> + fb = state->fb; >>> + >>> + priority = state->base.normalized_zpos + 1; >>> + enable = test_bit(win, &ctx->active_windows); >>> + >>> + if (!enable) >>> + continue; >>> + >>> + BUG_ON(!fb); >> >> Not good. Even through some problem happens at mixer driver, other KMS drivers should work. > That would be a issue at core level. And in this case, BUG_ON() should > be triggered. If so, the fb should be checked at core level not specific driver. > > - Tobias > > >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
Hey Inki, Inki Dae wrote: > 2016년 09월 27일 14:40에 Tobias Jakobi 이(가) 쓴 글: >> Inki Dae wrote: >>> >>> >>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>> in mixer_cfg_layer(). >>>> Trigger this via atomic flush. >>>> >>>> Changes in v2: >>>> - issue mixer_cfg_layer() in mixer_disable() >>>> - rename fields as suggested by Andrzej >>>> - added docu to mixer context struct >>>> - simplify mixer_win_reset() as well >>>> >>>> Changes in v3: >>>> - simplify some conditions as suggested by Inki >>>> - add docu to mixer_cfg_layer() >>>> - fold switch statements into a single one >>>> >>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>> index 1e78d57..4f06f4d 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>> DRM_FORMAT_NV21, >>>> }; >>>> >>>> +/* >>>> + * Mixer context structure. >>>> + * >>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>> + * @planes: Array of plane objects for each of the mixer windows. >>>> + * @active_windows: Cache of the mixer's hardware state. >>>> + * Tracks which mixer windows are active/inactive. >>>> + * @pipe: The CRTC index. >>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>> + */ >>>> struct mixer_context { >>>> struct platform_device *pdev; >>>> struct device *dev; >>>> struct drm_device *drm_dev; >>>> struct exynos_drm_crtc *crtc; >>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>> + unsigned long active_windows; >>>> int pipe; >>>> unsigned long flags; >>>> >>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>> } >>>> >>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>> - unsigned int priority, bool enable) >>>> +/** >>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>> + * @ctx: mixer context >>>> + * >>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>> + * using the 'active_windows' field of the the mixer content, and >>>> + * the pixel format of the framebuffers associated with the enabled >>>> + * windows. >>>> + * >>>> + * Has to be called under mixer lock. >>>> + */ >>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>> { >>>> struct mixer_resources *res = &ctx->mixer_res; >>>> - u32 val = enable ? ~0 : 0; >>>> - >>>> - switch (win) { >>>> - case 0: >>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>> - MXR_LAYER_CFG_GRP0_MASK); >>>> - break; >>>> - case 1: >>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>> - MXR_LAYER_CFG_GRP1_MASK); >>>> + unsigned int win; >>>> >>>> - break; >>>> - case VP_DEFAULT_WIN: >>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>> - MXR_CFG_VP_ENABLE); >>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>> - MXR_LAYER_CFG_VP_MASK); >>>> + struct exynos_drm_plane_state *state; >>>> + struct drm_framebuffer *fb; >>>> + unsigned int priority; >>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>> + bool enable; >>>> + >>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>> + fb = state->fb; >>>> + >>>> + priority = state->base.normalized_zpos + 1; >>>> + enable = test_bit(win, &ctx->active_windows); >>>> + >>>> + if (!enable) >>>> + continue; >>>> + >>>> + BUG_ON(!fb); >>> >>> Not good. Even through some problem happens at mixer driver, other KMS drivers should work. >> That would be a issue at core level. And in this case, BUG_ON() should >> be triggered. > > If so, the fb should be checked at core level not specific driver. I'm not sure I can follow you here. So you actually agree on putting BUG_ON() here, or not? Or let me phrase it differently. We have three options here: (1) Put a BUG_ON(). If something goes terribly wrong in DRM core and fb is NULL at this point, then we get a nice descriptive Oops. (2) Put nothing here. Again, something goes terribly wrong, fb is NULL, etc. Then we also get an Oops here, since we dereference NULL. (3) Put 'if (fb) something();' here. Now we mask/ignore the issue in DRM core and continue as if nothing went wrong. Obviously (3) is the worst approach. We want to fix bugs, not hide them. Second best in my opinion is (2). The Oops is going to tell us that NULL was dereferenced, but depending on the build, it's not immediately clear where in the code that happens. Best is (1) in my opinion since the Oops is more descriptive and the code itself actually tells us that this shouldn't happen. With best wishes, Tobias > >> >> - Tobias >> >> >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >>
Hello Inki, Inki Dae wrote: > 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >> in mixer_cfg_layer(). >> Trigger this via atomic flush. >> >> Changes in v2: >> - issue mixer_cfg_layer() in mixer_disable() >> - rename fields as suggested by Andrzej >> - added docu to mixer context struct >> - simplify mixer_win_reset() as well >> >> Changes in v3: >> - simplify some conditions as suggested by Inki >> - add docu to mixer_cfg_layer() >> - fold switch statements into a single one >> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> --- >> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >> 2 files changed, 92 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >> index 1e78d57..4f06f4d 100644 >> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >> DRM_FORMAT_NV21, >> }; >> >> +/* >> + * Mixer context structure. >> + * >> + * @crtc: The HDMI CRTC attached to the mixer. >> + * @planes: Array of plane objects for each of the mixer windows. >> + * @active_windows: Cache of the mixer's hardware state. >> + * Tracks which mixer windows are active/inactive. >> + * @pipe: The CRTC index. >> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >> + * @mixer_resources: A struct containing registers, clocks, etc. >> + * @mxr_ver: The hardware revision/version of the mixer. >> + */ >> struct mixer_context { >> struct platform_device *pdev; >> struct device *dev; >> struct drm_device *drm_dev; >> struct exynos_drm_crtc *crtc; >> struct exynos_drm_plane planes[MIXER_WIN_NR]; >> + unsigned long active_windows; >> int pipe; >> unsigned long flags; >> >> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >> } >> >> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >> - unsigned int priority, bool enable) >> +/** >> + * mixer_cfg_layer - apply layer configuration to hardware >> + * @ctx: mixer context >> + * >> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >> + * using the 'active_windows' field of the the mixer content, and >> + * the pixel format of the framebuffers associated with the enabled >> + * windows. >> + * >> + * Has to be called under mixer lock. >> + */ >> +static void mixer_cfg_layer(struct mixer_context *ctx) >> { >> struct mixer_resources *res = &ctx->mixer_res; >> - u32 val = enable ? ~0 : 0; >> - >> - switch (win) { >> - case 0: >> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >> - mixer_reg_writemask(res, MXR_LAYER_CFG, >> - MXR_LAYER_CFG_GRP0_VAL(priority), >> - MXR_LAYER_CFG_GRP0_MASK); >> - break; >> - case 1: >> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >> - mixer_reg_writemask(res, MXR_LAYER_CFG, >> - MXR_LAYER_CFG_GRP1_VAL(priority), >> - MXR_LAYER_CFG_GRP1_MASK); >> + unsigned int win; >> >> - break; >> - case VP_DEFAULT_WIN: >> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >> - mixer_reg_writemask(res, MXR_CFG, val, >> - MXR_CFG_VP_ENABLE); >> - mixer_reg_writemask(res, MXR_LAYER_CFG, >> - MXR_LAYER_CFG_VP_VAL(priority), >> - MXR_LAYER_CFG_VP_MASK); >> + struct exynos_drm_plane_state *state; >> + struct drm_framebuffer *fb; >> + unsigned int priority; >> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >> + bool enable; >> + >> + for (win = 0; win < MIXER_WIN_NR; ++win) { >> + state = to_exynos_plane_state(ctx->planes[win].base.state); >> + fb = state->fb; >> + >> + priority = state->base.normalized_zpos + 1; >> + enable = test_bit(win, &ctx->active_windows); >> + >> + if (!enable) >> + continue; >> + >> + BUG_ON(!fb); >> + >> + /* >> + * TODO: Don't enable alpha blending for the bottom window. >> + */ >> + switch (win) { >> + case 0: >> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >> + break; >> + >> + case 1: >> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >> + break; >> + >> + case VP_DEFAULT_WIN: >> + vp_enable = VP_ENABLE_ON; >> + mxr_cfg |= MXR_CFG_VP_ENABLE; >> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >> + mixer_cfg_vp_blend(ctx); >> + break; >> } >> - break; >> } >> + >> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >> + >> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >> } >> >> static void mixer_run(struct mixer_context *ctx) >> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >> struct mixer_resources *res = &ctx->mixer_res; >> struct drm_framebuffer *fb = state->fb; >> - unsigned int priority = state->base.normalized_zpos + 1; >> unsigned long flags; >> dma_addr_t luma_addr[2], chroma_addr[2]; >> bool tiled_mode = false; >> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >> >> mixer_cfg_scan(ctx, mode->vdisplay); >> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >> - mixer_cfg_layer(ctx, plane->index, priority, true); >> - mixer_cfg_vp_blend(ctx); > > You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? yes, you're right. I completly forgot about the legacy codepaths. Sorry about that! >> mixer_run(ctx); >> >> spin_unlock_irqrestore(&res->reg_slock, flags); >> @@ -588,7 +631,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, >> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >> struct mixer_resources *res = &ctx->mixer_res; >> struct drm_framebuffer *fb = state->fb; >> - unsigned int priority = state->base.normalized_zpos + 1; >> unsigned long flags; >> unsigned int win = plane->index; >> unsigned int x_ratio = 0, y_ratio = 0; >> @@ -680,8 +722,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, >> >> mixer_cfg_scan(ctx, mode->vdisplay); >> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >> - mixer_cfg_layer(ctx, win, priority, true); >> - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); > > ditto. > >> >> /* layer update mandatory for mixer 16.0.33.0 */ >> if (ctx->mxr_ver == MXR_VER_16_0_33_0 || >> @@ -726,9 +766,6 @@ static void mixer_win_reset(struct mixer_context *ctx) >> mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST, >> MXR_STATUS_BURST_MASK); >> >> - /* reset default layer priority */ >> - mixer_reg_write(res, MXR_LAYER_CFG, 0); >> - >> /* setting background color */ >> mixer_reg_write(res, MXR_BG_COLOR0, 0x008080); >> mixer_reg_write(res, MXR_BG_COLOR1, 0x008080); >> @@ -740,11 +777,9 @@ static void mixer_win_reset(struct mixer_context *ctx) >> vp_default_filter(res); >> } >> >> - /* disable all layers */ >> - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); >> - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); >> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >> - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); >> + /* disable all layers and reset layer priority to default */ >> + ctx->active_windows = 0; >> + mixer_cfg_layer(ctx); >> >> spin_unlock_irqrestore(&res->reg_slock, flags); >> } >> @@ -994,32 +1029,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, >> vp_video_buffer(mixer_ctx, plane); >> else >> mixer_graph_buffer(mixer_ctx, plane); >> + >> + __set_bit(plane->index, &mixer_ctx->active_windows); >> } >> >> static void mixer_disable_plane(struct exynos_drm_crtc *crtc, >> struct exynos_drm_plane *plane) >> { >> struct mixer_context *mixer_ctx = crtc->ctx; >> - struct mixer_resources *res = &mixer_ctx->mixer_res; >> - unsigned long flags; >> >> DRM_DEBUG_KMS("win: %d\n", plane->index); >> >> if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) >> return; >> >> - spin_lock_irqsave(&res->reg_slock, flags); >> - mixer_cfg_layer(mixer_ctx, plane->index, 0, false); >> - spin_unlock_irqrestore(&res->reg_slock, flags); >> + __clear_bit(plane->index, &mixer_ctx->active_windows); >> } >> >> static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) >> { >> struct mixer_context *mixer_ctx = crtc->ctx; >> + struct mixer_resources *res = &mixer_ctx->mixer_res; >> + unsigned long flags; >> >> if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) >> return; >> >> + spin_lock_irqsave(&res->reg_slock, flags); >> + mixer_cfg_layer(mixer_ctx); > > atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable. Please the see discussion with Andrzej for reference. The idea of the patch was to move register manipulation to atomic_flush(). For atomic we have: (1) atomic_begin() (2) atomic_{disable,update}_plane(), probably multiple calls (3) atomic_flush() Hence the idea to move (most) manipulation of MXR_CFG and MXR_LAYER_CFG to mixer_atomic_flush(). Of course this doesn't work with the legacy codepaths. I need to think about this some more. For now, please drop the patch. With best wishes, Tobias
On 27.09.2016 13:22, Tobias Jakobi wrote: > Hello Inki, > > > Inki Dae wrote: >> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>> in mixer_cfg_layer(). >>> Trigger this via atomic flush. >>> >>> Changes in v2: >>> - issue mixer_cfg_layer() in mixer_disable() >>> - rename fields as suggested by Andrzej >>> - added docu to mixer context struct >>> - simplify mixer_win_reset() as well >>> >>> Changes in v3: >>> - simplify some conditions as suggested by Inki >>> - add docu to mixer_cfg_layer() >>> - fold switch statements into a single one >>> >>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>> --- >>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>> 2 files changed, 92 insertions(+), 45 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>> index 1e78d57..4f06f4d 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>> DRM_FORMAT_NV21, >>> }; >>> >>> +/* >>> + * Mixer context structure. >>> + * >>> + * @crtc: The HDMI CRTC attached to the mixer. >>> + * @planes: Array of plane objects for each of the mixer windows. >>> + * @active_windows: Cache of the mixer's hardware state. >>> + * Tracks which mixer windows are active/inactive. >>> + * @pipe: The CRTC index. >>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>> + * @mixer_resources: A struct containing registers, clocks, etc. >>> + * @mxr_ver: The hardware revision/version of the mixer. >>> + */ >>> struct mixer_context { >>> struct platform_device *pdev; >>> struct device *dev; >>> struct drm_device *drm_dev; >>> struct exynos_drm_crtc *crtc; >>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>> + unsigned long active_windows; >>> int pipe; >>> unsigned long flags; >>> >>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>> } >>> >>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>> - unsigned int priority, bool enable) >>> +/** >>> + * mixer_cfg_layer - apply layer configuration to hardware >>> + * @ctx: mixer context >>> + * >>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>> + * using the 'active_windows' field of the the mixer content, and >>> + * the pixel format of the framebuffers associated with the enabled >>> + * windows. >>> + * >>> + * Has to be called under mixer lock. >>> + */ >>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>> { >>> struct mixer_resources *res = &ctx->mixer_res; >>> - u32 val = enable ? ~0 : 0; >>> - >>> - switch (win) { >>> - case 0: >>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>> - MXR_LAYER_CFG_GRP0_MASK); >>> - break; >>> - case 1: >>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>> - MXR_LAYER_CFG_GRP1_MASK); >>> + unsigned int win; >>> >>> - break; >>> - case VP_DEFAULT_WIN: >>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>> - mixer_reg_writemask(res, MXR_CFG, val, >>> - MXR_CFG_VP_ENABLE); >>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>> - MXR_LAYER_CFG_VP_VAL(priority), >>> - MXR_LAYER_CFG_VP_MASK); >>> + struct exynos_drm_plane_state *state; >>> + struct drm_framebuffer *fb; >>> + unsigned int priority; >>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>> + bool enable; >>> + >>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>> + fb = state->fb; >>> + >>> + priority = state->base.normalized_zpos + 1; >>> + enable = test_bit(win, &ctx->active_windows); >>> + >>> + if (!enable) >>> + continue; >>> + >>> + BUG_ON(!fb); >>> + >>> + /* >>> + * TODO: Don't enable alpha blending for the bottom window. >>> + */ >>> + switch (win) { >>> + case 0: >>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>> + break; >>> + >>> + case 1: >>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>> + break; >>> + >>> + case VP_DEFAULT_WIN: >>> + vp_enable = VP_ENABLE_ON; >>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>> + mixer_cfg_vp_blend(ctx); >>> + break; >>> } >>> - break; >>> } >>> + >>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>> + >>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>> } >>> >>> static void mixer_run(struct mixer_context *ctx) >>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>> struct mixer_resources *res = &ctx->mixer_res; >>> struct drm_framebuffer *fb = state->fb; >>> - unsigned int priority = state->base.normalized_zpos + 1; >>> unsigned long flags; >>> dma_addr_t luma_addr[2], chroma_addr[2]; >>> bool tiled_mode = false; >>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>> >>> mixer_cfg_scan(ctx, mode->vdisplay); >>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>> - mixer_cfg_vp_blend(ctx); >> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? > yes, you're right. I completly forgot about the legacy codepaths. Sorry > about that! Are you talking about DRM_IOCTL_MODE_SETPLANE? It does not seem to be legacy, or to be more precise it calls .update_plane and .disable_plane callbacks which in exynos are set as follow: .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. Regards Andrzej
Hello Andrzej, Andrzej Hajda wrote: > On 27.09.2016 13:22, Tobias Jakobi wrote: >> Hello Inki, >> >> >> Inki Dae wrote: >>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>> in mixer_cfg_layer(). >>>> Trigger this via atomic flush. >>>> >>>> Changes in v2: >>>> - issue mixer_cfg_layer() in mixer_disable() >>>> - rename fields as suggested by Andrzej >>>> - added docu to mixer context struct >>>> - simplify mixer_win_reset() as well >>>> >>>> Changes in v3: >>>> - simplify some conditions as suggested by Inki >>>> - add docu to mixer_cfg_layer() >>>> - fold switch statements into a single one >>>> >>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>> index 1e78d57..4f06f4d 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>> DRM_FORMAT_NV21, >>>> }; >>>> >>>> +/* >>>> + * Mixer context structure. >>>> + * >>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>> + * @planes: Array of plane objects for each of the mixer windows. >>>> + * @active_windows: Cache of the mixer's hardware state. >>>> + * Tracks which mixer windows are active/inactive. >>>> + * @pipe: The CRTC index. >>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>> + */ >>>> struct mixer_context { >>>> struct platform_device *pdev; >>>> struct device *dev; >>>> struct drm_device *drm_dev; >>>> struct exynos_drm_crtc *crtc; >>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>> + unsigned long active_windows; >>>> int pipe; >>>> unsigned long flags; >>>> >>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>> } >>>> >>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>> - unsigned int priority, bool enable) >>>> +/** >>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>> + * @ctx: mixer context >>>> + * >>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>> + * using the 'active_windows' field of the the mixer content, and >>>> + * the pixel format of the framebuffers associated with the enabled >>>> + * windows. >>>> + * >>>> + * Has to be called under mixer lock. >>>> + */ >>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>> { >>>> struct mixer_resources *res = &ctx->mixer_res; >>>> - u32 val = enable ? ~0 : 0; >>>> - >>>> - switch (win) { >>>> - case 0: >>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>> - MXR_LAYER_CFG_GRP0_MASK); >>>> - break; >>>> - case 1: >>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>> - MXR_LAYER_CFG_GRP1_MASK); >>>> + unsigned int win; >>>> >>>> - break; >>>> - case VP_DEFAULT_WIN: >>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>> - MXR_CFG_VP_ENABLE); >>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>> - MXR_LAYER_CFG_VP_MASK); >>>> + struct exynos_drm_plane_state *state; >>>> + struct drm_framebuffer *fb; >>>> + unsigned int priority; >>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>> + bool enable; >>>> + >>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>> + fb = state->fb; >>>> + >>>> + priority = state->base.normalized_zpos + 1; >>>> + enable = test_bit(win, &ctx->active_windows); >>>> + >>>> + if (!enable) >>>> + continue; >>>> + >>>> + BUG_ON(!fb); >>>> + >>>> + /* >>>> + * TODO: Don't enable alpha blending for the bottom window. >>>> + */ >>>> + switch (win) { >>>> + case 0: >>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>> + break; >>>> + >>>> + case 1: >>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>> + break; >>>> + >>>> + case VP_DEFAULT_WIN: >>>> + vp_enable = VP_ENABLE_ON; >>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>> + mixer_cfg_vp_blend(ctx); >>>> + break; >>>> } >>>> - break; >>>> } >>>> + >>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>> + >>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>> } >>>> >>>> static void mixer_run(struct mixer_context *ctx) >>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>> struct mixer_resources *res = &ctx->mixer_res; >>>> struct drm_framebuffer *fb = state->fb; >>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>> unsigned long flags; >>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>> bool tiled_mode = false; >>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>> >>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>> - mixer_cfg_vp_blend(ctx); >>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >> yes, you're right. I completly forgot about the legacy codepaths. Sorry >> about that! > > Are you talking about DRM_IOCTL_MODE_SETPLANE? > It does not seem to be legacy, or to be more precise it calls > .update_plane and .disable_plane > callbacks which in exynos are set as follow: > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > > So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. thanks a lot for the heads-up. I can confirm that drm_mode_setplane() also goes through the atomic path. The simplified callstack: - drm_mode_setplane() - __setplane_internal() - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) - update_plane() (same here) For completeness I've also checked drm_mode_setcrtc(), the other legacy DRM call that manipulates the primary plane. It goes the following route: - drm_mode_setcrtc - drm_mode_set_config_internal() - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) Also here: .set_config = drm_atomic_helper_set_config Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic route, so the patch is fine this way. @Inki: Can you still queue this up for 4.9.y? With best wishes, Tobias > > Regards > Andrzej >
2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: > Hello Andrzej, > > > Andrzej Hajda wrote: >> On 27.09.2016 13:22, Tobias Jakobi wrote: >>> Hello Inki, >>> >>> >>> Inki Dae wrote: >>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>> in mixer_cfg_layer(). >>>>> Trigger this via atomic flush. >>>>> >>>>> Changes in v2: >>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>> - rename fields as suggested by Andrzej >>>>> - added docu to mixer context struct >>>>> - simplify mixer_win_reset() as well >>>>> >>>>> Changes in v3: >>>>> - simplify some conditions as suggested by Inki >>>>> - add docu to mixer_cfg_layer() >>>>> - fold switch statements into a single one >>>>> >>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>> --- >>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> index 1e78d57..4f06f4d 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>> DRM_FORMAT_NV21, >>>>> }; >>>>> >>>>> +/* >>>>> + * Mixer context structure. >>>>> + * >>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>> + * Tracks which mixer windows are active/inactive. >>>>> + * @pipe: The CRTC index. >>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>> + */ >>>>> struct mixer_context { >>>>> struct platform_device *pdev; >>>>> struct device *dev; >>>>> struct drm_device *drm_dev; >>>>> struct exynos_drm_crtc *crtc; >>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>> + unsigned long active_windows; >>>>> int pipe; >>>>> unsigned long flags; >>>>> >>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>> } >>>>> >>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>> - unsigned int priority, bool enable) >>>>> +/** >>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>> + * @ctx: mixer context >>>>> + * >>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>> + * using the 'active_windows' field of the the mixer content, and >>>>> + * the pixel format of the framebuffers associated with the enabled >>>>> + * windows. >>>>> + * >>>>> + * Has to be called under mixer lock. >>>>> + */ >>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>> { >>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>> - u32 val = enable ? ~0 : 0; >>>>> - >>>>> - switch (win) { >>>>> - case 0: >>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>> - break; >>>>> - case 1: >>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>> + unsigned int win; >>>>> >>>>> - break; >>>>> - case VP_DEFAULT_WIN: >>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>> - MXR_CFG_VP_ENABLE); >>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>> - MXR_LAYER_CFG_VP_MASK); >>>>> + struct exynos_drm_plane_state *state; >>>>> + struct drm_framebuffer *fb; >>>>> + unsigned int priority; >>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>> + bool enable; >>>>> + >>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>> + fb = state->fb; >>>>> + >>>>> + priority = state->base.normalized_zpos + 1; >>>>> + enable = test_bit(win, &ctx->active_windows); >>>>> + >>>>> + if (!enable) >>>>> + continue; >>>>> + >>>>> + BUG_ON(!fb); >>>>> + >>>>> + /* >>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>> + */ >>>>> + switch (win) { >>>>> + case 0: >>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>> + break; >>>>> + >>>>> + case 1: >>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>> + break; >>>>> + >>>>> + case VP_DEFAULT_WIN: >>>>> + vp_enable = VP_ENABLE_ON; >>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>> + mixer_cfg_vp_blend(ctx); >>>>> + break; >>>>> } >>>>> - break; >>>>> } >>>>> + >>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>> + >>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>> } >>>>> >>>>> static void mixer_run(struct mixer_context *ctx) >>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>> struct drm_framebuffer *fb = state->fb; >>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>> unsigned long flags; >>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>> bool tiled_mode = false; >>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>> >>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>> - mixer_cfg_vp_blend(ctx); >>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>> about that! >> >> Are you talking about DRM_IOCTL_MODE_SETPLANE? >> It does not seem to be legacy, or to be more precise it calls >> .update_plane and .disable_plane >> callbacks which in exynos are set as follow: >> .update_plane = drm_atomic_helper_update_plane, >> .disable_plane = drm_atomic_helper_disable_plane, >> >> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. > thanks a lot for the heads-up. I can confirm that drm_mode_setplane() > also goes through the atomic path. > > The simplified callstack: > - drm_mode_setplane() > - __setplane_internal() > - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) > - update_plane() (same here) Sorry for previous comments not enough. update_plane ... - mixer_update_plane - vp_video_buffer or mixer_graph_buffer However, you removed mixer_cfg_layer call from above functions. So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. For this I mentioned already at previous my comment, "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. > > > For completeness I've also checked drm_mode_setcrtc(), the other legacy > DRM call that manipulates the primary plane. > > It goes the following route: > - drm_mode_setcrtc > - drm_mode_set_config_internal() > - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) > > Also here: .set_config = drm_atomic_helper_set_config > > Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic > route, so the patch is fine this way. > > @Inki: Can you still queue this up for 4.9.y? > > > With best wishes, > Tobias > > >> >> Regards >> Andrzej >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
2016년 09월 27일 20:22에 Tobias Jakobi 이(가) 쓴 글: > Hey Inki, > > > Inki Dae wrote: >> 2016년 09월 27일 14:40에 Tobias Jakobi 이(가) 쓴 글: >>> Inki Dae wrote: >>>> >>>> >>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>> in mixer_cfg_layer(). >>>>> Trigger this via atomic flush. >>>>> >>>>> Changes in v2: >>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>> - rename fields as suggested by Andrzej >>>>> - added docu to mixer context struct >>>>> - simplify mixer_win_reset() as well >>>>> >>>>> Changes in v3: >>>>> - simplify some conditions as suggested by Inki >>>>> - add docu to mixer_cfg_layer() >>>>> - fold switch statements into a single one >>>>> >>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>> --- >>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> index 1e78d57..4f06f4d 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>> DRM_FORMAT_NV21, >>>>> }; >>>>> >>>>> +/* >>>>> + * Mixer context structure. >>>>> + * >>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>> + * Tracks which mixer windows are active/inactive. >>>>> + * @pipe: The CRTC index. >>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>> + */ >>>>> struct mixer_context { >>>>> struct platform_device *pdev; >>>>> struct device *dev; >>>>> struct drm_device *drm_dev; >>>>> struct exynos_drm_crtc *crtc; >>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>> + unsigned long active_windows; >>>>> int pipe; >>>>> unsigned long flags; >>>>> >>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>> } >>>>> >>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>> - unsigned int priority, bool enable) >>>>> +/** >>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>> + * @ctx: mixer context >>>>> + * >>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>> + * using the 'active_windows' field of the the mixer content, and >>>>> + * the pixel format of the framebuffers associated with the enabled >>>>> + * windows. >>>>> + * >>>>> + * Has to be called under mixer lock. >>>>> + */ >>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>> { >>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>> - u32 val = enable ? ~0 : 0; >>>>> - >>>>> - switch (win) { >>>>> - case 0: >>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>> - break; >>>>> - case 1: >>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>> + unsigned int win; >>>>> >>>>> - break; >>>>> - case VP_DEFAULT_WIN: >>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>> - MXR_CFG_VP_ENABLE); >>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>> - MXR_LAYER_CFG_VP_MASK); >>>>> + struct exynos_drm_plane_state *state; >>>>> + struct drm_framebuffer *fb; >>>>> + unsigned int priority; >>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>> + bool enable; >>>>> + >>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>> + fb = state->fb; >>>>> + >>>>> + priority = state->base.normalized_zpos + 1; >>>>> + enable = test_bit(win, &ctx->active_windows); >>>>> + >>>>> + if (!enable) >>>>> + continue; >>>>> + >>>>> + BUG_ON(!fb); >>>> >>>> Not good. Even through some problem happens at mixer driver, other KMS drivers should work. >>> That would be a issue at core level. And in this case, BUG_ON() should >>> be triggered. >> >> If so, the fb should be checked at core level not specific driver. > I'm not sure I can follow you here. So you actually agree on putting > BUG_ON() here, or not? > > Or let me phrase it differently. We have three options here: > (1) Put a BUG_ON(). If something goes terribly wrong in DRM core and fb > is NULL at this point, then we get a nice descriptive Oops. > (2) Put nothing here. Again, something goes terribly wrong, fb is NULL, > etc. Then we also get an Oops here, since we dereference NULL. > (3) Put 'if (fb) something();' here. Now we mask/ignore the issue in DRM > core and continue as if nothing went wrong. > > Obviously (3) is the worst approach. We want to fix bugs, not hide them. > Second best in my opinion is (2). The Oops is going to tell us that NULL > was dereferenced, but depending on the build, it's not immediately clear > where in the code that happens. > Best is (1) in my opinion since the Oops is more descriptive and the > code itself actually tells us that this shouldn't happen. Simply saying, the issue should be handled by someone who made the issue. How about adding the BUG_ON at core code which incurs actual problem if you can assue it? Otherwise, is there any reason that this exception should be handled only here? IMO, looks reasonable to remove such BUG_ON call from specific driver. > > > With best wishes, > Tobias > > > >> >>> >>> - Tobias >>> >>> >>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
2016년 09월 28일 08:31에 Inki Dae 이(가) 쓴 글: > > > 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >> Hello Andrzej, >> >> >> Andrzej Hajda wrote: >>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>> Hello Inki, >>>> >>>> >>>> Inki Dae wrote: >>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>> in mixer_cfg_layer(). >>>>>> Trigger this via atomic flush. >>>>>> >>>>>> Changes in v2: >>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>> - rename fields as suggested by Andrzej >>>>>> - added docu to mixer context struct >>>>>> - simplify mixer_win_reset() as well >>>>>> >>>>>> Changes in v3: >>>>>> - simplify some conditions as suggested by Inki >>>>>> - add docu to mixer_cfg_layer() >>>>>> - fold switch statements into a single one >>>>>> >>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>> --- >>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>> index 1e78d57..4f06f4d 100644 >>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>> DRM_FORMAT_NV21, >>>>>> }; >>>>>> >>>>>> +/* >>>>>> + * Mixer context structure. >>>>>> + * >>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>> + * Tracks which mixer windows are active/inactive. >>>>>> + * @pipe: The CRTC index. >>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>> + */ >>>>>> struct mixer_context { >>>>>> struct platform_device *pdev; >>>>>> struct device *dev; >>>>>> struct drm_device *drm_dev; >>>>>> struct exynos_drm_crtc *crtc; >>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>> + unsigned long active_windows; >>>>>> int pipe; >>>>>> unsigned long flags; >>>>>> >>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>> } >>>>>> >>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>> - unsigned int priority, bool enable) >>>>>> +/** >>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>> + * @ctx: mixer context >>>>>> + * >>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>> + * windows. >>>>>> + * >>>>>> + * Has to be called under mixer lock. >>>>>> + */ >>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>> { >>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>> - u32 val = enable ? ~0 : 0; >>>>>> - >>>>>> - switch (win) { >>>>>> - case 0: >>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>> - break; >>>>>> - case 1: >>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>> + unsigned int win; >>>>>> >>>>>> - break; >>>>>> - case VP_DEFAULT_WIN: >>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>> - MXR_CFG_VP_ENABLE); >>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>> + struct exynos_drm_plane_state *state; >>>>>> + struct drm_framebuffer *fb; >>>>>> + unsigned int priority; >>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>> + bool enable; >>>>>> + >>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>> + fb = state->fb; >>>>>> + >>>>>> + priority = state->base.normalized_zpos + 1; >>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>> + >>>>>> + if (!enable) >>>>>> + continue; >>>>>> + >>>>>> + BUG_ON(!fb); >>>>>> + >>>>>> + /* >>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>> + */ >>>>>> + switch (win) { >>>>>> + case 0: >>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>> + break; >>>>>> + >>>>>> + case 1: >>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>> + break; >>>>>> + >>>>>> + case VP_DEFAULT_WIN: >>>>>> + vp_enable = VP_ENABLE_ON; >>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>> + mixer_cfg_vp_blend(ctx); >>>>>> + break; >>>>>> } >>>>>> - break; >>>>>> } >>>>>> + >>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>> + >>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>> } >>>>>> >>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>> struct drm_framebuffer *fb = state->fb; >>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>> unsigned long flags; >>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>> bool tiled_mode = false; >>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>> >>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>> - mixer_cfg_vp_blend(ctx); >>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>> about that! >>> >>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>> It does not seem to be legacy, or to be more precise it calls >>> .update_plane and .disable_plane >>> callbacks which in exynos are set as follow: >>> .update_plane = drm_atomic_helper_update_plane, >>> .disable_plane = drm_atomic_helper_disable_plane, >>> >>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >> also goes through the atomic path. >> >> The simplified callstack: >> - drm_mode_setplane() >> - __setplane_internal() >> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >> - update_plane() (same here) > > Sorry for previous comments not enough. > > update_plane > ... > - mixer_update_plane > - vp_video_buffer or mixer_graph_buffer > > However, you removed mixer_cfg_layer call from above functions. > > So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. > mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. > For this I mentioned already at previous my comment, > "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." > > Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. > How about calling mixer_cfg_layer function just in mixer_update_plane? Seems more reasonable becasue now mixer_cfg_layer handles both of them - video and grphis layers. >> >> >> For completeness I've also checked drm_mode_setcrtc(), the other legacy >> DRM call that manipulates the primary plane. >> >> It goes the following route: >> - drm_mode_setcrtc >> - drm_mode_set_config_internal() >> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >> >> Also here: .set_config = drm_atomic_helper_set_config >> >> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >> route, so the patch is fine this way. >> >> @Inki: Can you still queue this up for 4.9.y? >> >> >> With best wishes, >> Tobias >> >> >>> >>> Regards >>> Andrzej >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >>
Hey Inki, Inki Dae wrote: > > > 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >> Hello Andrzej, >> >> >> Andrzej Hajda wrote: >>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>> Hello Inki, >>>> >>>> >>>> Inki Dae wrote: >>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>> in mixer_cfg_layer(). >>>>>> Trigger this via atomic flush. >>>>>> >>>>>> Changes in v2: >>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>> - rename fields as suggested by Andrzej >>>>>> - added docu to mixer context struct >>>>>> - simplify mixer_win_reset() as well >>>>>> >>>>>> Changes in v3: >>>>>> - simplify some conditions as suggested by Inki >>>>>> - add docu to mixer_cfg_layer() >>>>>> - fold switch statements into a single one >>>>>> >>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>> --- >>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>> index 1e78d57..4f06f4d 100644 >>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>> DRM_FORMAT_NV21, >>>>>> }; >>>>>> >>>>>> +/* >>>>>> + * Mixer context structure. >>>>>> + * >>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>> + * Tracks which mixer windows are active/inactive. >>>>>> + * @pipe: The CRTC index. >>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>> + */ >>>>>> struct mixer_context { >>>>>> struct platform_device *pdev; >>>>>> struct device *dev; >>>>>> struct drm_device *drm_dev; >>>>>> struct exynos_drm_crtc *crtc; >>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>> + unsigned long active_windows; >>>>>> int pipe; >>>>>> unsigned long flags; >>>>>> >>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>> } >>>>>> >>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>> - unsigned int priority, bool enable) >>>>>> +/** >>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>> + * @ctx: mixer context >>>>>> + * >>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>> + * windows. >>>>>> + * >>>>>> + * Has to be called under mixer lock. >>>>>> + */ >>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>> { >>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>> - u32 val = enable ? ~0 : 0; >>>>>> - >>>>>> - switch (win) { >>>>>> - case 0: >>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>> - break; >>>>>> - case 1: >>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>> + unsigned int win; >>>>>> >>>>>> - break; >>>>>> - case VP_DEFAULT_WIN: >>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>> - MXR_CFG_VP_ENABLE); >>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>> + struct exynos_drm_plane_state *state; >>>>>> + struct drm_framebuffer *fb; >>>>>> + unsigned int priority; >>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>> + bool enable; >>>>>> + >>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>> + fb = state->fb; >>>>>> + >>>>>> + priority = state->base.normalized_zpos + 1; >>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>> + >>>>>> + if (!enable) >>>>>> + continue; >>>>>> + >>>>>> + BUG_ON(!fb); >>>>>> + >>>>>> + /* >>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>> + */ >>>>>> + switch (win) { >>>>>> + case 0: >>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>> + break; >>>>>> + >>>>>> + case 1: >>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>> + break; >>>>>> + >>>>>> + case VP_DEFAULT_WIN: >>>>>> + vp_enable = VP_ENABLE_ON; >>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>> + mixer_cfg_vp_blend(ctx); >>>>>> + break; >>>>>> } >>>>>> - break; >>>>>> } >>>>>> + >>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>> + >>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>> } >>>>>> >>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>> struct drm_framebuffer *fb = state->fb; >>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>> unsigned long flags; >>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>> bool tiled_mode = false; >>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>> >>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>> - mixer_cfg_vp_blend(ctx); >>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>> about that! >>> >>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>> It does not seem to be legacy, or to be more precise it calls >>> .update_plane and .disable_plane >>> callbacks which in exynos are set as follow: >>> .update_plane = drm_atomic_helper_update_plane, >>> .disable_plane = drm_atomic_helper_disable_plane, >>> >>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >> also goes through the atomic path. >> >> The simplified callstack: >> - drm_mode_setplane() >> - __setplane_internal() >> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >> - update_plane() (same here) > > Sorry for previous comments not enough. > > update_plane > ... > - mixer_update_plane > - vp_video_buffer or mixer_graph_buffer > > However, you removed mixer_cfg_layer call from above functions. exactly, because that is the very purpose of this patch. > So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. That is not my intention. I want to move register manipulation to the atomic flush step. > mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. Why do you think so? I think it makes perfect sense to put it there. We flush the new configuration to the hardware registers there. > For this I mentioned already at previous my comment, > "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." You're confusing me. That is exactly what my patch is doing. It moves more of the actual hardware register updating to the atomic flush step. > Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. Why is that? In mixer_win_reset() we want to apply a default configuration to the various registers. Hence we set active_windows to zero and call mixer_cfg_layer(). With best wishes, Tobias >> For completeness I've also checked drm_mode_setcrtc(), the other legacy >> DRM call that manipulates the primary plane. >> >> It goes the following route: >> - drm_mode_setcrtc >> - drm_mode_set_config_internal() >> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >> >> Also here: .set_config = drm_atomic_helper_set_config >> >> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >> route, so the patch is fine this way. >> >> @Inki: Can you still queue this up for 4.9.y? >> >> >> With best wishes, >> Tobias >> >> >>> >>> Regards >>> Andrzej >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hello Inki, Inki Dae wrote: > > > 2016년 09월 28일 08:31에 Inki Dae 이(가) 쓴 글: >> >> >> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>> Hello Andrzej, >>> >>> >>> Andrzej Hajda wrote: >>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>> Hello Inki, >>>>> >>>>> >>>>> Inki Dae wrote: >>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>> in mixer_cfg_layer(). >>>>>>> Trigger this via atomic flush. >>>>>>> >>>>>>> Changes in v2: >>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>> - rename fields as suggested by Andrzej >>>>>>> - added docu to mixer context struct >>>>>>> - simplify mixer_win_reset() as well >>>>>>> >>>>>>> Changes in v3: >>>>>>> - simplify some conditions as suggested by Inki >>>>>>> - add docu to mixer_cfg_layer() >>>>>>> - fold switch statements into a single one >>>>>>> >>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>> --- >>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>> DRM_FORMAT_NV21, >>>>>>> }; >>>>>>> >>>>>>> +/* >>>>>>> + * Mixer context structure. >>>>>>> + * >>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>> + * @pipe: The CRTC index. >>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>> + */ >>>>>>> struct mixer_context { >>>>>>> struct platform_device *pdev; >>>>>>> struct device *dev; >>>>>>> struct drm_device *drm_dev; >>>>>>> struct exynos_drm_crtc *crtc; >>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>> + unsigned long active_windows; >>>>>>> int pipe; >>>>>>> unsigned long flags; >>>>>>> >>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>> } >>>>>>> >>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>> - unsigned int priority, bool enable) >>>>>>> +/** >>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>> + * @ctx: mixer context >>>>>>> + * >>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>> + * windows. >>>>>>> + * >>>>>>> + * Has to be called under mixer lock. >>>>>>> + */ >>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>> { >>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>> - >>>>>>> - switch (win) { >>>>>>> - case 0: >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>> - break; >>>>>>> - case 1: >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>> + unsigned int win; >>>>>>> >>>>>>> - break; >>>>>>> - case VP_DEFAULT_WIN: >>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>> + struct exynos_drm_plane_state *state; >>>>>>> + struct drm_framebuffer *fb; >>>>>>> + unsigned int priority; >>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>> + bool enable; >>>>>>> + >>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>> + fb = state->fb; >>>>>>> + >>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>> + >>>>>>> + if (!enable) >>>>>>> + continue; >>>>>>> + >>>>>>> + BUG_ON(!fb); >>>>>>> + >>>>>>> + /* >>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>> + */ >>>>>>> + switch (win) { >>>>>>> + case 0: >>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>> + break; >>>>>>> + >>>>>>> + case 1: >>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>> + break; >>>>>>> + >>>>>>> + case VP_DEFAULT_WIN: >>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>> + break; >>>>>>> } >>>>>>> - break; >>>>>>> } >>>>>>> + >>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>> + >>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>> } >>>>>>> >>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>> unsigned long flags; >>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>> bool tiled_mode = false; >>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>> >>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>> about that! >>>> >>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>> It does not seem to be legacy, or to be more precise it calls >>>> .update_plane and .disable_plane >>>> callbacks which in exynos are set as follow: >>>> .update_plane = drm_atomic_helper_update_plane, >>>> .disable_plane = drm_atomic_helper_disable_plane, >>>> >>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>> also goes through the atomic path. >>> >>> The simplified callstack: >>> - drm_mode_setplane() >>> - __setplane_internal() >>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>> - update_plane() (same here) >> >> Sorry for previous comments not enough. >> >> update_plane >> ... >> - mixer_update_plane >> - vp_video_buffer or mixer_graph_buffer >> >> However, you removed mixer_cfg_layer call from above functions. >> >> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >> For this I mentioned already at previous my comment, >> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >> >> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >> > > How about calling mixer_cfg_layer function just in mixer_update_plane? Seems more reasonable becasue now mixer_cfg_layer handles both of them - video and grphis layers. An example of a atomic operation: mixer_atomic_check() mixer_atomic_begin() mixer_update_plane() mixer_disable_plane() mixer_update_plane() mixer_atomic_flush() Flushing to hardware registers, i.e. writing MXR_CFG and MXR_LAYER_CFG should happen _once_ in mixer_atomic_flush(). With best wishes, Tobias P.S.: While this is only the first step, I also plan to move mixer_cfg_scan() and mixer_cfg_rgb_fmt() to mixer_atomic_flush(). > >>> >>> >>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>> DRM call that manipulates the primary plane. >>> >>> It goes the following route: >>> - drm_mode_setcrtc >>> - drm_mode_set_config_internal() >>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>> >>> Also here: .set_config = drm_atomic_helper_set_config >>> >>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>> route, so the patch is fine this way. >>> >>> @Inki: Can you still queue this up for 4.9.y? >>> >>> >>> With best wishes, >>> Tobias >>> >>> >>>> >>>> Regards >>>> Andrzej >>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: > Hey Inki, > > > Inki Dae wrote: >> >> >> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>> Hello Andrzej, >>> >>> >>> Andrzej Hajda wrote: >>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>> Hello Inki, >>>>> >>>>> >>>>> Inki Dae wrote: >>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>> in mixer_cfg_layer(). >>>>>>> Trigger this via atomic flush. >>>>>>> >>>>>>> Changes in v2: >>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>> - rename fields as suggested by Andrzej >>>>>>> - added docu to mixer context struct >>>>>>> - simplify mixer_win_reset() as well >>>>>>> >>>>>>> Changes in v3: >>>>>>> - simplify some conditions as suggested by Inki >>>>>>> - add docu to mixer_cfg_layer() >>>>>>> - fold switch statements into a single one >>>>>>> >>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>> --- >>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>> DRM_FORMAT_NV21, >>>>>>> }; >>>>>>> >>>>>>> +/* >>>>>>> + * Mixer context structure. >>>>>>> + * >>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>> + * @pipe: The CRTC index. >>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>> + */ >>>>>>> struct mixer_context { >>>>>>> struct platform_device *pdev; >>>>>>> struct device *dev; >>>>>>> struct drm_device *drm_dev; >>>>>>> struct exynos_drm_crtc *crtc; >>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>> + unsigned long active_windows; >>>>>>> int pipe; >>>>>>> unsigned long flags; >>>>>>> >>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>> } >>>>>>> >>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>> - unsigned int priority, bool enable) >>>>>>> +/** >>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>> + * @ctx: mixer context >>>>>>> + * >>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>> + * windows. >>>>>>> + * >>>>>>> + * Has to be called under mixer lock. >>>>>>> + */ >>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>> { >>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>> - >>>>>>> - switch (win) { >>>>>>> - case 0: >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>> - break; >>>>>>> - case 1: >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>> + unsigned int win; >>>>>>> >>>>>>> - break; >>>>>>> - case VP_DEFAULT_WIN: >>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>> + struct exynos_drm_plane_state *state; >>>>>>> + struct drm_framebuffer *fb; >>>>>>> + unsigned int priority; >>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>> + bool enable; >>>>>>> + >>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>> + fb = state->fb; >>>>>>> + >>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>> + >>>>>>> + if (!enable) >>>>>>> + continue; >>>>>>> + >>>>>>> + BUG_ON(!fb); >>>>>>> + >>>>>>> + /* >>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>> + */ >>>>>>> + switch (win) { >>>>>>> + case 0: >>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>> + break; >>>>>>> + >>>>>>> + case 1: >>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>> + break; >>>>>>> + >>>>>>> + case VP_DEFAULT_WIN: >>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>> + break; >>>>>>> } >>>>>>> - break; >>>>>>> } >>>>>>> + >>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>> + >>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>> } >>>>>>> >>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>> unsigned long flags; >>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>> bool tiled_mode = false; >>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>> >>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>> about that! >>>> >>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>> It does not seem to be legacy, or to be more precise it calls >>>> .update_plane and .disable_plane >>>> callbacks which in exynos are set as follow: >>>> .update_plane = drm_atomic_helper_update_plane, >>>> .disable_plane = drm_atomic_helper_disable_plane, >>>> >>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>> also goes through the atomic path. >>> >>> The simplified callstack: >>> - drm_mode_setplane() >>> - __setplane_internal() >>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>> - update_plane() (same here) >> >> Sorry for previous comments not enough. >> >> update_plane >> ... >> - mixer_update_plane >> - vp_video_buffer or mixer_graph_buffer >> >> However, you removed mixer_cfg_layer call from above functions. > exactly, because that is the very purpose of this patch. > > >> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. > That is not my intention. I want to move register manipulation to the > atomic flush step. > > >> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. > Why do you think so? I think it makes perfect sense to put it there. We > flush the new configuration to the hardware registers there. > > >> For this I mentioned already at previous my comment, >> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." > You're confusing me. That is exactly what my patch is doing. It moves > more of the actual hardware register updating to the atomic flush step. Ok, for your understanding, you can check atomic_begin, atomic_flush and update_plane callback functions of exynos_drm_fimd.c. After that I think we could continue to discuss this. I doesn't really want to tackle you. > > >> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. > Why is that? In mixer_win_reset() we want to apply a default > configuration to the various registers. Hence we set active_windows to > zero and call mixer_cfg_layer(). > > > With best wishes, > Tobias > > > >>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>> DRM call that manipulates the primary plane. >>> >>> It goes the following route: >>> - drm_mode_setcrtc >>> - drm_mode_set_config_internal() >>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>> >>> Also here: .set_config = drm_atomic_helper_set_config >>> >>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>> route, so the patch is fine this way. >>> >>> @Inki: Can you still queue this up for 4.9.y? >>> >>> >>> With best wishes, >>> Tobias >>> >>> >>>> >>>> Regards >>>> Andrzej >>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > >
2016년 09월 28일 09:12에 Tobias Jakobi 이(가) 쓴 글: > Hello Inki, > > > Inki Dae wrote: >> >> >> 2016년 09월 28일 08:31에 Inki Dae 이(가) 쓴 글: >>> >>> >>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>> Hello Andrzej, >>>> >>>> >>>> Andrzej Hajda wrote: >>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>> Hello Inki, >>>>>> >>>>>> >>>>>> Inki Dae wrote: >>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>> in mixer_cfg_layer(). >>>>>>>> Trigger this via atomic flush. >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>> - rename fields as suggested by Andrzej >>>>>>>> - added docu to mixer context struct >>>>>>>> - simplify mixer_win_reset() as well >>>>>>>> >>>>>>>> Changes in v3: >>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>> - fold switch statements into a single one >>>>>>>> >>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>> DRM_FORMAT_NV21, >>>>>>>> }; >>>>>>>> >>>>>>>> +/* >>>>>>>> + * Mixer context structure. >>>>>>>> + * >>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>> + * @pipe: The CRTC index. >>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>> + */ >>>>>>>> struct mixer_context { >>>>>>>> struct platform_device *pdev; >>>>>>>> struct device *dev; >>>>>>>> struct drm_device *drm_dev; >>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>> + unsigned long active_windows; >>>>>>>> int pipe; >>>>>>>> unsigned long flags; >>>>>>>> >>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>> } >>>>>>>> >>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>> - unsigned int priority, bool enable) >>>>>>>> +/** >>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>> + * @ctx: mixer context >>>>>>>> + * >>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>> + * windows. >>>>>>>> + * >>>>>>>> + * Has to be called under mixer lock. >>>>>>>> + */ >>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>> { >>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>> - >>>>>>>> - switch (win) { >>>>>>>> - case 0: >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>> - break; >>>>>>>> - case 1: >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>> + unsigned int win; >>>>>>>> >>>>>>>> - break; >>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>> + struct drm_framebuffer *fb; >>>>>>>> + unsigned int priority; >>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>> + bool enable; >>>>>>>> + >>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>> + fb = state->fb; >>>>>>>> + >>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>> + >>>>>>>> + if (!enable) >>>>>>>> + continue; >>>>>>>> + >>>>>>>> + BUG_ON(!fb); >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>> + */ >>>>>>>> + switch (win) { >>>>>>>> + case 0: >>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>> + break; >>>>>>>> + >>>>>>>> + case 1: >>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>> + break; >>>>>>>> + >>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>> + break; >>>>>>>> } >>>>>>>> - break; >>>>>>>> } >>>>>>>> + >>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>> + >>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>> } >>>>>>>> >>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>> unsigned long flags; >>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>> bool tiled_mode = false; >>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>> >>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>> about that! >>>>> >>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>> It does not seem to be legacy, or to be more precise it calls >>>>> .update_plane and .disable_plane >>>>> callbacks which in exynos are set as follow: >>>>> .update_plane = drm_atomic_helper_update_plane, >>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>> >>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>> also goes through the atomic path. >>>> >>>> The simplified callstack: >>>> - drm_mode_setplane() >>>> - __setplane_internal() >>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>> - update_plane() (same here) >>> >>> Sorry for previous comments not enough. >>> >>> update_plane >>> ... >>> - mixer_update_plane >>> - vp_video_buffer or mixer_graph_buffer >>> >>> However, you removed mixer_cfg_layer call from above functions. >>> >>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >>> For this I mentioned already at previous my comment, >>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >>> >>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >>> >> >> How about calling mixer_cfg_layer function just in mixer_update_plane? Seems more reasonable becasue now mixer_cfg_layer handles both of them - video and grphis layers. > An example of a atomic operation: > mixer_atomic_check() > mixer_atomic_begin() > mixer_update_plane() > mixer_disable_plane() > mixer_update_plane() > mixer_atomic_flush() > > Flushing to hardware registers, i.e. writing MXR_CFG and MXR_LAYER_CFG > should happen _once_ in mixer_atomic_flush(). Please check my comments of previous email. you need to check relevant callback functions of exynos_drm_fimd.c. > > > With best wishes, > Tobias > > P.S.: While this is only the first step, I also plan to move > mixer_cfg_scan() and mixer_cfg_rgb_fmt() to mixer_atomic_flush(). > > >> >>>> >>>> >>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>> DRM call that manipulates the primary plane. >>>> >>>> It goes the following route: >>>> - drm_mode_setcrtc >>>> - drm_mode_set_config_internal() >>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>> >>>> Also here: .set_config = drm_atomic_helper_set_config >>>> >>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>> route, so the patch is fine this way. >>>> >>>> @Inki: Can you still queue this up for 4.9.y? >>>> >>>> >>>> With best wishes, >>>> Tobias >>>> >>>> >>>>> >>>>> Regards >>>>> Andrzej >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: > Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once > in mixer_cfg_layer(). > Trigger this via atomic flush. > > Changes in v2: > - issue mixer_cfg_layer() in mixer_disable() > - rename fields as suggested by Andrzej > - added docu to mixer context struct > - simplify mixer_win_reset() as well > > Changes in v3: > - simplify some conditions as suggested by Inki > - add docu to mixer_cfg_layer() > - fold switch statements into a single one > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ > drivers/gpu/drm/exynos/regs-mixer.h | 2 + > 2 files changed, 92 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 1e78d57..4f06f4d 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { > DRM_FORMAT_NV21, > }; > > +/* > + * Mixer context structure. > + * > + * @crtc: The HDMI CRTC attached to the mixer. > + * @planes: Array of plane objects for each of the mixer windows. > + * @active_windows: Cache of the mixer's hardware state. > + * Tracks which mixer windows are active/inactive. > + * @pipe: The CRTC index. > + * @flags: Bitfield build from the mixer_flag_bits enumerator. > + * @mixer_resources: A struct containing registers, clocks, etc. > + * @mxr_ver: The hardware revision/version of the mixer. > + */ > struct mixer_context { > struct platform_device *pdev; > struct device *dev; > struct drm_device *drm_dev; > struct exynos_drm_crtc *crtc; > struct exynos_drm_plane planes[MIXER_WIN_NR]; > + unsigned long active_windows; > int pipe; > unsigned long flags; > > @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); > } > > -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, > - unsigned int priority, bool enable) > +/** > + * mixer_cfg_layer - apply layer configuration to hardware > + * @ctx: mixer context > + * > + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers > + * using the 'active_windows' field of the the mixer content, and > + * the pixel format of the framebuffers associated with the enabled > + * windows. > + * > + * Has to be called under mixer lock. > + */ > +static void mixer_cfg_layer(struct mixer_context *ctx) > { > struct mixer_resources *res = &ctx->mixer_res; > - u32 val = enable ? ~0 : 0; > - > - switch (win) { > - case 0: > - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_GRP0_VAL(priority), > - MXR_LAYER_CFG_GRP0_MASK); > - break; > - case 1: > - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_GRP1_VAL(priority), > - MXR_LAYER_CFG_GRP1_MASK); > + unsigned int win; > > - break; > - case VP_DEFAULT_WIN: > - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { > - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); > - mixer_reg_writemask(res, MXR_CFG, val, > - MXR_CFG_VP_ENABLE); > - mixer_reg_writemask(res, MXR_LAYER_CFG, > - MXR_LAYER_CFG_VP_VAL(priority), > - MXR_LAYER_CFG_VP_MASK); > + struct exynos_drm_plane_state *state; > + struct drm_framebuffer *fb; > + unsigned int priority; > + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; > + bool enable; > + > + for (win = 0; win < MIXER_WIN_NR; ++win) { > + state = to_exynos_plane_state(ctx->planes[win].base.state); > + fb = state->fb; > + > + priority = state->base.normalized_zpos + 1; > + enable = test_bit(win, &ctx->active_windows); > + > + if (!enable) > + continue; > + > + BUG_ON(!fb); > + > + /* > + * TODO: Don't enable alpha blending for the bottom window. > + */ > + switch (win) { > + case 0: > + mxr_cfg |= MXR_CFG_GRP0_ENABLE; > + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); > + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); > + break; > + > + case 1: > + mxr_cfg |= MXR_CFG_GRP1_ENABLE; > + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); > + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); > + break; > + > + case VP_DEFAULT_WIN: > + vp_enable = VP_ENABLE_ON; > + mxr_cfg |= MXR_CFG_VP_ENABLE; > + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); > + mixer_cfg_vp_blend(ctx); > + break; > } > - break; > } > + > + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) > + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); > + > + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); > + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); > } > > static void mixer_run(struct mixer_context *ctx) > @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, > struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; > struct mixer_resources *res = &ctx->mixer_res; > struct drm_framebuffer *fb = state->fb; > - unsigned int priority = state->base.normalized_zpos + 1; > unsigned long flags; > dma_addr_t luma_addr[2], chroma_addr[2]; > bool tiled_mode = false; > @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, > > mixer_cfg_scan(ctx, mode->vdisplay); > mixer_cfg_rgb_fmt(ctx, mode->vdisplay); > - mixer_cfg_layer(ctx, plane->index, priority, true); > - mixer_cfg_vp_blend(ctx); > mixer_run(ctx); > > spin_unlock_irqrestore(&res->reg_slock, flags); > @@ -588,7 +631,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; > struct mixer_resources *res = &ctx->mixer_res; > struct drm_framebuffer *fb = state->fb; > - unsigned int priority = state->base.normalized_zpos + 1; > unsigned long flags; > unsigned int win = plane->index; > unsigned int x_ratio = 0, y_ratio = 0; > @@ -680,8 +722,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > > mixer_cfg_scan(ctx, mode->vdisplay); > mixer_cfg_rgb_fmt(ctx, mode->vdisplay); > - mixer_cfg_layer(ctx, win, priority, true); > - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); > > /* layer update mandatory for mixer 16.0.33.0 */ > if (ctx->mxr_ver == MXR_VER_16_0_33_0 || > @@ -726,9 +766,6 @@ static void mixer_win_reset(struct mixer_context *ctx) > mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST, > MXR_STATUS_BURST_MASK); > > - /* reset default layer priority */ > - mixer_reg_write(res, MXR_LAYER_CFG, 0); > - > /* setting background color */ > mixer_reg_write(res, MXR_BG_COLOR0, 0x008080); > mixer_reg_write(res, MXR_BG_COLOR1, 0x008080); > @@ -740,11 +777,9 @@ static void mixer_win_reset(struct mixer_context *ctx) > vp_default_filter(res); > } > > - /* disable all layers */ > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); > - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); > + /* disable all layers and reset layer priority to default */ > + ctx->active_windows = 0; > + mixer_cfg_layer(ctx); > > spin_unlock_irqrestore(&res->reg_slock, flags); > } > @@ -994,32 +1029,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, > vp_video_buffer(mixer_ctx, plane); > else > mixer_graph_buffer(mixer_ctx, plane); > + > + __set_bit(plane->index, &mixer_ctx->active_windows); > } > > static void mixer_disable_plane(struct exynos_drm_crtc *crtc, > struct exynos_drm_plane *plane) > { > struct mixer_context *mixer_ctx = crtc->ctx; > - struct mixer_resources *res = &mixer_ctx->mixer_res; > - unsigned long flags; > > DRM_DEBUG_KMS("win: %d\n", plane->index); > > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > return; > > - spin_lock_irqsave(&res->reg_slock, flags); > - mixer_cfg_layer(mixer_ctx, plane->index, 0, false); > - spin_unlock_irqrestore(&res->reg_slock, flags); > + __clear_bit(plane->index, &mixer_ctx->active_windows); > } > > static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) > { > struct mixer_context *mixer_ctx = crtc->ctx; > + struct mixer_resources *res = &mixer_ctx->mixer_res; > + unsigned long flags; > > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > return; > > + spin_lock_irqsave(&res->reg_slock, flags); > + mixer_cfg_layer(mixer_ctx); > + spin_unlock_irqrestore(&res->reg_slock, flags); > + > mixer_vsync_set_update(mixer_ctx, true); > } > > @@ -1053,6 +1092,8 @@ static void mixer_enable(struct exynos_drm_crtc *crtc) > static void mixer_disable(struct exynos_drm_crtc *crtc) > { > struct mixer_context *ctx = crtc->ctx; > + struct mixer_resources *res = &ctx->mixer_res; > + unsigned long flags; > int i; > > if (!test_bit(MXR_BIT_POWERED, &ctx->flags)) > @@ -1064,6 +1105,10 @@ static void mixer_disable(struct exynos_drm_crtc *crtc) > for (i = 0; i < MIXER_WIN_NR; i++) > mixer_disable_plane(crtc, &ctx->planes[i]); > > + spin_lock_irqsave(&res->reg_slock, flags); > + mixer_cfg_layer(ctx); Is there any reasone to call mixer_cfg_layer function here? mixer_disable function tries to disable mixer device.
2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: > Hey Inki, > > > Inki Dae wrote: >> >> >> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>> Hello Andrzej, >>> >>> >>> Andrzej Hajda wrote: >>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>> Hello Inki, >>>>> >>>>> >>>>> Inki Dae wrote: >>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>> in mixer_cfg_layer(). >>>>>>> Trigger this via atomic flush. >>>>>>> >>>>>>> Changes in v2: >>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>> - rename fields as suggested by Andrzej >>>>>>> - added docu to mixer context struct >>>>>>> - simplify mixer_win_reset() as well >>>>>>> >>>>>>> Changes in v3: >>>>>>> - simplify some conditions as suggested by Inki >>>>>>> - add docu to mixer_cfg_layer() >>>>>>> - fold switch statements into a single one >>>>>>> >>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>> --- >>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>> DRM_FORMAT_NV21, >>>>>>> }; >>>>>>> >>>>>>> +/* >>>>>>> + * Mixer context structure. >>>>>>> + * >>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>> + * @pipe: The CRTC index. >>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>> + */ >>>>>>> struct mixer_context { >>>>>>> struct platform_device *pdev; >>>>>>> struct device *dev; >>>>>>> struct drm_device *drm_dev; >>>>>>> struct exynos_drm_crtc *crtc; >>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>> + unsigned long active_windows; >>>>>>> int pipe; >>>>>>> unsigned long flags; >>>>>>> >>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>> } >>>>>>> >>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>> - unsigned int priority, bool enable) >>>>>>> +/** >>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>> + * @ctx: mixer context >>>>>>> + * >>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>> + * windows. >>>>>>> + * >>>>>>> + * Has to be called under mixer lock. >>>>>>> + */ >>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>> { >>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>> - >>>>>>> - switch (win) { >>>>>>> - case 0: >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>> - break; >>>>>>> - case 1: >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>> + unsigned int win; >>>>>>> >>>>>>> - break; >>>>>>> - case VP_DEFAULT_WIN: >>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>> + struct exynos_drm_plane_state *state; >>>>>>> + struct drm_framebuffer *fb; >>>>>>> + unsigned int priority; >>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>> + bool enable; >>>>>>> + >>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>> + fb = state->fb; >>>>>>> + >>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>> + >>>>>>> + if (!enable) >>>>>>> + continue; >>>>>>> + >>>>>>> + BUG_ON(!fb); >>>>>>> + >>>>>>> + /* >>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>> + */ >>>>>>> + switch (win) { >>>>>>> + case 0: >>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>> + break; >>>>>>> + >>>>>>> + case 1: >>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>> + break; >>>>>>> + >>>>>>> + case VP_DEFAULT_WIN: >>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>> + break; >>>>>>> } >>>>>>> - break; >>>>>>> } >>>>>>> + >>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>> + >>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>> } >>>>>>> >>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>> unsigned long flags; >>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>> bool tiled_mode = false; >>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>> >>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>> about that! >>>> >>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>> It does not seem to be legacy, or to be more precise it calls >>>> .update_plane and .disable_plane >>>> callbacks which in exynos are set as follow: >>>> .update_plane = drm_atomic_helper_update_plane, >>>> .disable_plane = drm_atomic_helper_disable_plane, >>>> >>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>> also goes through the atomic path. >>> >>> The simplified callstack: >>> - drm_mode_setplane() >>> - __setplane_internal() >>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>> - update_plane() (same here) >> >> Sorry for previous comments not enough. >> >> update_plane >> ... >> - mixer_update_plane >> - vp_video_buffer or mixer_graph_buffer >> >> However, you removed mixer_cfg_layer call from above functions. > exactly, because that is the very purpose of this patch. > > >> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. > That is not my intention. I want to move register manipulation to the > atomic flush step. > > >> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. > Why do you think so? I think it makes perfect sense to put it there. We > flush the new configuration to the hardware registers there. > > >> For this I mentioned already at previous my comment, >> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." > You're confusing me. That is exactly what my patch is doing. It moves > more of the actual hardware register updating to the atomic flush step. > > >> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. > Why is that? In mixer_win_reset() we want to apply a default > configuration to the various registers. Hence we set active_windows to > zero and call mixer_cfg_layer(). mixer_win_reset function sets registers to default value because mixer device has no reset HW register like vp did. So original codes set the registers to default value but you removed them like below, - /* reset default layer priority */ - mixer_reg_write(res, MXR_LAYER_CFG, 0); - /* disable all layers */ - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); and called mixer_cfg_layer function which configures overlay registers as previous plane state but not HW default values. With this patch do you know that how many times mixer_cfg_layer function is called when we requested atomic operation? > > > With best wishes, > Tobias > > > >>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>> DRM call that manipulates the primary plane. >>> >>> It goes the following route: >>> - drm_mode_setcrtc >>> - drm_mode_set_config_internal() >>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>> >>> Also here: .set_config = drm_atomic_helper_set_config >>> >>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>> route, so the patch is fine this way. >>> >>> @Inki: Can you still queue this up for 4.9.y? >>> >>> >>> With best wishes, >>> Tobias >>> >>> >>>> >>>> Regards >>>> Andrzej >>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > >
Inki Dae wrote: > > > 2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: >> Hey Inki, >> >> >> Inki Dae wrote: >>> >>> >>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>> Hello Andrzej, >>>> >>>> >>>> Andrzej Hajda wrote: >>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>> Hello Inki, >>>>>> >>>>>> >>>>>> Inki Dae wrote: >>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>> in mixer_cfg_layer(). >>>>>>>> Trigger this via atomic flush. >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>> - rename fields as suggested by Andrzej >>>>>>>> - added docu to mixer context struct >>>>>>>> - simplify mixer_win_reset() as well >>>>>>>> >>>>>>>> Changes in v3: >>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>> - fold switch statements into a single one >>>>>>>> >>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>> DRM_FORMAT_NV21, >>>>>>>> }; >>>>>>>> >>>>>>>> +/* >>>>>>>> + * Mixer context structure. >>>>>>>> + * >>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>> + * @pipe: The CRTC index. >>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>> + */ >>>>>>>> struct mixer_context { >>>>>>>> struct platform_device *pdev; >>>>>>>> struct device *dev; >>>>>>>> struct drm_device *drm_dev; >>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>> + unsigned long active_windows; >>>>>>>> int pipe; >>>>>>>> unsigned long flags; >>>>>>>> >>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>> } >>>>>>>> >>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>> - unsigned int priority, bool enable) >>>>>>>> +/** >>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>> + * @ctx: mixer context >>>>>>>> + * >>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>> + * windows. >>>>>>>> + * >>>>>>>> + * Has to be called under mixer lock. >>>>>>>> + */ >>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>> { >>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>> - >>>>>>>> - switch (win) { >>>>>>>> - case 0: >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>> - break; >>>>>>>> - case 1: >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>> + unsigned int win; >>>>>>>> >>>>>>>> - break; >>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>> + struct drm_framebuffer *fb; >>>>>>>> + unsigned int priority; >>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>> + bool enable; >>>>>>>> + >>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>> + fb = state->fb; >>>>>>>> + >>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>> + >>>>>>>> + if (!enable) >>>>>>>> + continue; >>>>>>>> + >>>>>>>> + BUG_ON(!fb); >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>> + */ >>>>>>>> + switch (win) { >>>>>>>> + case 0: >>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>> + break; >>>>>>>> + >>>>>>>> + case 1: >>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>> + break; >>>>>>>> + >>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>> + break; >>>>>>>> } >>>>>>>> - break; >>>>>>>> } >>>>>>>> + >>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>> + >>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>> } >>>>>>>> >>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>> unsigned long flags; >>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>> bool tiled_mode = false; >>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>> >>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>> about that! >>>>> >>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>> It does not seem to be legacy, or to be more precise it calls >>>>> .update_plane and .disable_plane >>>>> callbacks which in exynos are set as follow: >>>>> .update_plane = drm_atomic_helper_update_plane, >>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>> >>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>> also goes through the atomic path. >>>> >>>> The simplified callstack: >>>> - drm_mode_setplane() >>>> - __setplane_internal() >>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>> - update_plane() (same here) >>> >>> Sorry for previous comments not enough. >>> >>> update_plane >>> ... >>> - mixer_update_plane >>> - vp_video_buffer or mixer_graph_buffer >>> >>> However, you removed mixer_cfg_layer call from above functions. >> exactly, because that is the very purpose of this patch. >> >> >>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >> That is not my intention. I want to move register manipulation to the >> atomic flush step. >> >> >>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >> Why do you think so? I think it makes perfect sense to put it there. We >> flush the new configuration to the hardware registers there. >> >> >>> For this I mentioned already at previous my comment, >>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >> You're confusing me. That is exactly what my patch is doing. It moves >> more of the actual hardware register updating to the atomic flush step. >> >> >>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >> Why is that? In mixer_win_reset() we want to apply a default >> configuration to the various registers. Hence we set active_windows to >> zero and call mixer_cfg_layer(). > > mixer_win_reset function sets registers to default value because mixer device has no reset HW register like vp did. > So original codes set the registers to default value but you removed them like below, > - /* reset default layer priority */ > - mixer_reg_write(res, MXR_LAYER_CFG, 0); > > > - /* disable all layers */ > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); > - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) > - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); > > and called mixer_cfg_layer function which configures overlay registers as previous plane state but not HW default values. OK, let me explain you. The previous code does the following: - write zero to MXR_LAYER_CFG - write zero to the GRP0_ENABLE, GRP1_ENABLE and maybe VP_ENABLE bits of MXR_CFG A brief note about the MXR_LAYER_CFG register. Only the region [11:0] is to be manipulated, bits [31:12] should be left zero. The region [11:0] is the newly introduced mask MXR_LAYER_CFG_MASK. The new code does the following: - set active_windows to zero - issue mixer_cfg_layer(), which now does the following: - initializes local variables mxr_cfg, mxr_layer_cfg, vp_enable - iterates over planes - since no plane is enabled, all three variables above stay zero - zero is written to MXR_CFG with writemask MXR_CFG_ENABLE_MASK - zero is written to MXR_LAYER_CFG with writemask MXR_LAYER_CFG_MASK So in the end, both the old and the new code write exactly the same thing into MXR_CFG and MXR_LAYER_CFG. I hope this clears things up! :-) > With this patch do you know that how many times mixer_cfg_layer function is called when we requested atomic operation? For your typical operation, where mixer_{enable,disable}() is not called, it should now be just once (in mixer_atomic_flush()). With best wishes, Tobias >> With best wishes, >> Tobias >> >> >> >>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>> DRM call that manipulates the primary plane. >>>> >>>> It goes the following route: >>>> - drm_mode_setcrtc >>>> - drm_mode_set_config_internal() >>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>> >>>> Also here: .set_config = drm_atomic_helper_set_config >>>> >>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>> route, so the patch is fine this way. >>>> >>>> @Inki: Can you still queue this up for 4.9.y? >>>> >>>> >>>> With best wishes, >>>> Tobias >>>> >>>> >>>>> >>>>> Regards >>>>> Andrzej >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> >> >>
Hello Inki, Inki Dae wrote: > > > 2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: >> Hey Inki, >> >> >> Inki Dae wrote: >>> >>> >>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>> Hello Andrzej, >>>> >>>> >>>> Andrzej Hajda wrote: >>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>> Hello Inki, >>>>>> >>>>>> >>>>>> Inki Dae wrote: >>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>> in mixer_cfg_layer(). >>>>>>>> Trigger this via atomic flush. >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>> - rename fields as suggested by Andrzej >>>>>>>> - added docu to mixer context struct >>>>>>>> - simplify mixer_win_reset() as well >>>>>>>> >>>>>>>> Changes in v3: >>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>> - fold switch statements into a single one >>>>>>>> >>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>> DRM_FORMAT_NV21, >>>>>>>> }; >>>>>>>> >>>>>>>> +/* >>>>>>>> + * Mixer context structure. >>>>>>>> + * >>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>> + * @pipe: The CRTC index. >>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>> + */ >>>>>>>> struct mixer_context { >>>>>>>> struct platform_device *pdev; >>>>>>>> struct device *dev; >>>>>>>> struct drm_device *drm_dev; >>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>> + unsigned long active_windows; >>>>>>>> int pipe; >>>>>>>> unsigned long flags; >>>>>>>> >>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>> } >>>>>>>> >>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>> - unsigned int priority, bool enable) >>>>>>>> +/** >>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>> + * @ctx: mixer context >>>>>>>> + * >>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>> + * windows. >>>>>>>> + * >>>>>>>> + * Has to be called under mixer lock. >>>>>>>> + */ >>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>> { >>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>> - >>>>>>>> - switch (win) { >>>>>>>> - case 0: >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>> - break; >>>>>>>> - case 1: >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>> + unsigned int win; >>>>>>>> >>>>>>>> - break; >>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>> + struct drm_framebuffer *fb; >>>>>>>> + unsigned int priority; >>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>> + bool enable; >>>>>>>> + >>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>> + fb = state->fb; >>>>>>>> + >>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>> + >>>>>>>> + if (!enable) >>>>>>>> + continue; >>>>>>>> + >>>>>>>> + BUG_ON(!fb); >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>> + */ >>>>>>>> + switch (win) { >>>>>>>> + case 0: >>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>> + break; >>>>>>>> + >>>>>>>> + case 1: >>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>> + break; >>>>>>>> + >>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>> + break; >>>>>>>> } >>>>>>>> - break; >>>>>>>> } >>>>>>>> + >>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>> + >>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>> } >>>>>>>> >>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>> unsigned long flags; >>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>> bool tiled_mode = false; >>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>> >>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>> about that! >>>>> >>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>> It does not seem to be legacy, or to be more precise it calls >>>>> .update_plane and .disable_plane >>>>> callbacks which in exynos are set as follow: >>>>> .update_plane = drm_atomic_helper_update_plane, >>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>> >>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>> also goes through the atomic path. >>>> >>>> The simplified callstack: >>>> - drm_mode_setplane() >>>> - __setplane_internal() >>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>> - update_plane() (same here) >>> >>> Sorry for previous comments not enough. >>> >>> update_plane >>> ... >>> - mixer_update_plane >>> - vp_video_buffer or mixer_graph_buffer >>> >>> However, you removed mixer_cfg_layer call from above functions. >> exactly, because that is the very purpose of this patch. >> >> >>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >> That is not my intention. I want to move register manipulation to the >> atomic flush step. >> >> >>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >> Why do you think so? I think it makes perfect sense to put it there. We >> flush the new configuration to the hardware registers there. >> >> >>> For this I mentioned already at previous my comment, >>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >> You're confusing me. That is exactly what my patch is doing. It moves >> more of the actual hardware register updating to the atomic flush step. > > Ok, for your understanding, you can check atomic_begin, atomic_flush and update_plane callback functions of exynos_drm_fimd.c. > After that I think we could continue to discuss this. I doesn't really want to tackle you. I'm aware of that code, but it's for the FIMD and not the mixer. So I don't see how it is relevant to the discussion. One could probably do the same thing for the FIMD, i.e. moving more code to the flush step. However I lack the hardware to properly test this, so this would be a bit difficult to work on. With best wishes, Tobias >>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >> Why is that? In mixer_win_reset() we want to apply a default >> configuration to the various registers. Hence we set active_windows to >> zero and call mixer_cfg_layer(). >> >> >> With best wishes, >> Tobias >> >> >> >>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>> DRM call that manipulates the primary plane. >>>> >>>> It goes the following route: >>>> - drm_mode_setcrtc >>>> - drm_mode_set_config_internal() >>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>> >>>> Also here: .set_config = drm_atomic_helper_set_config >>>> >>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>> route, so the patch is fine this way. >>>> >>>> @Inki: Can you still queue this up for 4.9.y? >>>> >>>> >>>> With best wishes, >>>> Tobias >>>> >>>> >>>>> >>>>> Regards >>>>> Andrzej >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> >> >>
Hello Inki, Inki Dae wrote: > > > 2016년 09월 28일 09:12에 Tobias Jakobi 이(가) 쓴 글: >> Hello Inki, >> >> >> Inki Dae wrote: >>> >>> >>> 2016년 09월 28일 08:31에 Inki Dae 이(가) 쓴 글: >>>> >>>> >>>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>>> Hello Andrzej, >>>>> >>>>> >>>>> Andrzej Hajda wrote: >>>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>>> Hello Inki, >>>>>>> >>>>>>> >>>>>>> Inki Dae wrote: >>>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>>> in mixer_cfg_layer(). >>>>>>>>> Trigger this via atomic flush. >>>>>>>>> >>>>>>>>> Changes in v2: >>>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>>> - rename fields as suggested by Andrzej >>>>>>>>> - added docu to mixer context struct >>>>>>>>> - simplify mixer_win_reset() as well >>>>>>>>> >>>>>>>>> Changes in v3: >>>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>>> - fold switch statements into a single one >>>>>>>>> >>>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>>> DRM_FORMAT_NV21, >>>>>>>>> }; >>>>>>>>> >>>>>>>>> +/* >>>>>>>>> + * Mixer context structure. >>>>>>>>> + * >>>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>>> + * @pipe: The CRTC index. >>>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>>> + */ >>>>>>>>> struct mixer_context { >>>>>>>>> struct platform_device *pdev; >>>>>>>>> struct device *dev; >>>>>>>>> struct drm_device *drm_dev; >>>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>>> + unsigned long active_windows; >>>>>>>>> int pipe; >>>>>>>>> unsigned long flags; >>>>>>>>> >>>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>>> } >>>>>>>>> >>>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>>> - unsigned int priority, bool enable) >>>>>>>>> +/** >>>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>>> + * @ctx: mixer context >>>>>>>>> + * >>>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>>> + * windows. >>>>>>>>> + * >>>>>>>>> + * Has to be called under mixer lock. >>>>>>>>> + */ >>>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>>> { >>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>>> - >>>>>>>>> - switch (win) { >>>>>>>>> - case 0: >>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>>> - break; >>>>>>>>> - case 1: >>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>>> + unsigned int win; >>>>>>>>> >>>>>>>>> - break; >>>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>>> + struct drm_framebuffer *fb; >>>>>>>>> + unsigned int priority; >>>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>>> + bool enable; >>>>>>>>> + >>>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>>> + fb = state->fb; >>>>>>>>> + >>>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>>> + >>>>>>>>> + if (!enable) >>>>>>>>> + continue; >>>>>>>>> + >>>>>>>>> + BUG_ON(!fb); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>>> + */ >>>>>>>>> + switch (win) { >>>>>>>>> + case 0: >>>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>> + break; >>>>>>>>> + >>>>>>>>> + case 1: >>>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>> + break; >>>>>>>>> + >>>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>>> + break; >>>>>>>>> } >>>>>>>>> - break; >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>>> + >>>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>>> } >>>>>>>>> >>>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>>> unsigned long flags; >>>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>>> bool tiled_mode = false; >>>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>> >>>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>>> about that! >>>>>> >>>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>>> It does not seem to be legacy, or to be more precise it calls >>>>>> .update_plane and .disable_plane >>>>>> callbacks which in exynos are set as follow: >>>>>> .update_plane = drm_atomic_helper_update_plane, >>>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>>> >>>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>>> also goes through the atomic path. >>>>> >>>>> The simplified callstack: >>>>> - drm_mode_setplane() >>>>> - __setplane_internal() >>>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>>> - update_plane() (same here) >>>> >>>> Sorry for previous comments not enough. >>>> >>>> update_plane >>>> ... >>>> - mixer_update_plane >>>> - vp_video_buffer or mixer_graph_buffer >>>> >>>> However, you removed mixer_cfg_layer call from above functions. >>>> >>>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >>>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >>>> For this I mentioned already at previous my comment, >>>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >>>> >>>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >>>> >>> >>> How about calling mixer_cfg_layer function just in mixer_update_plane? Seems more reasonable becasue now mixer_cfg_layer handles both of them - video and grphis layers. >> An example of a atomic operation: >> mixer_atomic_check() >> mixer_atomic_begin() >> mixer_update_plane() >> mixer_disable_plane() >> mixer_update_plane() >> mixer_atomic_flush() >> >> Flushing to hardware registers, i.e. writing MXR_CFG and MXR_LAYER_CFG >> should happen _once_ in mixer_atomic_flush(). > > Please check my comments of previous email. you need to check relevant callback functions of exynos_drm_fimd.c. this patch only changes mixer code, it doesn't introduce changes to the FIMD. Hence I don't see how this comment is helpful to me. With best wishes, Tobias >> With best wishes, >> Tobias >> >> P.S.: While this is only the first step, I also plan to move >> mixer_cfg_scan() and mixer_cfg_rgb_fmt() to mixer_atomic_flush(). >> >> >>> >>>>> >>>>> >>>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>>> DRM call that manipulates the primary plane. >>>>> >>>>> It goes the following route: >>>>> - drm_mode_setcrtc >>>>> - drm_mode_set_config_internal() >>>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>>> >>>>> Also here: .set_config = drm_atomic_helper_set_config >>>>> >>>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>>> route, so the patch is fine this way. >>>>> >>>>> @Inki: Can you still queue this up for 4.9.y? >>>>> >>>>> >>>>> With best wishes, >>>>> Tobias >>>>> >>>>> >>>>>> >>>>>> Regards >>>>>> Andrzej >>>>>> >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>>> >>>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >>
2016-09-28 18:06 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>: > Hello Inki, > > > Inki Dae wrote: >> >> >> 2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: >>> Hey Inki, >>> >>> >>> Inki Dae wrote: >>>> >>>> >>>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>>> Hello Andrzej, >>>>> >>>>> >>>>> Andrzej Hajda wrote: >>>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>>> Hello Inki, >>>>>>> >>>>>>> >>>>>>> Inki Dae wrote: >>>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>>> in mixer_cfg_layer(). >>>>>>>>> Trigger this via atomic flush. >>>>>>>>> >>>>>>>>> Changes in v2: >>>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>>> - rename fields as suggested by Andrzej >>>>>>>>> - added docu to mixer context struct >>>>>>>>> - simplify mixer_win_reset() as well >>>>>>>>> >>>>>>>>> Changes in v3: >>>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>>> - fold switch statements into a single one >>>>>>>>> >>>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>>> DRM_FORMAT_NV21, >>>>>>>>> }; >>>>>>>>> >>>>>>>>> +/* >>>>>>>>> + * Mixer context structure. >>>>>>>>> + * >>>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>>> + * @pipe: The CRTC index. >>>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>>> + */ >>>>>>>>> struct mixer_context { >>>>>>>>> struct platform_device *pdev; >>>>>>>>> struct device *dev; >>>>>>>>> struct drm_device *drm_dev; >>>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>>> + unsigned long active_windows; >>>>>>>>> int pipe; >>>>>>>>> unsigned long flags; >>>>>>>>> >>>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>>> } >>>>>>>>> >>>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>>> - unsigned int priority, bool enable) >>>>>>>>> +/** >>>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>>> + * @ctx: mixer context >>>>>>>>> + * >>>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>>> + * windows. >>>>>>>>> + * >>>>>>>>> + * Has to be called under mixer lock. >>>>>>>>> + */ >>>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>>> { >>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>>> - >>>>>>>>> - switch (win) { >>>>>>>>> - case 0: >>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>>> - break; >>>>>>>>> - case 1: >>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>>> + unsigned int win; >>>>>>>>> >>>>>>>>> - break; >>>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>>> + struct drm_framebuffer *fb; >>>>>>>>> + unsigned int priority; >>>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>>> + bool enable; >>>>>>>>> + >>>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>>> + fb = state->fb; >>>>>>>>> + >>>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>>> + >>>>>>>>> + if (!enable) >>>>>>>>> + continue; >>>>>>>>> + >>>>>>>>> + BUG_ON(!fb); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>>> + */ >>>>>>>>> + switch (win) { >>>>>>>>> + case 0: >>>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>> + break; >>>>>>>>> + >>>>>>>>> + case 1: >>>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>> + break; >>>>>>>>> + >>>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>>> + break; >>>>>>>>> } >>>>>>>>> - break; >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>>> + >>>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>>> } >>>>>>>>> >>>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>>> unsigned long flags; >>>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>>> bool tiled_mode = false; >>>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>> >>>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>>> about that! >>>>>> >>>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>>> It does not seem to be legacy, or to be more precise it calls >>>>>> .update_plane and .disable_plane >>>>>> callbacks which in exynos are set as follow: >>>>>> .update_plane = drm_atomic_helper_update_plane, >>>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>>> >>>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>>> also goes through the atomic path. >>>>> >>>>> The simplified callstack: >>>>> - drm_mode_setplane() >>>>> - __setplane_internal() >>>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>>> - update_plane() (same here) >>>> >>>> Sorry for previous comments not enough. >>>> >>>> update_plane >>>> ... >>>> - mixer_update_plane >>>> - vp_video_buffer or mixer_graph_buffer >>>> >>>> However, you removed mixer_cfg_layer call from above functions. >>> exactly, because that is the very purpose of this patch. >>> >>> >>>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >>> That is not my intention. I want to move register manipulation to the >>> atomic flush step. >>> >>> >>>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >>> Why do you think so? I think it makes perfect sense to put it there. We >>> flush the new configuration to the hardware registers there. >>> >>> >>>> For this I mentioned already at previous my comment, >>>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >>> You're confusing me. That is exactly what my patch is doing. It moves >>> more of the actual hardware register updating to the atomic flush step. >> >> Ok, for your understanding, you can check atomic_begin, atomic_flush and update_plane callback functions of exynos_drm_fimd.c. >> After that I think we could continue to discuss this. I doesn't really want to tackle you. > I'm aware of that code, but it's for the FIMD and not the mixer. So I > don't see how it is relevant to the discussion. One could probably do > the same thing for the FIMD, i.e. moving more code to the flush step. > However I lack the hardware to properly test this, so this would be a > bit difficult to work on. First, check below descriptions for atomic_begin and atomic_flush callbacks, "@atomic_begin: ... Depending upon hardware this might be vblank evasion, blocking updates by setting bits or doing preparatory work for e.g. manual update display. @atomic_flush: ... Depending upon hardware this might include checking that vblank evasion was successful, unblocking updates by setting bits or setting the GO bit to flush out all updates." And for this Mixer device has similar registers to FIMD ones. See the below codes, static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable) { struct mixer_resources *res = &ctx->mixer_res; /* block update on vsync */ mixer_reg_writemask(res, MXR_STATUS, enable ? MXR_STATUS_SYNC_ENABLE : 0, MXR_STATUS_SYNC_ENABLE); if (ctx->vp_enabled) vp_reg_write(res, VP_SHADOW_UPDATE, enable ? VP_SHADOW_UPDATE_ENABLE : 0); } According to the second argument of above function - enable value -, this function will do something required by atomic_begin or atomic_flush callback. And this function is already called by mixer_atomic_begin and mixer_atomic_flush functions properly. You are trying to really spread out register setting codes here and there with this patch. Ps. most crtc devices have special registers which block setting values to be updated to real hardware after vsync completion to make sure for all register setting values to be updated to real hardware when they are really ready. So atomic_begin and atomic_flush callbacks are used for such purpose. Thanks, Inki Dae > > > With best wishes, > Tobias > > >>>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >>> Why is that? In mixer_win_reset() we want to apply a default >>> configuration to the various registers. Hence we set active_windows to >>> zero and call mixer_cfg_layer(). >>> >>> >>> With best wishes, >>> Tobias >>> >>> >>> >>>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>>> DRM call that manipulates the primary plane. >>>>> >>>>> It goes the following route: >>>>> - drm_mode_setcrtc >>>>> - drm_mode_set_config_internal() >>>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>>> >>>>> Also here: .set_config = drm_atomic_helper_set_config >>>>> >>>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>>> route, so the patch is fine this way. >>>>> >>>>> @Inki: Can you still queue this up for 4.9.y? >>>>> >>>>> >>>>> With best wishes, >>>>> Tobias >>>>> >>>>> >>>>>> >>>>>> Regards >>>>>> Andrzej >>>>>> >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>>> >>>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>> >>> >>> >>> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Inki Dae wrote: > 2016-09-28 18:06 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>: >> Hello Inki, >> >> >> Inki Dae wrote: >>> >>> >>> 2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: >>>> Hey Inki, >>>> >>>> >>>> Inki Dae wrote: >>>>> >>>>> >>>>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>>>> Hello Andrzej, >>>>>> >>>>>> >>>>>> Andrzej Hajda wrote: >>>>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>>>> Hello Inki, >>>>>>>> >>>>>>>> >>>>>>>> Inki Dae wrote: >>>>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>>>> in mixer_cfg_layer(). >>>>>>>>>> Trigger this via atomic flush. >>>>>>>>>> >>>>>>>>>> Changes in v2: >>>>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>>>> - rename fields as suggested by Andrzej >>>>>>>>>> - added docu to mixer context struct >>>>>>>>>> - simplify mixer_win_reset() as well >>>>>>>>>> >>>>>>>>>> Changes in v3: >>>>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>>>> - fold switch statements into a single one >>>>>>>>>> >>>>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>>>> DRM_FORMAT_NV21, >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> +/* >>>>>>>>>> + * Mixer context structure. >>>>>>>>>> + * >>>>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>>>> + * @pipe: The CRTC index. >>>>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>>>> + */ >>>>>>>>>> struct mixer_context { >>>>>>>>>> struct platform_device *pdev; >>>>>>>>>> struct device *dev; >>>>>>>>>> struct drm_device *drm_dev; >>>>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>>>> + unsigned long active_windows; >>>>>>>>>> int pipe; >>>>>>>>>> unsigned long flags; >>>>>>>>>> >>>>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>>>> - unsigned int priority, bool enable) >>>>>>>>>> +/** >>>>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>>>> + * @ctx: mixer context >>>>>>>>>> + * >>>>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>>>> + * windows. >>>>>>>>>> + * >>>>>>>>>> + * Has to be called under mixer lock. >>>>>>>>>> + */ >>>>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>>>> { >>>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>>>> - >>>>>>>>>> - switch (win) { >>>>>>>>>> - case 0: >>>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>>>> - break; >>>>>>>>>> - case 1: >>>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>>>> + unsigned int win; >>>>>>>>>> >>>>>>>>>> - break; >>>>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>>>> + struct drm_framebuffer *fb; >>>>>>>>>> + unsigned int priority; >>>>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>>>> + bool enable; >>>>>>>>>> + >>>>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>>>> + fb = state->fb; >>>>>>>>>> + >>>>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>>>> + >>>>>>>>>> + if (!enable) >>>>>>>>>> + continue; >>>>>>>>>> + >>>>>>>>>> + BUG_ON(!fb); >>>>>>>>>> + >>>>>>>>>> + /* >>>>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>>>> + */ >>>>>>>>>> + switch (win) { >>>>>>>>>> + case 0: >>>>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>>> + break; >>>>>>>>>> + >>>>>>>>>> + case 1: >>>>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>>> + break; >>>>>>>>>> + >>>>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>>>> + break; >>>>>>>>>> } >>>>>>>>>> - break; >>>>>>>>>> } >>>>>>>>>> + >>>>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>>>> + >>>>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>>>> unsigned long flags; >>>>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>>>> bool tiled_mode = false; >>>>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>>> >>>>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>>>> about that! >>>>>>> >>>>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>>>> It does not seem to be legacy, or to be more precise it calls >>>>>>> .update_plane and .disable_plane >>>>>>> callbacks which in exynos are set as follow: >>>>>>> .update_plane = drm_atomic_helper_update_plane, >>>>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>>>> >>>>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>>>> also goes through the atomic path. >>>>>> >>>>>> The simplified callstack: >>>>>> - drm_mode_setplane() >>>>>> - __setplane_internal() >>>>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>>>> - update_plane() (same here) >>>>> >>>>> Sorry for previous comments not enough. >>>>> >>>>> update_plane >>>>> ... >>>>> - mixer_update_plane >>>>> - vp_video_buffer or mixer_graph_buffer >>>>> >>>>> However, you removed mixer_cfg_layer call from above functions. >>>> exactly, because that is the very purpose of this patch. >>>> >>>> >>>>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >>>> That is not my intention. I want to move register manipulation to the >>>> atomic flush step. >>>> >>>> >>>>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >>>> Why do you think so? I think it makes perfect sense to put it there. We >>>> flush the new configuration to the hardware registers there. >>>> >>>> >>>>> For this I mentioned already at previous my comment, >>>>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >>>> You're confusing me. That is exactly what my patch is doing. It moves >>>> more of the actual hardware register updating to the atomic flush step. >>> >>> Ok, for your understanding, you can check atomic_begin, atomic_flush and update_plane callback functions of exynos_drm_fimd.c. >>> After that I think we could continue to discuss this. I doesn't really want to tackle you. >> I'm aware of that code, but it's for the FIMD and not the mixer. So I >> don't see how it is relevant to the discussion. One could probably do >> the same thing for the FIMD, i.e. moving more code to the flush step. >> However I lack the hardware to properly test this, so this would be a >> bit difficult to work on. > > First, check below descriptions for atomic_begin and atomic_flush callbacks, > > "@atomic_begin: > ... > Depending upon hardware this might be vblank > evasion, blocking updates by setting bits or doing preparatory work > for e.g. manual update display. > > > @atomic_flush: > ... > Depending upon hardware this might include > checking that vblank evasion was successful, unblocking updates by > setting bits or setting the GO bit to flush out all updates." > > And for this Mixer device has similar registers to FIMD ones. See the > below codes, > > static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable) > { > struct mixer_resources *res = &ctx->mixer_res; > /* block update on vsync */ > mixer_reg_writemask(res, MXR_STATUS, enable ? > MXR_STATUS_SYNC_ENABLE : 0, MXR_STATUS_SYNC_ENABLE); > if (ctx->vp_enabled) > vp_reg_write(res, VP_SHADOW_UPDATE, enable ? > VP_SHADOW_UPDATE_ENABLE : 0); > } > > According to the second argument of above function - enable value -, > this function will do something required by atomic_begin or > atomic_flush callback. > And this function is already called by mixer_atomic_begin and > mixer_atomic_flush functions properly. I'm aware how the mixer operates. However my code doesn't modify manipulation of the MXR_STATUS register, update blocking still works the same as before. > You are trying to really spread out register setting codes here and > there with this patch. > Uhm, no? The contrary is true, I consolidate register manipulation. Are you sure you're reading my patch correctly? @Andrzej: Maybe you can comment on this? > Ps. most crtc devices have special registers which block setting > values to be updated to real hardware after vsync completion to make > sure for all register setting values to be updated to real hardware > when they are really ready. So atomic_begin and atomic_flush callbacks > are used for such purpose. I'm also aware of this. But this is orthogonal to what my patch want to achieve. With best wishes, Tobias > > Thanks, > Inki Dae > >> >> >> With best wishes, >> Tobias >> >> >>>>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >>>> Why is that? In mixer_win_reset() we want to apply a default >>>> configuration to the various registers. Hence we set active_windows to >>>> zero and call mixer_cfg_layer(). >>>> >>>> >>>> With best wishes, >>>> Tobias >>>> >>>> >>>> >>>>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>>>> DRM call that manipulates the primary plane. >>>>>> >>>>>> It goes the following route: >>>>>> - drm_mode_setcrtc >>>>>> - drm_mode_set_config_internal() >>>>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>>>> >>>>>> Also here: .set_config = drm_atomic_helper_set_config >>>>>> >>>>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>>>> route, so the patch is fine this way. >>>>>> >>>>>> @Inki: Can you still queue this up for 4.9.y? >>>>>> >>>>>> >>>>>> With best wishes, >>>>>> Tobias >>>>>> >>>>>> >>>>>>> >>>>>>> Regards >>>>>>> Andrzej >>>>>>> >>>>>> >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>>>> the body of a message to majordomo@vger.kernel.org >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>> >>>>>> >>>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>> >>>> >>>> >>>> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
2016년 09월 29일 01:28에 Tobias Jakobi 이(가) 쓴 글: > Inki Dae wrote: >> 2016-09-28 18:06 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>: >>> Hello Inki, >>> >>> >>> Inki Dae wrote: >>>> >>>> >>>> 2016년 09월 28일 09:03에 Tobias Jakobi 이(가) 쓴 글: >>>>> Hey Inki, >>>>> >>>>> >>>>> Inki Dae wrote: >>>>>> >>>>>> >>>>>> 2016년 09월 28일 01:52에 Tobias Jakobi 이(가) 쓴 글: >>>>>>> Hello Andrzej, >>>>>>> >>>>>>> >>>>>>> Andrzej Hajda wrote: >>>>>>>> On 27.09.2016 13:22, Tobias Jakobi wrote: >>>>>>>>> Hello Inki, >>>>>>>>> >>>>>>>>> >>>>>>>>> Inki Dae wrote: >>>>>>>>>> 2016년 09월 26일 20:36에 Tobias Jakobi 이(가) 쓴 글: >>>>>>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once >>>>>>>>>>> in mixer_cfg_layer(). >>>>>>>>>>> Trigger this via atomic flush. >>>>>>>>>>> >>>>>>>>>>> Changes in v2: >>>>>>>>>>> - issue mixer_cfg_layer() in mixer_disable() >>>>>>>>>>> - rename fields as suggested by Andrzej >>>>>>>>>>> - added docu to mixer context struct >>>>>>>>>>> - simplify mixer_win_reset() as well >>>>>>>>>>> >>>>>>>>>>> Changes in v3: >>>>>>>>>>> - simplify some conditions as suggested by Inki >>>>>>>>>>> - add docu to mixer_cfg_layer() >>>>>>>>>>> - fold switch statements into a single one >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>>>>>>> --- >>>>>>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ >>>>>>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 + >>>>>>>>>>> 2 files changed, 92 insertions(+), 45 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>>>> index 1e78d57..4f06f4d 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>>>>>>>> @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { >>>>>>>>>>> DRM_FORMAT_NV21, >>>>>>>>>>> }; >>>>>>>>>>> >>>>>>>>>>> +/* >>>>>>>>>>> + * Mixer context structure. >>>>>>>>>>> + * >>>>>>>>>>> + * @crtc: The HDMI CRTC attached to the mixer. >>>>>>>>>>> + * @planes: Array of plane objects for each of the mixer windows. >>>>>>>>>>> + * @active_windows: Cache of the mixer's hardware state. >>>>>>>>>>> + * Tracks which mixer windows are active/inactive. >>>>>>>>>>> + * @pipe: The CRTC index. >>>>>>>>>>> + * @flags: Bitfield build from the mixer_flag_bits enumerator. >>>>>>>>>>> + * @mixer_resources: A struct containing registers, clocks, etc. >>>>>>>>>>> + * @mxr_ver: The hardware revision/version of the mixer. >>>>>>>>>>> + */ >>>>>>>>>>> struct mixer_context { >>>>>>>>>>> struct platform_device *pdev; >>>>>>>>>>> struct device *dev; >>>>>>>>>>> struct drm_device *drm_dev; >>>>>>>>>>> struct exynos_drm_crtc *crtc; >>>>>>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR]; >>>>>>>>>>> + unsigned long active_windows; >>>>>>>>>>> int pipe; >>>>>>>>>>> unsigned long flags; >>>>>>>>>>> >>>>>>>>>>> @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) >>>>>>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, >>>>>>>>>>> - unsigned int priority, bool enable) >>>>>>>>>>> +/** >>>>>>>>>>> + * mixer_cfg_layer - apply layer configuration to hardware >>>>>>>>>>> + * @ctx: mixer context >>>>>>>>>>> + * >>>>>>>>>>> + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers >>>>>>>>>>> + * using the 'active_windows' field of the the mixer content, and >>>>>>>>>>> + * the pixel format of the framebuffers associated with the enabled >>>>>>>>>>> + * windows. >>>>>>>>>>> + * >>>>>>>>>>> + * Has to be called under mixer lock. >>>>>>>>>>> + */ >>>>>>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx) >>>>>>>>>>> { >>>>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>>>> - u32 val = enable ? ~0 : 0; >>>>>>>>>>> - >>>>>>>>>>> - switch (win) { >>>>>>>>>>> - case 0: >>>>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); >>>>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority), >>>>>>>>>>> - MXR_LAYER_CFG_GRP0_MASK); >>>>>>>>>>> - break; >>>>>>>>>>> - case 1: >>>>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); >>>>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority), >>>>>>>>>>> - MXR_LAYER_CFG_GRP1_MASK); >>>>>>>>>>> + unsigned int win; >>>>>>>>>>> >>>>>>>>>>> - break; >>>>>>>>>>> - case VP_DEFAULT_WIN: >>>>>>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { >>>>>>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); >>>>>>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, >>>>>>>>>>> - MXR_CFG_VP_ENABLE); >>>>>>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG, >>>>>>>>>>> - MXR_LAYER_CFG_VP_VAL(priority), >>>>>>>>>>> - MXR_LAYER_CFG_VP_MASK); >>>>>>>>>>> + struct exynos_drm_plane_state *state; >>>>>>>>>>> + struct drm_framebuffer *fb; >>>>>>>>>>> + unsigned int priority; >>>>>>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; >>>>>>>>>>> + bool enable; >>>>>>>>>>> + >>>>>>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) { >>>>>>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state); >>>>>>>>>>> + fb = state->fb; >>>>>>>>>>> + >>>>>>>>>>> + priority = state->base.normalized_zpos + 1; >>>>>>>>>>> + enable = test_bit(win, &ctx->active_windows); >>>>>>>>>>> + >>>>>>>>>>> + if (!enable) >>>>>>>>>>> + continue; >>>>>>>>>>> + >>>>>>>>>>> + BUG_ON(!fb); >>>>>>>>>>> + >>>>>>>>>>> + /* >>>>>>>>>>> + * TODO: Don't enable alpha blending for the bottom window. >>>>>>>>>>> + */ >>>>>>>>>>> + switch (win) { >>>>>>>>>>> + case 0: >>>>>>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE; >>>>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); >>>>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case 1: >>>>>>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE; >>>>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); >>>>>>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case VP_DEFAULT_WIN: >>>>>>>>>>> + vp_enable = VP_ENABLE_ON; >>>>>>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE; >>>>>>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); >>>>>>>>>>> + mixer_cfg_vp_blend(ctx); >>>>>>>>>>> + break; >>>>>>>>>>> } >>>>>>>>>>> - break; >>>>>>>>>>> } >>>>>>>>>>> + >>>>>>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) >>>>>>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); >>>>>>>>>>> + >>>>>>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); >>>>>>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> static void mixer_run(struct mixer_context *ctx) >>>>>>>>>>> @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>>>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; >>>>>>>>>>> struct mixer_resources *res = &ctx->mixer_res; >>>>>>>>>>> struct drm_framebuffer *fb = state->fb; >>>>>>>>>>> - unsigned int priority = state->base.normalized_zpos + 1; >>>>>>>>>>> unsigned long flags; >>>>>>>>>>> dma_addr_t luma_addr[2], chroma_addr[2]; >>>>>>>>>>> bool tiled_mode = false; >>>>>>>>>>> @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>>>>>>>> >>>>>>>>>>> mixer_cfg_scan(ctx, mode->vdisplay); >>>>>>>>>>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay); >>>>>>>>>>> - mixer_cfg_layer(ctx, plane->index, priority, true); >>>>>>>>>>> - mixer_cfg_vp_blend(ctx); >>>>>>>>>> You removed the function call to set MXR_CFG and MXR_LAYER_CFG registers so what if lagacy set plane ioctl is called by user user space? Where these register are updated? >>>>>>>>> yes, you're right. I completly forgot about the legacy codepaths. Sorry >>>>>>>>> about that! >>>>>>>> >>>>>>>> Are you talking about DRM_IOCTL_MODE_SETPLANE? >>>>>>>> It does not seem to be legacy, or to be more precise it calls >>>>>>>> .update_plane and .disable_plane >>>>>>>> callbacks which in exynos are set as follow: >>>>>>>> .update_plane = drm_atomic_helper_update_plane, >>>>>>>> .disable_plane = drm_atomic_helper_disable_plane, >>>>>>>> >>>>>>>> So DRM_IOCTL_MODE_SETPLANE calls atomic codepath anyway. >>>>>>> thanks a lot for the heads-up. I can confirm that drm_mode_setplane() >>>>>>> also goes through the atomic path. >>>>>>> >>>>>>> The simplified callstack: >>>>>>> - drm_mode_setplane() >>>>>>> - __setplane_internal() >>>>>>> - disable_plane() (set in plane_helper_funcs in exynos_drm_plane.c) >>>>>>> - update_plane() (same here) >>>>>> >>>>>> Sorry for previous comments not enough. >>>>>> >>>>>> update_plane >>>>>> ... >>>>>> - mixer_update_plane >>>>>> - vp_video_buffer or mixer_graph_buffer >>>>>> >>>>>> However, you removed mixer_cfg_layer call from above functions. >>>>> exactly, because that is the very purpose of this patch. >>>>> >>>>> >>>>>> So my intention was to keep the modifed mixer_cfg_layer call on vp_video_buffer and mixer_grpah_buffer functions. >>>>> That is not my intention. I want to move register manipulation to the >>>>> atomic flush step. >>>>> >>>>> >>>>>> mixer_atomic_flush function is not really proper place to call mixer_cfg_layer function even mixer_win_reset. >>>>> Why do you think so? I think it makes perfect sense to put it there. We >>>>> flush the new configuration to the hardware registers there. >>>>> >>>>> >>>>>> For this I mentioned already at previous my comment, >>>>>> "atomic_flush callback will be used to make sure registers to be updated to the real hardware. So calling mixer_cfg_layer function here wouldn't be reasonable." >>>>> You're confusing me. That is exactly what my patch is doing. It moves >>>>> more of the actual hardware register updating to the atomic flush step. >>>> >>>> Ok, for your understanding, you can check atomic_begin, atomic_flush and update_plane callback functions of exynos_drm_fimd.c. >>>> After that I think we could continue to discuss this. I doesn't really want to tackle you. >>> I'm aware of that code, but it's for the FIMD and not the mixer. So I >>> don't see how it is relevant to the discussion. One could probably do >>> the same thing for the FIMD, i.e. moving more code to the flush step. >>> However I lack the hardware to properly test this, so this would be a >>> bit difficult to work on. >> >> First, check below descriptions for atomic_begin and atomic_flush callbacks, >> >> "@atomic_begin: >> ... >> Depending upon hardware this might be vblank >> evasion, blocking updates by setting bits or doing preparatory work >> for e.g. manual update display. >> >> >> @atomic_flush: >> ... >> Depending upon hardware this might include >> checking that vblank evasion was successful, unblocking updates by >> setting bits or setting the GO bit to flush out all updates." >> >> And for this Mixer device has similar registers to FIMD ones. See the >> below codes, >> >> static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable) >> { >> struct mixer_resources *res = &ctx->mixer_res; >> /* block update on vsync */ >> mixer_reg_writemask(res, MXR_STATUS, enable ? >> MXR_STATUS_SYNC_ENABLE : 0, MXR_STATUS_SYNC_ENABLE); >> if (ctx->vp_enabled) >> vp_reg_write(res, VP_SHADOW_UPDATE, enable ? >> VP_SHADOW_UPDATE_ENABLE : 0); >> } >> >> According to the second argument of above function - enable value -, >> this function will do something required by atomic_begin or >> atomic_flush callback. >> And this function is already called by mixer_atomic_begin and >> mixer_atomic_flush functions properly. > I'm aware how the mixer operates. However my code doesn't modify > manipulation of the MXR_STATUS register, update blocking still works the > same as before. My comment is really simple. Just DO NOT other setting here, atomic_flush callback. So NAK. > > >> You are trying to really spread out register setting codes here and >> there with this patch. >> > Uhm, no? The contrary is true, I consolidate register manipulation. Are > you sure you're reading my patch correctly? You are spreading out. > > @Andrzej: Maybe you can comment on this? > > >> Ps. most crtc devices have special registers which block setting >> values to be updated to real hardware after vsync completion to make >> sure for all register setting values to be updated to real hardware >> when they are really ready. So atomic_begin and atomic_flush callbacks >> are used for such purpose. > I'm also aware of this. But this is orthogonal to what my patch want to > achieve. > > > With best wishes, > Tobias > > > >> >> Thanks, >> Inki Dae >> >>> >>> >>> With best wishes, >>> Tobias >>> >>> >>>>>> Also it would be reasonable to remove mixer_cfg_layer function call from mixer_win_reset function. >>>>> Why is that? In mixer_win_reset() we want to apply a default >>>>> configuration to the various registers. Hence we set active_windows to >>>>> zero and call mixer_cfg_layer(). >>>>> >>>>> >>>>> With best wishes, >>>>> Tobias >>>>> >>>>> >>>>> >>>>>>> For completeness I've also checked drm_mode_setcrtc(), the other legacy >>>>>>> DRM call that manipulates the primary plane. >>>>>>> >>>>>>> It goes the following route: >>>>>>> - drm_mode_setcrtc >>>>>>> - drm_mode_set_config_internal() >>>>>>> - set_config() (set in exynos_crtc_funcs in exynos_drm_crtc.c) >>>>>>> >>>>>>> Also here: .set_config = drm_atomic_helper_set_config >>>>>>> >>>>>>> Both DRM_IOCTL_MODE_SETPLANE and DRM_IOCTL_MODE_CRTC go the atomic >>>>>>> route, so the patch is fine this way. >>>>>>> >>>>>>> @Inki: Can you still queue this up for 4.9.y? >>>>>>> >>>>>>> >>>>>>> With best wishes, >>>>>>> Tobias >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Regards >>>>>>>> Andrzej >>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>>>>> the body of a message to majordomo@vger.kernel.org >>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>>> >>>>>>> >>>>>>> >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>>>> the body of a message to majordomo@vger.kernel.org >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>> >>>>> >>>>> >>>>> >>>>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..4f06f4d 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -93,12 +93,25 @@ static const uint32_t vp_formats[] = { DRM_FORMAT_NV21, }; +/* + * Mixer context structure. + * + * @crtc: The HDMI CRTC attached to the mixer. + * @planes: Array of plane objects for each of the mixer windows. + * @active_windows: Cache of the mixer's hardware state. + * Tracks which mixer windows are active/inactive. + * @pipe: The CRTC index. + * @flags: Bitfield build from the mixer_flag_bits enumerator. + * @mixer_resources: A struct containing registers, clocks, etc. + * @mxr_ver: The hardware revision/version of the mixer. + */ struct mixer_context { struct platform_device *pdev; struct device *dev; struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR]; + unsigned long active_windows; int pipe; unsigned long flags; @@ -418,37 +431,70 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); } -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, - unsigned int priority, bool enable) +/** + * mixer_cfg_layer - apply layer configuration to hardware + * @ctx: mixer context + * + * This configures the MXR_CFG and MXR_LAYER_CFG hardware registers + * using the 'active_windows' field of the the mixer content, and + * the pixel format of the framebuffers associated with the enabled + * windows. + * + * Has to be called under mixer lock. + */ +static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res; - u32 val = enable ? ~0 : 0; - - switch (win) { - case 0: - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); - mixer_reg_writemask(res, MXR_LAYER_CFG, - MXR_LAYER_CFG_GRP0_VAL(priority), - MXR_LAYER_CFG_GRP0_MASK); - break; - case 1: - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); - mixer_reg_writemask(res, MXR_LAYER_CFG, - MXR_LAYER_CFG_GRP1_VAL(priority), - MXR_LAYER_CFG_GRP1_MASK); + unsigned int win; - break; - case VP_DEFAULT_WIN: - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); - mixer_reg_writemask(res, MXR_CFG, val, - MXR_CFG_VP_ENABLE); - mixer_reg_writemask(res, MXR_LAYER_CFG, - MXR_LAYER_CFG_VP_VAL(priority), - MXR_LAYER_CFG_VP_MASK); + struct exynos_drm_plane_state *state; + struct drm_framebuffer *fb; + unsigned int priority; + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; + bool enable; + + for (win = 0; win < MIXER_WIN_NR; ++win) { + state = to_exynos_plane_state(ctx->planes[win].base.state); + fb = state->fb; + + priority = state->base.normalized_zpos + 1; + enable = test_bit(win, &ctx->active_windows); + + if (!enable) + continue; + + BUG_ON(!fb); + + /* + * TODO: Don't enable alpha blending for the bottom window. + */ + switch (win) { + case 0: + mxr_cfg |= MXR_CFG_GRP0_ENABLE; + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); + break; + + case 1: + mxr_cfg |= MXR_CFG_GRP1_ENABLE; + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); + break; + + case VP_DEFAULT_WIN: + vp_enable = VP_ENABLE_ON; + mxr_cfg |= MXR_CFG_VP_ENABLE; + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); + mixer_cfg_vp_blend(ctx); + break; } - break; } + + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); + + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); } static void mixer_run(struct mixer_context *ctx) @@ -478,7 +524,6 @@ static void vp_video_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb; - unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; dma_addr_t luma_addr[2], chroma_addr[2]; bool tiled_mode = false; @@ -563,8 +608,6 @@ static void vp_video_buffer(struct mixer_context *ctx, mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay); - mixer_cfg_layer(ctx, plane->index, priority, true); - mixer_cfg_vp_blend(ctx); mixer_run(ctx); spin_unlock_irqrestore(&res->reg_slock, flags); @@ -588,7 +631,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb; - unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; unsigned int win = plane->index; unsigned int x_ratio = 0, y_ratio = 0; @@ -680,8 +722,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay); - mixer_cfg_layer(ctx, win, priority, true); - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); /* layer update mandatory for mixer 16.0.33.0 */ if (ctx->mxr_ver == MXR_VER_16_0_33_0 || @@ -726,9 +766,6 @@ static void mixer_win_reset(struct mixer_context *ctx) mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST, MXR_STATUS_BURST_MASK); - /* reset default layer priority */ - mixer_reg_write(res, MXR_LAYER_CFG, 0); - /* setting background color */ mixer_reg_write(res, MXR_BG_COLOR0, 0x008080); mixer_reg_write(res, MXR_BG_COLOR1, 0x008080); @@ -740,11 +777,9 @@ static void mixer_win_reset(struct mixer_context *ctx) vp_default_filter(res); } - /* disable all layers */ - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) - mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); + /* disable all layers and reset layer priority to default */ + ctx->active_windows = 0; + mixer_cfg_layer(ctx); spin_unlock_irqrestore(&res->reg_slock, flags); } @@ -994,32 +1029,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, vp_video_buffer(mixer_ctx, plane); else mixer_graph_buffer(mixer_ctx, plane); + + __set_bit(plane->index, &mixer_ctx->active_windows); } static void mixer_disable_plane(struct exynos_drm_crtc *crtc, struct exynos_drm_plane *plane) { struct mixer_context *mixer_ctx = crtc->ctx; - struct mixer_resources *res = &mixer_ctx->mixer_res; - unsigned long flags; DRM_DEBUG_KMS("win: %d\n", plane->index); if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return; - spin_lock_irqsave(&res->reg_slock, flags); - mixer_cfg_layer(mixer_ctx, plane->index, 0, false); - spin_unlock_irqrestore(&res->reg_slock, flags); + __clear_bit(plane->index, &mixer_ctx->active_windows); } static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) { struct mixer_context *mixer_ctx = crtc->ctx; + struct mixer_resources *res = &mixer_ctx->mixer_res; + unsigned long flags; if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return; + spin_lock_irqsave(&res->reg_slock, flags); + mixer_cfg_layer(mixer_ctx); + spin_unlock_irqrestore(&res->reg_slock, flags); + mixer_vsync_set_update(mixer_ctx, true); } @@ -1053,6 +1092,8 @@ static void mixer_enable(struct exynos_drm_crtc *crtc) static void mixer_disable(struct exynos_drm_crtc *crtc) { struct mixer_context *ctx = crtc->ctx; + struct mixer_resources *res = &ctx->mixer_res; + unsigned long flags; int i; if (!test_bit(MXR_BIT_POWERED, &ctx->flags)) @@ -1064,6 +1105,10 @@ static void mixer_disable(struct exynos_drm_crtc *crtc) for (i = 0; i < MIXER_WIN_NR; i++) mixer_disable_plane(crtc, &ctx->planes[i]); + spin_lock_irqsave(&res->reg_slock, flags); + mixer_cfg_layer(ctx); + spin_unlock_irqrestore(&res->reg_slock, flags); + exynos_drm_pipe_clk_enable(crtc, false); pm_runtime_put(ctx->dev); diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h index 7f22df5..728a18e 100644 --- a/drivers/gpu/drm/exynos/regs-mixer.h +++ b/drivers/gpu/drm/exynos/regs-mixer.h @@ -100,6 +100,7 @@ #define MXR_CFG_GRP1_ENABLE (1 << 5) #define MXR_CFG_GRP0_ENABLE (1 << 4) #define MXR_CFG_VP_ENABLE (1 << 3) +#define MXR_CFG_ENABLE_MASK (0x7 << 3) #define MXR_CFG_SCAN_INTERLACE (0 << 2) #define MXR_CFG_SCAN_PROGRESSIVE (1 << 2) #define MXR_CFG_SCAN_NTSC (0 << 1) @@ -151,6 +152,7 @@ #define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0) #define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0) #define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0) +#define MXR_LAYER_CFG_MASK 0xFFF #endif /* SAMSUNG_REGS_MIXER_H */
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush. Changes in v2: - issue mixer_cfg_layer() in mixer_disable() - rename fields as suggested by Andrzej - added docu to mixer context struct - simplify mixer_win_reset() as well Changes in v3: - simplify some conditions as suggested by Inki - add docu to mixer_cfg_layer() - fold switch statements into a single one Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> --- drivers/gpu/drm/exynos/exynos_mixer.c | 135 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 92 insertions(+), 45 deletions(-)