[PATCH v3] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
Inki Dae
daeinki at gmail.com
Wed Sep 28 16:04:24 UTC 2016
2016-09-28 18:06 GMT+09:00 Tobias Jakobi <tjakobi at 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 at 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 at 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 at vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>>
>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list