| Message ID | 20230706100102.563458-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/ast: report connection status on Display Port. | expand |
On 06.07.23 11:58, Jocelyn Falempe wrote: > Aspeed always report the display port as "connected", because it > doesn't set a .detect callback. > Fix this by providing the proper detect callback for astdp and dp501. > > This also fixes the following regression: > Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no > EDID on DP") > The default resolution is now 640x480 when no monitor is connected. > But Aspeed graphics is mostly used in servers, where no monitor > is attached. This also affects the remote BMC resolution to 640x480, > which is inconvenient, and breaks the anaconda installer. > > v2: Add .detect callback to the dp/dp501 connector (Jani Nikula) > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> So if this "also fixes a regression" how about a Fixes: tag and a CC: <stable tag to ensure this is not only fixed in latest mainline, but also in all affected stable and longterm kernels? Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page.
Hi Jocelyn Am 06.07.23 um 11:58 schrieb Jocelyn Falempe: > Aspeed always report the display port as "connected", because it > doesn't set a .detect callback. > Fix this by providing the proper detect callback for astdp and dp501. > > This also fixes the following regression: > Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no > EDID on DP") > The default resolution is now 640x480 when no monitor is connected. > But Aspeed graphics is mostly used in servers, where no monitor > is attached. This also affects the remote BMC resolution to 640x480, > which is inconvenient, and breaks the anaconda installer. The formating of these paraghraphs looks off. > > v2: Add .detect callback to the dp/dp501 connector (Jani Nikula) > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> Maybe add Fixes with the patch that added the DP default resolution. It's not really correct, but at least tells people to consider your patch. I assume that you'll send another patch with the BMC connector? > --- > drivers/gpu/drm/ast/ast_dp.c | 9 ++++++++ > drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++----------- > drivers/gpu/drm/ast/ast_drv.h | 2 ++ > drivers/gpu/drm/ast/ast_mode.c | 24 +++++++++++++++++++++ > 4 files changed, 60 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c > index 6dc1a09504e1..fbc154930fdf 100644 > --- a/drivers/gpu/drm/ast/ast_dp.c > +++ b/drivers/gpu/drm/ast/ast_dp.c > @@ -7,6 +7,15 @@ > #include <drm/drm_print.h> > #include "ast_drv.h" > > +bool ast_astdp_is_connected(struct ast_device *ast) > +{ > + if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, ASTDP_MCU_FW_EXECUTING) && > + ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, ASTDP_LINK_SUCCESS) && > + ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD)) The indention of those two lines is off. You could also rewrite this as if (!a) return false; if (!b) return false; if (!c) return false; return true; > + return true; > + return false; > +} > + > int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata) > { > struct ast_device *ast = to_ast_device(dev); > diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c > index a5d285850ffb..f10d53b0c94f 100644 > --- a/drivers/gpu/drm/ast/ast_dp501.c > +++ b/drivers/gpu/drm/ast/ast_dp501.c > @@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev) > return true; > } > > -bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) > +bool ast_dp501_is_connected(struct ast_device *ast) > { > - struct ast_device *ast = to_ast_device(dev); > - u32 i, boot_address, offset, data; > - u32 *pEDIDidx; > + u32 boot_address, offset, data; > > if (ast->config_mode == ast_use_p2a) { > boot_address = get_fw_base(ast); > @@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) > data = ast_mindwm(ast, boot_address + offset); > if (!(data & AST_DP501_PNP_CONNECTED)) > return false; > - > - /* Read EDID */ > - offset = AST_DP501_EDID_DATA; > - for (i = 0; i < 128; i += 4) { > - data = ast_mindwm(ast, boot_address + offset + i); > - pEDIDidx = (u32 *)(ediddata + i); > - *pEDIDidx = data; > - } > } else { > if (!ast->dp501_fw_buf) > return false; > @@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) > data = readl(ast->dp501_fw_buf + offset); > if (!(data & AST_DP501_PNP_CONNECTED)) > return false; > + } > + return true; > +} > + > +bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) > +{ > + struct ast_device *ast = to_ast_device(dev); > + u32 i, boot_address, offset, data; > + u32 *pEDIDidx; > + > + if (!ast_dp501_is_connected(ast)) > + return false; > + > + if (ast->config_mode == ast_use_p2a) { > + boot_address = get_fw_base(ast); > > + /* Read EDID */ > + offset = AST_DP501_EDID_DATA; > + for (i = 0; i < 128; i += 4) { > + data = ast_mindwm(ast, boot_address + offset + i); > + pEDIDidx = (u32 *)(ediddata + i); > + *pEDIDidx = data; > + } > + } else { > /* Read EDID */ > offset = AST_DP501_EDID_DATA; > for (i = 0; i < 128; i += 4) { > diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h > index 3f6e0c984523..99a24d779b9c 100644 > --- a/drivers/gpu/drm/ast/ast_drv.h > +++ b/drivers/gpu/drm/ast/ast_drv.h > @@ -510,6 +510,7 @@ void ast_patch_ahb_2500(struct ast_device *ast); > /* ast dp501 */ > void ast_set_dp501_video_output(struct drm_device *dev, u8 mode); > bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); > +bool ast_dp501_is_connected(struct ast_device *ast); > bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); > u8 ast_get_dp501_max_clk(struct drm_device *dev); > void ast_init_3rdtx(struct drm_device *dev); > @@ -518,6 +519,7 @@ void ast_init_3rdtx(struct drm_device *dev); > struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); > > /* aspeed DP */ > +bool ast_astdp_is_connected(struct ast_device *ast); > int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata); > void ast_dp_launch(struct drm_device *dev); > void ast_dp_power_on_off(struct drm_device *dev, bool no); > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index f711d592da52..ccf48d57d239 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -1586,12 +1586,24 @@ static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs > .get_modes = ast_dp501_connector_helper_get_modes, > }; > > +static enum drm_connector_status ast_dp501_connector_helper_detect( > + struct drm_connector *connector, > + bool force) > +{ > + struct ast_device *ast = to_ast_device(connector->dev); > + > + if (ast_dp501_is_connected(ast)) > + return connector_status_connected; > + return connector_status_disconnected; > +} It seems more appropriate to implement this as connector helper func detect_ctx. It does the same in practice, but the detect funtionality is rather part of the helpers. > + > static const struct drm_connector_funcs ast_dp501_connector_funcs = { > .reset = drm_atomic_helper_connector_reset, > .fill_modes = drm_helper_probe_single_connector_modes, > .destroy = drm_connector_cleanup, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > + .detect = ast_dp501_connector_helper_detect, That might sound pedantic, but I'd prefer to have the assignments sorted as in the struct's definiation. :) So .detect would be set just below .reset. > }; > > static int ast_dp501_connector_init(struct drm_device *dev, struct drm_connector *connector) > @@ -1684,12 +1696,24 @@ static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs > .get_modes = ast_astdp_connector_helper_get_modes, > }; > > +static enum drm_connector_status ast_astdp_connector_helper_detect( > + struct drm_connector *connector, > + bool force) > +{ > + struct ast_device *ast = to_ast_device(connector->dev); > + > + if (ast_astdp_is_connected(ast)) > + return connector_status_connected; > + return connector_status_disconnected; > +} > + > static const struct drm_connector_funcs ast_astdp_connector_funcs = { > .reset = drm_atomic_helper_connector_reset, > .fill_modes = drm_helper_probe_single_connector_modes, > .destroy = drm_connector_cleanup, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > + .detect = ast_astdp_connector_helper_detect, Same here. Best regards Thomas > }; > > static int ast_astdp_connector_init(struct drm_device *dev, struct drm_connector *connector) > > base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2
On 06/07/2023 15:03, Linux regression tracking (Thorsten Leemhuis) wrote: > On 06.07.23 11:58, Jocelyn Falempe wrote: >> Aspeed always report the display port as "connected", because it >> doesn't set a .detect callback. >> Fix this by providing the proper detect callback for astdp and dp501. >> >> This also fixes the following regression: >> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no >> EDID on DP") >> The default resolution is now 640x480 when no monitor is connected. >> But Aspeed graphics is mostly used in servers, where no monitor >> is attached. This also affects the remote BMC resolution to 640x480, >> which is inconvenient, and breaks the anaconda installer. >> >> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula) >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > > So if this "also fixes a regression" how about a Fixes: tag and a CC: > <stable tag to ensure this is not only fixed in latest mainline, but > also in all affected stable and longterm kernels? In this case, the regression only affect one userspace program (anaconda), and the fix looks too risky to backport to all stable kernels. I've to prepare a v3, as this one breaks on hardware were there is only one Display Port connector and no VGA connector. > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. > Best regards,
On 10.07.23 10:12, Jocelyn Falempe wrote: > On 06/07/2023 15:03, Linux regression tracking (Thorsten Leemhuis) wrote: >> On 06.07.23 11:58, Jocelyn Falempe wrote: >>> Aspeed always report the display port as "connected", because it >>> doesn't set a .detect callback. >>> Fix this by providing the proper detect callback for astdp and dp501. >>> >>> This also fixes the following regression: >>> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no >>> EDID on DP") >>> The default resolution is now 640x480 when no monitor is connected. >>> But Aspeed graphics is mostly used in servers, where no monitor >>> is attached. This also affects the remote BMC resolution to 640x480, >>> which is inconvenient, and breaks the anaconda installer. >>> >>> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula) >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> >> So if this "also fixes a regression" how about a Fixes: tag and a CC: >> <stable tag to ensure this is not only fixed in latest mainline, but >> also in all affected stable and longterm kernels? > > In this case, the regression only affect one userspace program > (anaconda), That is (mostly) irrelevant when it comes to regressions. > and the fix looks too risky to backport to all stable kernels. Not sure, but I tend to thing that decision would better be left to the stable team. Each developer will have a different opinion about what's too risky or not and they might be in the better position to judge what they want for their trees. A "Fixes:" tag thus still seems appropriate here; will also tell downstream distros that might want to pick this up. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page.
On 10/07/2023 08:57, Thomas Zimmermann wrote: > Hi Jocelyn > > Am 06.07.23 um 11:58 schrieb Jocelyn Falempe: >> Aspeed always report the display port as "connected", because it >> doesn't set a .detect callback. >> Fix this by providing the proper detect callback for astdp and dp501. >> >> This also fixes the following regression: >> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no >> EDID on DP") >> The default resolution is now 640x480 when no monitor is connected. >> But Aspeed graphics is mostly used in servers, where no monitor >> is attached. This also affects the remote BMC resolution to 640x480, >> which is inconvenient, and breaks the anaconda installer. > > The formating of these paraghraphs looks off. Ok > >> >> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula) >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > > Maybe add Fixes with the patch that added the DP default resolution. > It's not really correct, but at least tells people to consider your patch. > > I assume that you'll send another patch with the BMC connector? Yes, this patch alone will break more systems than what it tries to fix. > >> --- >> drivers/gpu/drm/ast/ast_dp.c | 9 ++++++++ >> drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++----------- >> drivers/gpu/drm/ast/ast_drv.h | 2 ++ >> drivers/gpu/drm/ast/ast_mode.c | 24 +++++++++++++++++++++ >> 4 files changed, 60 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c >> index 6dc1a09504e1..fbc154930fdf 100644 >> --- a/drivers/gpu/drm/ast/ast_dp.c >> +++ b/drivers/gpu/drm/ast/ast_dp.c >> @@ -7,6 +7,15 @@ >> #include <drm/drm_print.h> >> #include "ast_drv.h" >> +bool ast_astdp_is_connected(struct ast_device *ast) >> +{ >> + if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, >> ASTDP_MCU_FW_EXECUTING) && >> + ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, >> ASTDP_LINK_SUCCESS) && >> + ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD)) > > The indention of those two lines is off. > > You could also rewrite this as > > if (!a) > return false; > if (!b) > return false; > if (!c) > return false; > return true; ok > >> + return true; >> + return false; >> +} >> + >> int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata) >> { >> struct ast_device *ast = to_ast_device(dev); >> diff --git a/drivers/gpu/drm/ast/ast_dp501.c >> b/drivers/gpu/drm/ast/ast_dp501.c >> index a5d285850ffb..f10d53b0c94f 100644 >> --- a/drivers/gpu/drm/ast/ast_dp501.c >> +++ b/drivers/gpu/drm/ast/ast_dp501.c >> @@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev) >> return true; >> } >> -bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) >> +bool ast_dp501_is_connected(struct ast_device *ast) >> { >> - struct ast_device *ast = to_ast_device(dev); >> - u32 i, boot_address, offset, data; >> - u32 *pEDIDidx; >> + u32 boot_address, offset, data; >> if (ast->config_mode == ast_use_p2a) { >> boot_address = get_fw_base(ast); >> @@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, >> u8 *ediddata) >> data = ast_mindwm(ast, boot_address + offset); >> if (!(data & AST_DP501_PNP_CONNECTED)) >> return false; >> - >> - /* Read EDID */ >> - offset = AST_DP501_EDID_DATA; >> - for (i = 0; i < 128; i += 4) { >> - data = ast_mindwm(ast, boot_address + offset + i); >> - pEDIDidx = (u32 *)(ediddata + i); >> - *pEDIDidx = data; >> - } >> } else { >> if (!ast->dp501_fw_buf) >> return false; >> @@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, >> u8 *ediddata) >> data = readl(ast->dp501_fw_buf + offset); >> if (!(data & AST_DP501_PNP_CONNECTED)) >> return false; >> + } >> + return true; >> +} >> + >> +bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) >> +{ >> + struct ast_device *ast = to_ast_device(dev); >> + u32 i, boot_address, offset, data; >> + u32 *pEDIDidx; >> + >> + if (!ast_dp501_is_connected(ast)) >> + return false; >> + >> + if (ast->config_mode == ast_use_p2a) { >> + boot_address = get_fw_base(ast); >> + /* Read EDID */ >> + offset = AST_DP501_EDID_DATA; >> + for (i = 0; i < 128; i += 4) { >> + data = ast_mindwm(ast, boot_address + offset + i); >> + pEDIDidx = (u32 *)(ediddata + i); >> + *pEDIDidx = data; >> + } >> + } else { >> /* Read EDID */ >> offset = AST_DP501_EDID_DATA; >> for (i = 0; i < 128; i += 4) { >> diff --git a/drivers/gpu/drm/ast/ast_drv.h >> b/drivers/gpu/drm/ast/ast_drv.h >> index 3f6e0c984523..99a24d779b9c 100644 >> --- a/drivers/gpu/drm/ast/ast_drv.h >> +++ b/drivers/gpu/drm/ast/ast_drv.h >> @@ -510,6 +510,7 @@ void ast_patch_ahb_2500(struct ast_device *ast); >> /* ast dp501 */ >> void ast_set_dp501_video_output(struct drm_device *dev, u8 mode); >> bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); >> +bool ast_dp501_is_connected(struct ast_device *ast); >> bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); >> u8 ast_get_dp501_max_clk(struct drm_device *dev); >> void ast_init_3rdtx(struct drm_device *dev); >> @@ -518,6 +519,7 @@ void ast_init_3rdtx(struct drm_device *dev); >> struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); >> /* aspeed DP */ >> +bool ast_astdp_is_connected(struct ast_device *ast); >> int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata); >> void ast_dp_launch(struct drm_device *dev); >> void ast_dp_power_on_off(struct drm_device *dev, bool no); >> diff --git a/drivers/gpu/drm/ast/ast_mode.c >> b/drivers/gpu/drm/ast/ast_mode.c >> index f711d592da52..ccf48d57d239 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -1586,12 +1586,24 @@ static const struct drm_connector_helper_funcs >> ast_dp501_connector_helper_funcs >> .get_modes = ast_dp501_connector_helper_get_modes, >> }; >> +static enum drm_connector_status ast_dp501_connector_helper_detect( >> + struct drm_connector *connector, >> + bool force) >> +{ >> + struct ast_device *ast = to_ast_device(connector->dev); >> + >> + if (ast_dp501_is_connected(ast)) >> + return connector_status_connected; >> + return connector_status_disconnected; >> +} > > It seems more appropriate to implement this as connector helper func > detect_ctx. It does the same in practice, but the detect funtionality is > rather part of the helpers. ok, it looks easy to switch to detect_ctx(). > >> + >> static const struct drm_connector_funcs ast_dp501_connector_funcs = { >> .reset = drm_atomic_helper_connector_reset, >> .fill_modes = drm_helper_probe_single_connector_modes, >> .destroy = drm_connector_cleanup, >> .atomic_duplicate_state = >> drm_atomic_helper_connector_duplicate_state, >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> + .detect = ast_dp501_connector_helper_detect, > > That might sound pedantic, but I'd prefer to have the assignments sorted > as in the struct's definiation. :) So .detect would be set just below > .reset. ok, I was wondering what the order was, as it was not alphabetically. > >> }; >> static int ast_dp501_connector_init(struct drm_device *dev, struct >> drm_connector *connector) >> @@ -1684,12 +1696,24 @@ static const struct drm_connector_helper_funcs >> ast_astdp_connector_helper_funcs >> .get_modes = ast_astdp_connector_helper_get_modes, >> }; >> +static enum drm_connector_status ast_astdp_connector_helper_detect( >> + struct drm_connector *connector, >> + bool force) >> +{ >> + struct ast_device *ast = to_ast_device(connector->dev); >> + >> + if (ast_astdp_is_connected(ast)) >> + return connector_status_connected; >> + return connector_status_disconnected; >> +} >> + >> static const struct drm_connector_funcs ast_astdp_connector_funcs = { >> .reset = drm_atomic_helper_connector_reset, >> .fill_modes = drm_helper_probe_single_connector_modes, >> .destroy = drm_connector_cleanup, >> .atomic_duplicate_state = >> drm_atomic_helper_connector_duplicate_state, >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> + .detect = ast_astdp_connector_helper_detect, > > Same here. > > Best regards > Thomas > >> }; >> static int ast_astdp_connector_init(struct drm_device *dev, struct >> drm_connector *connector) >> >> base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2 > Thanks for your review. I will try to add a virtual BMC connector, and send a v3.
diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c index 6dc1a09504e1..fbc154930fdf 100644 --- a/drivers/gpu/drm/ast/ast_dp.c +++ b/drivers/gpu/drm/ast/ast_dp.c @@ -7,6 +7,15 @@ #include <drm/drm_print.h> #include "ast_drv.h" +bool ast_astdp_is_connected(struct ast_device *ast) +{ + if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, ASTDP_MCU_FW_EXECUTING) && + ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, ASTDP_LINK_SUCCESS) && + ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD)) + return true; + return false; +} + int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata) { struct ast_device *ast = to_ast_device(dev); diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index a5d285850ffb..f10d53b0c94f 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev) return true; } -bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) +bool ast_dp501_is_connected(struct ast_device *ast) { - struct ast_device *ast = to_ast_device(dev); - u32 i, boot_address, offset, data; - u32 *pEDIDidx; + u32 boot_address, offset, data; if (ast->config_mode == ast_use_p2a) { boot_address = get_fw_base(ast); @@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) data = ast_mindwm(ast, boot_address + offset); if (!(data & AST_DP501_PNP_CONNECTED)) return false; - - /* Read EDID */ - offset = AST_DP501_EDID_DATA; - for (i = 0; i < 128; i += 4) { - data = ast_mindwm(ast, boot_address + offset + i); - pEDIDidx = (u32 *)(ediddata + i); - *pEDIDidx = data; - } } else { if (!ast->dp501_fw_buf) return false; @@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) data = readl(ast->dp501_fw_buf + offset); if (!(data & AST_DP501_PNP_CONNECTED)) return false; + } + return true; +} + +bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) +{ + struct ast_device *ast = to_ast_device(dev); + u32 i, boot_address, offset, data; + u32 *pEDIDidx; + + if (!ast_dp501_is_connected(ast)) + return false; + + if (ast->config_mode == ast_use_p2a) { + boot_address = get_fw_base(ast); + /* Read EDID */ + offset = AST_DP501_EDID_DATA; + for (i = 0; i < 128; i += 4) { + data = ast_mindwm(ast, boot_address + offset + i); + pEDIDidx = (u32 *)(ediddata + i); + *pEDIDidx = data; + } + } else { /* Read EDID */ offset = AST_DP501_EDID_DATA; for (i = 0; i < 128; i += 4) { diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 3f6e0c984523..99a24d779b9c 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -510,6 +510,7 @@ void ast_patch_ahb_2500(struct ast_device *ast); /* ast dp501 */ void ast_set_dp501_video_output(struct drm_device *dev, u8 mode); bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); +bool ast_dp501_is_connected(struct ast_device *ast); bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); u8 ast_get_dp501_max_clk(struct drm_device *dev); void ast_init_3rdtx(struct drm_device *dev); @@ -518,6 +519,7 @@ void ast_init_3rdtx(struct drm_device *dev); struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); /* aspeed DP */ +bool ast_astdp_is_connected(struct ast_device *ast); int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata); void ast_dp_launch(struct drm_device *dev); void ast_dp_power_on_off(struct drm_device *dev, bool no); diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f711d592da52..ccf48d57d239 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1586,12 +1586,24 @@ static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs .get_modes = ast_dp501_connector_helper_get_modes, }; +static enum drm_connector_status ast_dp501_connector_helper_detect( + struct drm_connector *connector, + bool force) +{ + struct ast_device *ast = to_ast_device(connector->dev); + + if (ast_dp501_is_connected(ast)) + return connector_status_connected; + return connector_status_disconnected; +} + static const struct drm_connector_funcs ast_dp501_connector_funcs = { .reset = drm_atomic_helper_connector_reset, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, + .detect = ast_dp501_connector_helper_detect, }; static int ast_dp501_connector_init(struct drm_device *dev, struct drm_connector *connector) @@ -1684,12 +1696,24 @@ static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs .get_modes = ast_astdp_connector_helper_get_modes, }; +static enum drm_connector_status ast_astdp_connector_helper_detect( + struct drm_connector *connector, + bool force) +{ + struct ast_device *ast = to_ast_device(connector->dev); + + if (ast_astdp_is_connected(ast)) + return connector_status_connected; + return connector_status_disconnected; +} + static const struct drm_connector_funcs ast_astdp_connector_funcs = { .reset = drm_atomic_helper_connector_reset, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, + .detect = ast_astdp_connector_helper_detect, }; static int ast_astdp_connector_init(struct drm_device *dev, struct drm_connector *connector)
Aspeed always report the display port as "connected", because it doesn't set a .detect callback. Fix this by providing the proper detect callback for astdp and dp501. This also fixes the following regression: Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP") The default resolution is now 640x480 when no monitor is connected. But Aspeed graphics is mostly used in servers, where no monitor is attached. This also affects the remote BMC resolution to 640x480, which is inconvenient, and breaks the anaconda installer. v2: Add .detect callback to the dp/dp501 connector (Jani Nikula) Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/ast/ast_dp.c | 9 ++++++++ drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++----------- drivers/gpu/drm/ast/ast_drv.h | 2 ++ drivers/gpu/drm/ast/ast_mode.c | 24 +++++++++++++++++++++ 4 files changed, 60 insertions(+), 12 deletions(-) base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2