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

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux