Hi
Am 07.08.20 um 10:50 schrieb daniel@xxxxxxxx:
> 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@xxxxxxx>
>> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
>> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>> Cc: Dave Airlie <airlied@xxxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
>> Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>
>> Cc: "Y.C. Chen" <yc_chen@xxxxxxxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx> # 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:
signature.asc
Description: OpenPGP digital signature
