| Message ID | 20220622124815.356035-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Fix black screen when getting out of suspend | expand |
Some small nitpicks: On Wed, 2022-06-22 at 14:48 +0200, Jocelyn Falempe wrote: > With an AST2600, the screen is garbage when going out of suspend. > This is because color settings are lost, and not restored on resume. > Force the color settings on DPMS_ON, to make sure the settings are correct. > > I didn't write this code, it comes from the out-of-tree aspeed driver v1.13 > https://www.aspeedtech.com/support_driver/ > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> Should have a Cc: to stable imho, `dim` can do this for you: https://drm.pages.freedesktop.org/maintainer-tools/dim.html > --- > drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index 3eb9afecd9d4..cdddcb5c4439 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int > mode) > { > struct ast_private *ast = to_ast_private(crtc->dev); > 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). > @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int > mode) > ast_dp_set_on_off(crtc->dev, 1); > } > > + ast_state = to_ast_crtc_state(crtc->state); > + format = ast_state->format; > + > + if (format){ Should be a space between ')' and '{'. With that fixed, this is: Reviewed-by: Lyude Paul <lyude@redhat.com> > + vbios_mode_info = &ast_state->vbios_mode_info; > + > + ast_set_color_reg(ast, format); > + ast_set_vbios_color_reg(ast, format, > vbios_mode_info); > + } > + > ast_crtc_load_lut(ast, crtc); > break; > case DRM_MODE_DPMS_STANDBY:
Hi Am 22.06.22 um 14:48 schrieb Jocelyn Falempe: > With an AST2600, the screen is garbage when going out of suspend. > This is because color settings are lost, and not restored on resume. > Force the color settings on DPMS_ON, to make sure the settings are correct. > > I didn't write this code, it comes from the out-of-tree aspeed driver v1.13 > https://www.aspeedtech.com/support_driver/ > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> With the small fixes that Lyude mentioned: Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Best regards Thomas > --- > drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index 3eb9afecd9d4..cdddcb5c4439 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) > { > struct ast_private *ast = to_ast_private(crtc->dev); > 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). > @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) > ast_dp_set_on_off(crtc->dev, 1); > } > > + ast_state = to_ast_crtc_state(crtc->state); > + 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); > + } > + > ast_crtc_load_lut(ast, crtc); > break; > case DRM_MODE_DPMS_STANDBY:
On 22/06/2022 20:34, Lyude Paul wrote: > Some small nitpicks: > > On Wed, 2022-06-22 at 14:48 +0200, Jocelyn Falempe wrote: >> With an AST2600, the screen is garbage when going out of suspend. >> This is because color settings are lost, and not restored on resume. >> Force the color settings on DPMS_ON, to make sure the settings are correct. >> >> I didn't write this code, it comes from the out-of-tree aspeed driver v1.13 >> https://www.aspeedtech.com/support_driver/ >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> > > Should have a Cc: to stable imho, `dim` can do this for you: > > https://drm.pages.freedesktop.org/maintainer-tools/dim.html Sure I will add cc to stable for the v2 > >> --- >> drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index 3eb9afecd9d4..cdddcb5c4439 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int >> mode) >> { >> struct ast_private *ast = to_ast_private(crtc->dev); >> 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). >> @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int >> mode) >> ast_dp_set_on_off(crtc->dev, 1); >> } >> >> + ast_state = to_ast_crtc_state(crtc->state); >> + format = ast_state->format; >> + >> + if (format){ > > Should be a space between ')' and '{'. looks like I forget to run checkpatch.pl on it before sending :( I'll wait a bit for other comments, and sent a v2 with cc for stable. > > With that fixed, this is: Reviewed-by: Lyude Paul <lyude@redhat.com> > >> + vbios_mode_info = &ast_state->vbios_mode_info; >> + >> + ast_set_color_reg(ast, format); >> + ast_set_vbios_color_reg(ast, format, >> vbios_mode_info); >> + } >> + >> ast_crtc_load_lut(ast, crtc); >> break; >> case DRM_MODE_DPMS_STANDBY: >
Hi Am 22.06.22 um 14:48 schrieb Jocelyn Falempe: > With an AST2600, the screen is garbage when going out of suspend. > This is because color settings are lost, and not restored on resume. > Force the color settings on DPMS_ON, to make sure the settings are correct. > > I didn't write this code, it comes from the out-of-tree aspeed driver v1.13 > https://www.aspeedtech.com/support_driver/ > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> > --- > drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index 3eb9afecd9d4..cdddcb5c4439 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) > { > struct ast_private *ast = to_ast_private(crtc->dev); > 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). > @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) > ast_dp_set_on_off(crtc->dev, 1); > } > > + ast_state = to_ast_crtc_state(crtc->state); > + 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); > + } > + I've seen suspend issues on other AST devices besides the 2600. This seems to be an improvement on AST2300 at least. Therefore Tested-by: Thomas Zimmermann <tzimmermann@suse.de> The DPMS code need to be integrated into atomic_enable at some point. (It's for a later patchset.) It's a relict of the old non-atomic modesetting that never got done correctly. Best regards Thomas > ast_crtc_load_lut(ast, crtc); > break; > case DRM_MODE_DPMS_STANDBY:
On 23/06/2022 10:21, Jocelyn Falempe wrote: > On 22/06/2022 20:34, Lyude Paul wrote: >> Some small nitpicks: >> >> On Wed, 2022-06-22 at 14:48 +0200, Jocelyn Falempe wrote: >>> With an AST2600, the screen is garbage when going out of suspend. >>> This is because color settings are lost, and not restored on resume. >>> Force the color settings on DPMS_ON, to make sure the settings are >>> correct. >>> >>> I didn't write this code, it comes from the out-of-tree aspeed driver >>> v1.13 >>> https://www.aspeedtech.com/support_driver/ >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> >> >> Should have a Cc: to stable imho, `dim` can do this for you: >> >> https://drm.pages.freedesktop.org/maintainer-tools/dim.html > > Sure I will add cc to stable for the v2 In fact it doesn't apply to older kernel version, (it depends on 594e9c04b586 Create the driver for ASPEED proprietory Display-Port). So I think I will just push it to drm-misc-next, with the indentation fixed. >> >>> --- >>> drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/ast/ast_mode.c >>> b/drivers/gpu/drm/ast/ast_mode.c >>> index 3eb9afecd9d4..cdddcb5c4439 100644 >>> --- a/drivers/gpu/drm/ast/ast_mode.c >>> +++ b/drivers/gpu/drm/ast/ast_mode.c >>> @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int >>> mode) >>> { >>> struct ast_private *ast = to_ast_private(crtc->dev); >>> 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). >>> @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc >>> *crtc, int >>> mode) >>> ast_dp_set_on_off(crtc->dev, 1); >>> } >>> + ast_state = to_ast_crtc_state(crtc->state); >>> + format = ast_state->format; >>> + >>> + if (format){ >> >> Should be a space between ')' and '{'. > > looks like I forget to run checkpatch.pl on it before sending :( > > I'll wait a bit for other comments, and sent a v2 with cc for stable. >> >> With that fixed, this is: Reviewed-by: Lyude Paul <lyude@redhat.com> >> >>> + vbios_mode_info = &ast_state->vbios_mode_info; >>> + >>> + ast_set_color_reg(ast, format); >>> + ast_set_vbios_color_reg(ast, format, >>> vbios_mode_info); >>> + } >>> + >>> ast_crtc_load_lut(ast, crtc); >>> break; >>> case DRM_MODE_DPMS_STANDBY: >> >
Am 27.06.22 um 09:31 schrieb Jocelyn Falempe: > On 23/06/2022 10:21, Jocelyn Falempe wrote: >> On 22/06/2022 20:34, Lyude Paul wrote: >>> Some small nitpicks: >>> >>> On Wed, 2022-06-22 at 14:48 +0200, Jocelyn Falempe wrote: >>>> With an AST2600, the screen is garbage when going out of suspend. >>>> This is because color settings are lost, and not restored on resume. >>>> Force the color settings on DPMS_ON, to make sure the settings are >>>> correct. >>>> >>>> I didn't write this code, it comes from the out-of-tree aspeed >>>> driver v1.13 >>>> https://www.aspeedtech.com/support_driver/ >>>> >>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>>> Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> >>> >>> Should have a Cc: to stable imho, `dim` can do this for you: >>> >>> https://drm.pages.freedesktop.org/maintainer-tools/dim.html >> >> Sure I will add cc to stable for the v2 > > In fact it doesn't apply to older kernel version, (it depends on > 594e9c04b586 Create the driver for ASPEED proprietory Display-Port). > > So I think I will just push it to drm-misc-next, with the indentation > fixed. Sure, go ahead. > >>> >>>> --- >>>> drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c >>>> b/drivers/gpu/drm/ast/ast_mode.c >>>> index 3eb9afecd9d4..cdddcb5c4439 100644 >>>> --- a/drivers/gpu/drm/ast/ast_mode.c >>>> +++ b/drivers/gpu/drm/ast/ast_mode.c >>>> @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, >>>> int >>>> mode) >>>> { >>>> struct ast_private *ast = to_ast_private(crtc->dev); >>>> 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). >>>> @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc >>>> *crtc, int >>>> mode) >>>> ast_dp_set_on_off(crtc->dev, 1); >>>> } >>>> + ast_state = to_ast_crtc_state(crtc->state); >>>> + format = ast_state->format; >>>> + >>>> + if (format){ >>> >>> Should be a space between ')' and '{'. >> >> looks like I forget to run checkpatch.pl on it before sending :( >> >> I'll wait a bit for other comments, and sent a v2 with cc for stable. >>> >>> With that fixed, this is: Reviewed-by: Lyude Paul <lyude@redhat.com> >>> >>>> + vbios_mode_info = &ast_state->vbios_mode_info; >>>> + >>>> + ast_set_color_reg(ast, format); >>>> + ast_set_vbios_color_reg(ast, format, >>>> vbios_mode_info); >>>> + } >>>> + >>>> ast_crtc_load_lut(ast, crtc); >>>> break; >>>> case DRM_MODE_DPMS_STANDBY: >>> >> >
On 27/06/2022 09:39, Thomas Zimmermann wrote: > > > Am 27.06.22 um 09:31 schrieb Jocelyn Falempe: >> On 23/06/2022 10:21, Jocelyn Falempe wrote: >>> On 22/06/2022 20:34, Lyude Paul wrote: >>>> Some small nitpicks: >>>> >>>> On Wed, 2022-06-22 at 14:48 +0200, Jocelyn Falempe wrote: >>>>> With an AST2600, the screen is garbage when going out of suspend. >>>>> This is because color settings are lost, and not restored on resume. >>>>> Force the color settings on DPMS_ON, to make sure the settings are >>>>> correct. >>>>> >>>>> I didn't write this code, it comes from the out-of-tree aspeed >>>>> driver v1.13 >>>>> https://www.aspeedtech.com/support_driver/ >>>>> >>>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>>>> Tested-by: Venkat Tadikonda <venkateswara.rao@intel.com> >>>> >>>> Should have a Cc: to stable imho, `dim` can do this for you: >>>> >>>> https://drm.pages.freedesktop.org/maintainer-tools/dim.html >>> >>> Sure I will add cc to stable for the v2 >> >> In fact it doesn't apply to older kernel version, (it depends on >> 594e9c04b586 Create the driver for ASPEED proprietory Display-Port). >> >> So I think I will just push it to drm-misc-next, with the indentation >> fixed. > > Sure, go ahead. Applied to drm-misc-next, thank you all. > >> >>>> >>>>> --- >>>>> drivers/gpu/drm/ast/ast_mode.c | 13 +++++++++++++ >>>>> 1 file changed, 13 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c >>>>> b/drivers/gpu/drm/ast/ast_mode.c >>>>> index 3eb9afecd9d4..cdddcb5c4439 100644 >>>>> --- a/drivers/gpu/drm/ast/ast_mode.c >>>>> +++ b/drivers/gpu/drm/ast/ast_mode.c >>>>> @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc >>>>> *crtc, int >>>>> mode) >>>>> { >>>>> struct ast_private *ast = to_ast_private(crtc->dev); >>>>> 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). >>>>> @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc >>>>> *crtc, int >>>>> mode) >>>>> ast_dp_set_on_off(crtc->dev, 1); >>>>> } >>>>> + ast_state = to_ast_crtc_state(crtc->state); >>>>> + format = ast_state->format; >>>>> + >>>>> + if (format){ >>>> >>>> Should be a space between ')' and '{'. >>> >>> looks like I forget to run checkpatch.pl on it before sending :( >>> >>> I'll wait a bit for other comments, and sent a v2 with cc for stable. >>>> >>>> With that fixed, this is: Reviewed-by: Lyude Paul <lyude@redhat.com> >>>> >>>>> + vbios_mode_info = &ast_state->vbios_mode_info; >>>>> + >>>>> + ast_set_color_reg(ast, format); >>>>> + ast_set_vbios_color_reg(ast, format, >>>>> vbios_mode_info); >>>>> + } >>>>> + >>>>> ast_crtc_load_lut(ast, crtc); >>>>> break; >>>>> case DRM_MODE_DPMS_STANDBY: >>>> >>> >> >
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 3eb9afecd9d4..cdddcb5c4439 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -990,6 +990,9 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) { struct ast_private *ast = to_ast_private(crtc->dev); 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). @@ -1007,6 +1010,16 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode) ast_dp_set_on_off(crtc->dev, 1); } + ast_state = to_ast_crtc_state(crtc->state); + 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); + } + ast_crtc_load_lut(ast, crtc); break; case DRM_MODE_DPMS_STANDBY: