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@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)
>>>>
>>>> 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@xxxxxxxxxxxxxxx
>>>>> 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@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
