Re: [PATCH v3] drm/exynos: mixer: configure layers once in mixer_atomic_flush()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




2016년 09월 29일 01:28에 Tobias Jakobi 이(가) 쓴 글:
> Inki Dae wrote:
>> 2016-09-28 18:06 GMT+09:00 Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>:
>>> 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@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.
>>>>
>>>> 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@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
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> 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@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



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux