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@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.
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@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
