Hi
Am 27.07.20 um 12:40 schrieb daniel@xxxxxxxx:
> On Mon, Jul 27, 2020 at 09:37:05AM +0200, Thomas Zimmermann wrote:
>> The atomic modesetting code tried to distinguish format changes from
>> full modesetting operations. In practice, this was buggy and the format
>> registers were often updated even for simple pageflips.
>
> Nah it's not buggy, it's intentional. Most hw can update formats in a flip
> withouth having to shut down the hw to do so.
Admittedly it was intentional when I write it, but it never really
worked. I think it might have even updated these color registers on each
frame.
>
>
>> Instead do a full modeset if the primary plane changes formats. It's
>> just as rare as an actual mode change, so there will be no performance
>> penalty.
>>
>> The patch also replaces a reference to drm_crtc_state.allow_modeset with
>> the correct call to drm_atomic_crtc_needs_modeset().
>>
>> 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_mode.c | 23 ++++++++++++++++-------
>> 1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 154cd877d9d1..3680a000b812 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -527,8 +527,8 @@ static const uint32_t ast_primary_plane_formats[] = {
>> static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> struct drm_plane_state *state)
>> {
>> - struct drm_crtc_state *crtc_state;
>> - struct ast_crtc_state *ast_crtc_state;
>> + struct drm_crtc_state *crtc_state, *old_crtc_state;
>> + struct ast_crtc_state *ast_crtc_state, *old_ast_crtc_state;
>> int ret;
>>
>> if (!state->crtc)
>> @@ -550,6 +550,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>
>> ast_crtc_state->format = state->fb->format;
>>
>> + old_crtc_state = drm_atomic_get_old_crtc_state(state->state, state->crtc);
>> + if (!old_crtc_state)
>> + return 0;
>> + old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
>> + if (!old_ast_crtc_state)
>
> The above two if checks should never fail, I'd wrap them in a WARN_ON.
Really? But what's the old state when the first mode is being programmed?
>
>> + return 0;
>> + if (ast_crtc_state->format != old_ast_crtc_state->format)
>> + crtc_state->mode_changed = true;
>> +
>> return 0;
>> }
>>
>> @@ -775,18 +784,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>>
>> ast_state = to_ast_crtc_state(crtc->state);
>>
>> - format = ast_state->format;
>> - if (!format)
>> + if (!drm_atomic_crtc_needs_modeset(crtc->state))
>> return;
>>
>> + format = ast_state->format;
>> + if (drm_WARN_ON_ONCE(dev, !format))
>> + return; /* BUG: We didn't set format in primary check(). */
>
> Hm that entire ast_state->format machinery looks kinda strange, can't you
> just look up the primary plane state everywhere and that's it?
> drm_framebuffer are fully invariant and refcounted to the state, so there
> really shouldn't be any need to copy format around.
ast_state->format is the format that has to be programmed in
atomic_flush(). If it's NULL, the current format was used. Updating the
primary plane's format also requires the vbios info, which depends on
CRTC state. So it's collected in the CRTC's atomic_check().
It felt natural to use the various atomic_check() functions to collect
and store and store away these structures, and later use them in
atomic_flush().
I'd prefer to keep the current design. It's the one that worked best
while writing the atomic-modesetting support for ast.
Best regard
Thomas
>
> But that's maybe for a next patch. With the commit message clarified that
> everything works as designed, and maybe the two WARN_ON added:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
>> +
>> vbios_mode_info = &ast_state->vbios_mode_info;
>>
>> ast_set_color_reg(ast, format);
>> ast_set_vbios_color_reg(ast, format, vbios_mode_info);
>>
>> - if (!crtc->state->mode_changed)
>> - return;
>> -
>> adjusted_mode = &crtc->state->adjusted_mode;
>>
>> ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
>> --
>> 2.27.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
