Re: [PATCH v1 4/4] drm/ast: Disable planes while switching display modes

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 7261 bytes --]

Hi

Am 07.08.20 um 10:50 schrieb daniel@ffwll.ch:
> On Wed, Aug 05, 2020 at 12:54:28PM +0200, Thomas Zimmermann wrote:
>> The ast HW cursor requires the primary plane and CRTC to display at
>> a valid mode and format. This is not the case while switching
>> display modes, which can lead to the screen turing permanently dark.
>>
>> As a workaround, the ast driver now disables active planes while the
>> mode or format switch takes place. It also synchronizes with the vertical
>> refresh to give CRTC and planes some time to catch up on each other.
>> The active planes planes (primary or cursor) will be re-enabled by
>> each plane's atomic_update() function.
>>
>> v2:
>> 	* move the logic into the commit-tail function
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
>> Cc: <stable@vger.kernel.org> # v5.6+
>> ---
>>  drivers/gpu/drm/ast/ast_drv.h  |  2 +
>>  drivers/gpu/drm/ast/ast_mode.c | 68 ++++++++++++++++++++++++++++++++--
>>  2 files changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
>> index c1af6b725933..467049ca8430 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -177,6 +177,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
>>  
>>  #define AST_IO_MM_OFFSET		(0x380)
>>  
>> +#define AST_IO_VGAIR1_VREFRESH		BIT(3)
>> +
>>  #define __ast_read(x) \
>>  static inline u##x ast_read##x(struct ast_private *ast, u32 reg) { \
>>  u##x val = 0;\
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index ae5cb0a333f7..a379d51f3543 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -514,6 +514,17 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
>>  
>>  }
>>  
>> +static void ast_wait_for_vretrace(struct ast_private *ast)
>> +{
>> +	unsigned long timeout = jiffies + HZ;
>> +	u8 vgair1;
>> +
>> +	do {
>> +		vgair1 = ast_io_read8(ast, AST_IO_INPUT_STATUS1_READ);
>> +	} while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) &&
>> +		 time_before(jiffies, timeout));
>> +}
>> +
>>  /*
>>   * Primary plane
>>   */
>> @@ -1043,23 +1054,72 @@ static int ast_connector_init(struct drm_device *dev)
>>   * Mode config
>>   */
>>  
>> +static bool
>> +ast_crtc_needs_planes_disabled(struct drm_crtc_state *old_crtc_state,
>> +			       struct drm_crtc_state *new_crtc_state)
>> +{
>> +	struct ast_crtc_state *old_ast_crtc_state, *new_ast_crtc_state;
>> +
>> +	if (drm_atomic_crtc_needs_modeset(new_crtc_state))
>> +		return true;
>> +
>> +	old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
>> +	new_ast_crtc_state = to_ast_crtc_state(new_crtc_state);
>> +
>> +	if (old_ast_crtc_state->format != new_ast_crtc_state->format)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>>  static void
>>  ast_mode_config_helper_commit_tail(struct drm_atomic_state *old_state)
>>  {
>>  	struct drm_device *dev = old_state->dev;
>> +	struct ast_private *ast = to_ast_private(dev);
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> +	struct drm_crtc *crtc;
>> +	int i;
>> +	bool wait_for_vretrace = false;
>>  
>>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>>  
>> -	drm_atomic_helper_commit_planes(dev, old_state, 0);
>> +	/*
>> +	 * HW cursors require the underlying primary plane and CRTC to
>> +	 * display a valid mode and image. This is not the case during
>> +	 * full modeset operations. So we temporarily disable any active
>> +	 * plane, including the HW cursor. Each plane's atomic_update()
>> +	 * helper will re-enable it if necessary.
>> +	 *
>> +	 * We only do this during *full* modesets. It does not affect
>> +	 * simple pageflips on the planes.
>> +	 */
>> +	for_each_oldnew_crtc_in_state(old_state, crtc,
>> +				      old_crtc_state,
>> +				      new_crtc_state, i) {
>> +		if (!ast_crtc_needs_planes_disabled(old_crtc_state,
>> +						    new_crtc_state))
>> +			continue;
>> +		drm_atomic_helper_disable_planes_on_crtc(old_crtc_state,
>> +							 false);
>> +		wait_for_vretrace = true;
>> +	}
> 
> Hm this still feels like you're fighting the framework more than using it.
> Comment here, but it's kinda review comments on the entire series.
> 
> - ast_crtc_needs_planes_disabled feels a bit strange, the usual way to
>   handle this kind of stuff is to set crtc_state->needs_modeset from your
>   plane's atomic_check function. You might need your own atomic_check
>   implementation for that, so that after the plane checks you run the
>   modeset checks again.
> 
> - with that you can put your call here to disable all planes into the crtc
>   ->atomic_disable callback. You can then also put the
>   ast_wait_for_retrace in there, at the end.

The CRTC's atomic_disable/enable only run if needs_modeset() is true.

I brought back support for fast format changes of the primary plane.
Moving that code into atomic_disable/enable would require to set
needs_modeset in atomic_check() for format changes. And later figure out
in atomic_disable/enable if it's really a modeset or just a change of
the format. That's not good either.

Best regards
Thomas

> 
>> +
>> +	/*
>> +	 * Ensure that no scanout takes place before reprogramming mode
>> +	 * and format registers.
>> +	 */
>> +	if (wait_for_vretrace)
>> +		ast_wait_for_vretrace(ast);
>> +
>> +	drm_atomic_helper_commit_planes(dev, old_state,
>> +					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> 
> This order also feels a bit strange, especially with the first 2 patches
> where you put the crtc modeset code into atomic_begin. It feels a bit like
> if you do the plane commit _after_ modeset enables, then you could move
> the crtc code into the crtc ->atomic_enable hook, and then let the plane
> update stuff roll through all in commit_planes. Moving the modset code
> into atomic_begin at least suggests you want modeset enables before plane
> commit, and lots of drivers have that sequence in their commit_tail. It's
> even a default implementation with drm_atomic_helper_commit_tail_rpm.
> 
> Sorry this is all dragging around so much, figuring out the best atomic
> flow is occasionally a bit an endeavour :-/
> 
> Cheers, Daniel
> 
>>  
>>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>  
>>  	drm_atomic_helper_fake_vblank(old_state);
>> -
>>  	drm_atomic_helper_commit_hw_done(old_state);
>> -
>>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
>> -
>>  	drm_atomic_helper_cleanup_planes(dev, old_state);
>>  }
>>  
>> -- 
>> 2.28.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]