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@xxxxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
