| Message ID | 20240627153638.8765-5-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/ast: Untangle the chaos in mode setting | expand |
On 27/06/2024 17:27, Thomas Zimmermann wrote: > Several color registers are programmed in the DPMS code of the CRTC's > atomic_enable helper. This code will not be executed if the color format > changes without a full mode switch. The same code already exists in the > atomic_update helper of the primary plane. There, the code will not run > if only the display mode changes. This last sentence is unclear to me. > > The color format is a property of the primary plane, so consolidate all > code in the plane's atomic_update. With the comment clarified, Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/ast/ast_mode.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index e8312b3472ed..6a81d657175d 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -649,12 +649,12 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane, > struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); > struct drm_framebuffer *old_fb = old_plane_state->fb; > struct ast_plane *ast_plane = to_ast_plane(plane); > + struct drm_crtc *crtc = plane_state->crtc; > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > struct drm_rect damage; > struct drm_atomic_helper_damage_iter iter; > > - if (!old_fb || (fb->format != old_fb->format)) { > - struct drm_crtc *crtc = plane_state->crtc; > - struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + if (!old_fb || (fb->format != old_fb->format) || crtc_state->mode_changed) { > struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state); > struct ast_vbios_mode_info *vbios_mode_info = &ast_crtc_state->vbios_mode_info; > > @@ -1025,7 +1025,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) > u8 ch = AST_DPMS_VSYNC_OFF | AST_DPMS_HSYNC_OFF; > struct ast_crtc_state *ast_state; > const struct drm_format_info *format; > - struct ast_vbios_mode_info *vbios_mode_info; > > /* TODO: Maybe control display signal generation with > * Sync Enable (bit CR17.7). > @@ -1039,10 +1038,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) > format = ast_state->format; > > if (format) { > - 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->gamma_lut) > ast_crtc_set_gamma(ast, format, crtc->state->gamma_lut->data); > else
Hi Am 28.06.24 um 11:54 schrieb Jocelyn Falempe: > > > On 27/06/2024 17:27, Thomas Zimmermann wrote: >> Several color registers are programmed in the DPMS code of the CRTC's >> atomic_enable helper. This code will not be executed if the color format >> changes without a full mode switch. The same code already exists in the >> atomic_update helper of the primary plane. There, the code will not run >> if only the display mode changes. > > This last sentence is unclear to me. OK, sure. It means that the plane's code might not run if the color format stays the same when the display mode changes. I'll try to reword. Best regards Thomas > >> >> The color format is a property of the primary plane, so consolidate all >> code in the plane's atomic_update. > > With the comment clarified, > > Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> > >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/ast/ast_mode.c | 11 +++-------- >> 1 file changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c >> b/drivers/gpu/drm/ast/ast_mode.c >> index e8312b3472ed..6a81d657175d 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -649,12 +649,12 @@ static void >> ast_primary_plane_helper_atomic_update(struct drm_plane *plane, >> struct drm_plane_state *old_plane_state = >> drm_atomic_get_old_plane_state(state, plane); >> struct drm_framebuffer *old_fb = old_plane_state->fb; >> struct ast_plane *ast_plane = to_ast_plane(plane); >> + struct drm_crtc *crtc = plane_state->crtc; >> + struct drm_crtc_state *crtc_state = >> drm_atomic_get_new_crtc_state(state, crtc); >> struct drm_rect damage; >> struct drm_atomic_helper_damage_iter iter; >> - if (!old_fb || (fb->format != old_fb->format)) { >> - struct drm_crtc *crtc = plane_state->crtc; >> - struct drm_crtc_state *crtc_state = >> drm_atomic_get_new_crtc_state(state, crtc); >> + if (!old_fb || (fb->format != old_fb->format) || >> crtc_state->mode_changed) { >> struct ast_crtc_state *ast_crtc_state = >> to_ast_crtc_state(crtc_state); >> struct ast_vbios_mode_info *vbios_mode_info = >> &ast_crtc_state->vbios_mode_info; >> @@ -1025,7 +1025,6 @@ static void ast_crtc_dpms(struct drm_crtc >> *crtc, int mode) >> u8 ch = AST_DPMS_VSYNC_OFF | AST_DPMS_HSYNC_OFF; >> struct ast_crtc_state *ast_state; >> const struct drm_format_info *format; >> - struct ast_vbios_mode_info *vbios_mode_info; >> /* TODO: Maybe control display signal generation with >> * Sync Enable (bit CR17.7). >> @@ -1039,10 +1038,6 @@ static void ast_crtc_dpms(struct drm_crtc >> *crtc, int mode) >> format = ast_state->format; >> if (format) { >> - 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->gamma_lut) >> ast_crtc_set_gamma(ast, format, >> crtc->state->gamma_lut->data); >> else >
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index e8312b3472ed..6a81d657175d 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -649,12 +649,12 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); struct drm_framebuffer *old_fb = old_plane_state->fb; struct ast_plane *ast_plane = to_ast_plane(plane); + struct drm_crtc *crtc = plane_state->crtc; + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); struct drm_rect damage; struct drm_atomic_helper_damage_iter iter; - if (!old_fb || (fb->format != old_fb->format)) { - struct drm_crtc *crtc = plane_state->crtc; - struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (!old_fb || (fb->format != old_fb->format) || crtc_state->mode_changed) { struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state); struct ast_vbios_mode_info *vbios_mode_info = &ast_crtc_state->vbios_mode_info; @@ -1025,7 +1025,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) u8 ch = AST_DPMS_VSYNC_OFF | AST_DPMS_HSYNC_OFF; struct ast_crtc_state *ast_state; const struct drm_format_info *format; - struct ast_vbios_mode_info *vbios_mode_info; /* TODO: Maybe control display signal generation with * Sync Enable (bit CR17.7). @@ -1039,10 +1038,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) format = ast_state->format; if (format) { - 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->gamma_lut) ast_crtc_set_gamma(ast, format, crtc->state->gamma_lut->data); else
Several color registers are programmed in the DPMS code of the CRTC's atomic_enable helper. This code will not be executed if the color format changes without a full mode switch. The same code already exists in the atomic_update helper of the primary plane. There, the code will not run if only the display mode changes. The color format is a property of the primary plane, so consolidate all code in the plane's atomic_update. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/ast/ast_mode.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)