| Message ID | 20250124084546.2094575-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Fix default resolution on BMC when DP is not connected | expand |
Hi Jocelyn Am 24.01.25 um 09:45 schrieb Jocelyn Falempe: > The physical_status of ast_dp is not reliable, as it reports as > connected even when no monitor is connected. This status comes from VGACRDF, which is undocumented unfortunately. So IDK the exact semantics. Do you know if the other case can also happen where a connected monitor is not reported? > This makes the default > BMC resolution to be 640x480 for remote access. > So consider that if there is no edid, no monitor is connected, and > add the BMC 1024x768 default resolution. > I've debugged this regression on ast_dp, but as dp501 is similar, I > fixed both in this patch. > > This regression was likely introduced by commit 2281475168d2 > ("drm/ast: astdp: Perform link training during atomic_enable") > But I fixed it in the BMC get_modes handling. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > Fixes: bbad0090b9f4 ("drm/ast: astdp: Transparently handle BMC support") > --- > drivers/gpu/drm/ast/ast_dp.c | 14 +++++++------- > drivers/gpu/drm/ast/ast_dp501.c | 14 +++++++------- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c > index 0e282b7b167c..6c8ea95a2230 100644 > --- a/drivers/gpu/drm/ast/ast_dp.c > +++ b/drivers/gpu/drm/ast/ast_dp.c > @@ -361,19 +361,19 @@ static const struct drm_encoder_helper_funcs ast_astdp_encoder_helper_funcs = { > static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector) I don't think this is the right place to fix the problem. The field physical_status should always contain the correct physical status. So the fix should go into ast_dp_connector_helper_detect_ctx(). There's [1] something like if (ast_dp_status_is_connected(ast)) status = connected and that's where it should read the EDID without updating the connector's EDID property. Example code: if (ast_dp_status_is_connected(ast)) { edid = drm_edid_read_custom(/* like in get_modes */) if (drm_edid_valid(edid)) status = connected drm_edid_free(edid) } The EDID test could also go into _is_connected() directly. A comment about false positives from VGACRDF might make sense as well. [1] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ast/ast_dp.c#L397 > { > struct ast_connector *ast_connector = to_ast_connector(connector); > + struct ast_device *ast = to_ast_device(connector->dev); > + const struct drm_edid *drm_edid = NULL; > int count; > > - if (ast_connector->physical_status == connector_status_connected) { > - struct ast_device *ast = to_ast_device(connector->dev); > - const struct drm_edid *drm_edid; > - > + if (ast_connector->physical_status == connector_status_connected) > drm_edid = drm_edid_read_custom(connector, ast_astdp_read_edid_block, ast); > - drm_edid_connector_update(connector, drm_edid); > + > + drm_edid_connector_update(connector, drm_edid); > + > + if (drm_edid) { > count = drm_edid_connector_add_modes(connector); > drm_edid_free(drm_edid); > } else { > - drm_edid_connector_update(connector, NULL); > - > /* > * There's no EDID data without a connected monitor. Set BMC- > * compatible modes in this case. The XGA default resolution > diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c > index 9e19d8c17730..c92db65e3f20 100644 > --- a/drivers/gpu/drm/ast/ast_dp501.c > +++ b/drivers/gpu/drm/ast/ast_dp501.c I'd rather leave this out. The detection works differently for DP501. Best regards Thomas > @@ -504,19 +504,19 @@ static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = { > static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector) > { > struct ast_connector *ast_connector = to_ast_connector(connector); > + struct ast_device *ast = to_ast_device(connector->dev); > + const struct drm_edid *drm_edid = NULL; > int count; > > - if (ast_connector->physical_status == connector_status_connected) { > - struct ast_device *ast = to_ast_device(connector->dev); > - const struct drm_edid *drm_edid; > - > + if (ast_connector->physical_status == connector_status_connected) > drm_edid = drm_edid_read_custom(connector, ast_dp512_read_edid_block, ast); > - drm_edid_connector_update(connector, drm_edid); > + > + drm_edid_connector_update(connector, drm_edid); > + > + if (drm_edid) { > count = drm_edid_connector_add_modes(connector); > drm_edid_free(drm_edid); > } else { > - drm_edid_connector_update(connector, NULL); > - > /* > * There's no EDID data without a connected monitor. Set BMC- > * compatible modes in this case. The XGA default resolution > > base-commit: 798047e63ac970f105c49c22e6d44df901c486b2
On 24/01/2025 10:22, Thomas Zimmermann wrote: > Hi Jocelyn > > > Am 24.01.25 um 09:45 schrieb Jocelyn Falempe: >> The physical_status of ast_dp is not reliable, as it reports as >> connected even when no monitor is connected. > > This status comes from VGACRDF, which is undocumented unfortunately. So > IDK the exact semantics. Do you know if the other case can also happen > where a connected monitor is not reported? Previously, astdp_is_connected() also checked for ASTDP_LINK_SUCCESS https://elixir.bootlin.com/linux/v6.10.14/source/drivers/gpu/drm/ast/ast_dp.c#L16 which used to work more reliably, se maybe I can add it back? > >> This makes the default >> BMC resolution to be 640x480 for remote access. >> So consider that if there is no edid, no monitor is connected, and >> add the BMC 1024x768 default resolution. >> I've debugged this regression on ast_dp, but as dp501 is similar, I >> fixed both in this patch. >> >> This regression was likely introduced by commit 2281475168d2 >> ("drm/ast: astdp: Perform link training during atomic_enable") >> But I fixed it in the BMC get_modes handling. >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> Fixes: bbad0090b9f4 ("drm/ast: astdp: Transparently handle BMC support") >> --- >> drivers/gpu/drm/ast/ast_dp.c | 14 +++++++------- >> drivers/gpu/drm/ast/ast_dp501.c | 14 +++++++------- >> 2 files changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c >> index 0e282b7b167c..6c8ea95a2230 100644 >> --- a/drivers/gpu/drm/ast/ast_dp.c >> +++ b/drivers/gpu/drm/ast/ast_dp.c >> @@ -361,19 +361,19 @@ static const struct drm_encoder_helper_funcs >> ast_astdp_encoder_helper_funcs = { >> static int ast_astdp_connector_helper_get_modes(struct drm_connector >> *connector) > > I don't think this is the right place to fix the problem. The field > physical_status should always contain the correct physical status. So > the fix should go into ast_dp_connector_helper_detect_ctx(). There's [1] > something like If a DP monitor is connected, but the EDID is not readable, defaulting to 1024x768 is still a good choice. The default to 640x480 is only to comply with the DP specification, but in practice some DP monitors doesn't support 640x480. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e7c254d75d16b75abf1958095fd34e2ecdc0d645 > > if (ast_dp_status_is_connected(ast)) > status = connected > > and that's where it should read the EDID without updating the > connector's EDID property. Example code: > > if (ast_dp_status_is_connected(ast)) { > edid = drm_edid_read_custom(/* like in get_modes */) > if (drm_edid_valid(edid)) > status = connected > drm_edid_free(edid) > } > > The EDID test could also go into _is_connected() directly. A comment > about false positives from VGACRDF might make sense as well. > > [1] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ast/ > ast_dp.c#L397 > >> { >> struct ast_connector *ast_connector = to_ast_connector(connector); >> + struct ast_device *ast = to_ast_device(connector->dev); >> + const struct drm_edid *drm_edid = NULL; >> int count; >> - if (ast_connector->physical_status == connector_status_connected) { >> - struct ast_device *ast = to_ast_device(connector->dev); >> - const struct drm_edid *drm_edid; >> - >> + if (ast_connector->physical_status == connector_status_connected) >> drm_edid = drm_edid_read_custom(connector, >> ast_astdp_read_edid_block, ast); >> - drm_edid_connector_update(connector, drm_edid); >> + >> + drm_edid_connector_update(connector, drm_edid); >> + >> + if (drm_edid) { >> count = drm_edid_connector_add_modes(connector); >> drm_edid_free(drm_edid); >> } else { >> - drm_edid_connector_update(connector, NULL); >> - >> /* >> * There's no EDID data without a connected monitor. Set BMC- >> * compatible modes in this case. The XGA default resolution >> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ >> ast_dp501.c >> index 9e19d8c17730..c92db65e3f20 100644 >> --- a/drivers/gpu/drm/ast/ast_dp501.c >> +++ b/drivers/gpu/drm/ast/ast_dp501.c > > I'd rather leave this out. The detection works differently for DP501. ast_dp501_is_connected() hasn't changed, so yes I can drop this part. Best regards,
Hi Am 24.01.25 um 11:04 schrieb Jocelyn Falempe: > On 24/01/2025 10:22, Thomas Zimmermann wrote: >> Hi Jocelyn >> >> >> Am 24.01.25 um 09:45 schrieb Jocelyn Falempe: >>> The physical_status of ast_dp is not reliable, as it reports as >>> connected even when no monitor is connected. >> >> This status comes from VGACRDF, which is undocumented unfortunately. >> So IDK the exact semantics. Do you know if the other case can also >> happen where a connected monitor is not reported? > > Previously, astdp_is_connected() also checked for ASTDP_LINK_SUCCESS > https://elixir.bootlin.com/linux/v6.10.14/source/drivers/gpu/drm/ast/ast_dp.c#L16 > > > which used to work more reliably, se maybe I can add it back? That's a bit of a complicated issue. Protocol-wise Link Training runs only after HPD, EDID and DPCD. A nice overview is at https://www.ti.com/content/dam/videos/external-videos/en-us/8/3816841626001/6275649988001.mp4/subassets/understanding-dp-link-training-presentation-quiz.pdf Link training is only to be done at atomic_enable or _check time. I specifically asked about this back then. But having said that, ast doesn't really do Link Training; it just reads a bit from the VBIOS, which does it automatically. So it likely doesn't matter for ast. So yeah, maybe add it back if that works. Plus a comment that HPD alone is unreliable. I think this shoudl go into stable as well. Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable") Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Jocelyn Falempe <jfalempe@redhat.com> Cc: Dave Airlie <airlied@redhat.com> Cc: dri-devel@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v6.12+ > >> >>> This makes the default >>> BMC resolution to be 640x480 for remote access. >>> So consider that if there is no edid, no monitor is connected, and >>> add the BMC 1024x768 default resolution. >>> I've debugged this regression on ast_dp, but as dp501 is similar, I >>> fixed both in this patch. >>> >>> This regression was likely introduced by commit 2281475168d2 >>> ("drm/ast: astdp: Perform link training during atomic_enable") >>> But I fixed it in the BMC get_modes handling. >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> Fixes: bbad0090b9f4 ("drm/ast: astdp: Transparently handle BMC >>> support") >>> --- >>> drivers/gpu/drm/ast/ast_dp.c | 14 +++++++------- >>> drivers/gpu/drm/ast/ast_dp501.c | 14 +++++++------- >>> 2 files changed, 14 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ast/ast_dp.c >>> b/drivers/gpu/drm/ast/ast_dp.c >>> index 0e282b7b167c..6c8ea95a2230 100644 >>> --- a/drivers/gpu/drm/ast/ast_dp.c >>> +++ b/drivers/gpu/drm/ast/ast_dp.c >>> @@ -361,19 +361,19 @@ static const struct drm_encoder_helper_funcs >>> ast_astdp_encoder_helper_funcs = { >>> static int ast_astdp_connector_helper_get_modes(struct >>> drm_connector *connector) >> >> I don't think this is the right place to fix the problem. The field >> physical_status should always contain the correct physical status. So >> the fix should go into ast_dp_connector_helper_detect_ctx(). There's >> [1] something like > > If a DP monitor is connected, but the EDID is not readable, defaulting > to 1024x768 is still a good choice. If the EDID is not readable, we should assume that no monitor is connected. For this case, ast already sets 1024x768 to optimize for the BMC. > > The default to 640x480 is only to comply with the DP specification, > but in practice some DP monitors doesn't support 640x480. > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e7c254d75d16b75abf1958095fd34e2ecdc0d645 > If the defaults don't work for a certain system, users can force other resolutions via the kernel's command line. This is definitely not something that DRM drivers should address. Best regards Thomas > >> >> if (ast_dp_status_is_connected(ast)) >> status = connected >> >> and that's where it should read the EDID without updating the >> connector's EDID property. Example code: >> >> if (ast_dp_status_is_connected(ast)) { >> edid = drm_edid_read_custom(/* like in get_modes */) >> if (drm_edid_valid(edid)) >> status = connected >> drm_edid_free(edid) >> } >> >> The EDID test could also go into _is_connected() directly. A comment >> about false positives from VGACRDF might make sense as well. >> >> [1] >> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ast/ >> ast_dp.c#L397 >> >>> { >>> struct ast_connector *ast_connector = >>> to_ast_connector(connector); >>> + struct ast_device *ast = to_ast_device(connector->dev); >>> + const struct drm_edid *drm_edid = NULL; >>> int count; >>> - if (ast_connector->physical_status == >>> connector_status_connected) { >>> - struct ast_device *ast = to_ast_device(connector->dev); >>> - const struct drm_edid *drm_edid; >>> - >>> + if (ast_connector->physical_status == connector_status_connected) >>> drm_edid = drm_edid_read_custom(connector, >>> ast_astdp_read_edid_block, ast); >>> - drm_edid_connector_update(connector, drm_edid); >>> + >>> + drm_edid_connector_update(connector, drm_edid); >>> + >>> + if (drm_edid) { >>> count = drm_edid_connector_add_modes(connector); >>> drm_edid_free(drm_edid); >>> } else { >>> - drm_edid_connector_update(connector, NULL); >>> - >>> /* >>> * There's no EDID data without a connected monitor. Set BMC- >>> * compatible modes in this case. The XGA default resolution >>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ >>> ast_dp501.c >>> index 9e19d8c17730..c92db65e3f20 100644 >>> --- a/drivers/gpu/drm/ast/ast_dp501.c >>> +++ b/drivers/gpu/drm/ast/ast_dp501.c >> >> I'd rather leave this out. The detection works differently for DP501. > > ast_dp501_is_connected() hasn't changed, so yes I can drop this part. > > Best regards, >
> > > Link training is only to be done at atomic_enable or _check time. I > specifically asked about this back then. Apologies, I meant only at _enable time BUT NOT _check. > > But having said that, ast doesn't really do Link Training; it just > reads a bit from the VBIOS, which does it automatically. So it likely > doesn't matter for ast. So yeah, maybe add it back if that works. Plus > a comment that HPD alone is unreliable. I think this shoudl go into > stable as well. > > Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during > atomic_enable") > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Jocelyn Falempe <jfalempe@redhat.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: dri-devel@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v6.12+ > >> >>> >>>> This makes the default >>>> BMC resolution to be 640x480 for remote access. >>>> So consider that if there is no edid, no monitor is connected, and >>>> add the BMC 1024x768 default resolution. >>>> I've debugged this regression on ast_dp, but as dp501 is similar, I >>>> fixed both in this patch. >>>> >>>> This regression was likely introduced by commit 2281475168d2 >>>> ("drm/ast: astdp: Perform link training during atomic_enable") >>>> But I fixed it in the BMC get_modes handling. >>>> >>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>>> Fixes: bbad0090b9f4 ("drm/ast: astdp: Transparently handle BMC >>>> support") >>>> --- >>>> drivers/gpu/drm/ast/ast_dp.c | 14 +++++++------- >>>> drivers/gpu/drm/ast/ast_dp501.c | 14 +++++++------- >>>> 2 files changed, 14 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/ast/ast_dp.c >>>> b/drivers/gpu/drm/ast/ast_dp.c >>>> index 0e282b7b167c..6c8ea95a2230 100644 >>>> --- a/drivers/gpu/drm/ast/ast_dp.c >>>> +++ b/drivers/gpu/drm/ast/ast_dp.c >>>> @@ -361,19 +361,19 @@ static const struct drm_encoder_helper_funcs >>>> ast_astdp_encoder_helper_funcs = { >>>> static int ast_astdp_connector_helper_get_modes(struct >>>> drm_connector *connector) >>> >>> I don't think this is the right place to fix the problem. The field >>> physical_status should always contain the correct physical status. >>> So the fix should go into ast_dp_connector_helper_detect_ctx(). >>> There's [1] something like >> >> If a DP monitor is connected, but the EDID is not readable, >> defaulting to 1024x768 is still a good choice. > > If the EDID is not readable, we should assume that no monitor is > connected. For this case, ast already sets 1024x768 to optimize for > the BMC. > >> >> The default to 640x480 is only to comply with the DP specification, >> but in practice some DP monitors doesn't support 640x480. >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e7c254d75d16b75abf1958095fd34e2ecdc0d645 >> > > If the defaults don't work for a certain system, users can force other > resolutions via the kernel's command line. This is definitely not > something that DRM drivers should address. > > Best regards > Thomas > >> >>> >>> if (ast_dp_status_is_connected(ast)) >>> status = connected >>> >>> and that's where it should read the EDID without updating the >>> connector's EDID property. Example code: >>> >>> if (ast_dp_status_is_connected(ast)) { >>> edid = drm_edid_read_custom(/* like in get_modes */) >>> if (drm_edid_valid(edid)) >>> status = connected >>> drm_edid_free(edid) >>> } >>> >>> The EDID test could also go into _is_connected() directly. A comment >>> about false positives from VGACRDF might make sense as well. >>> >>> [1] >>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ast/ >>> ast_dp.c#L397 >>> >>>> { >>>> struct ast_connector *ast_connector = >>>> to_ast_connector(connector); >>>> + struct ast_device *ast = to_ast_device(connector->dev); >>>> + const struct drm_edid *drm_edid = NULL; >>>> int count; >>>> - if (ast_connector->physical_status == >>>> connector_status_connected) { >>>> - struct ast_device *ast = to_ast_device(connector->dev); >>>> - const struct drm_edid *drm_edid; >>>> - >>>> + if (ast_connector->physical_status == connector_status_connected) >>>> drm_edid = drm_edid_read_custom(connector, >>>> ast_astdp_read_edid_block, ast); >>>> - drm_edid_connector_update(connector, drm_edid); >>>> + >>>> + drm_edid_connector_update(connector, drm_edid); >>>> + >>>> + if (drm_edid) { >>>> count = drm_edid_connector_add_modes(connector); >>>> drm_edid_free(drm_edid); >>>> } else { >>>> - drm_edid_connector_update(connector, NULL); >>>> - >>>> /* >>>> * There's no EDID data without a connected monitor. Set >>>> BMC- >>>> * compatible modes in this case. The XGA default resolution >>>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ >>>> ast_dp501.c >>>> index 9e19d8c17730..c92db65e3f20 100644 >>>> --- a/drivers/gpu/drm/ast/ast_dp501.c >>>> +++ b/drivers/gpu/drm/ast/ast_dp501.c >>> >>> I'd rather leave this out. The detection works differently for DP501. >> >> ast_dp501_is_connected() hasn't changed, so yes I can drop this part. >> >> Best regards, >> >
On 24/01/2025 13:16, Thomas Zimmermann wrote: > Hi > > > Am 24.01.25 um 11:04 schrieb Jocelyn Falempe: >> On 24/01/2025 10:22, Thomas Zimmermann wrote: >>> Hi Jocelyn >>> >>> >>> Am 24.01.25 um 09:45 schrieb Jocelyn Falempe: >>>> The physical_status of ast_dp is not reliable, as it reports as >>>> connected even when no monitor is connected. >>> >>> This status comes from VGACRDF, which is undocumented unfortunately. >>> So IDK the exact semantics. Do you know if the other case can also >>> happen where a connected monitor is not reported? >> >> Previously, astdp_is_connected() also checked for ASTDP_LINK_SUCCESS >> https://elixir.bootlin.com/linux/v6.10.14/source/drivers/gpu/drm/ast/ >> ast_dp.c#L16 >> >> which used to work more reliably, se maybe I can add it back? > > That's a bit of a complicated issue. Protocol-wise Link Training runs > only after HPD, EDID and DPCD. A nice overview is at > > https://www.ti.com/content/dam/videos/external-videos/en- > us/8/3816841626001/6275649988001.mp4/subassets/understanding-dp-link- > training-presentation-quiz.pdf Thanks, I don't know much about the internal of DP protocol, but I though reading a register is faster than the whole EDID. > > Link training is only to be done at atomic_enable or _check time. I > specifically asked about this back then. > > But having said that, ast doesn't really do Link Training; it just reads > a bit from the VBIOS, which does it automatically. So it likely doesn't > matter for ast. So yeah, maybe add it back if that works. Plus a comment > that HPD alone is unreliable. I think this shoudl go into stable as well. ok, thanks a lot, I will send a v2 with this. Best regards,
diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c index 0e282b7b167c..6c8ea95a2230 100644 --- a/drivers/gpu/drm/ast/ast_dp.c +++ b/drivers/gpu/drm/ast/ast_dp.c @@ -361,19 +361,19 @@ static const struct drm_encoder_helper_funcs ast_astdp_encoder_helper_funcs = { static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector) { struct ast_connector *ast_connector = to_ast_connector(connector); + struct ast_device *ast = to_ast_device(connector->dev); + const struct drm_edid *drm_edid = NULL; int count; - if (ast_connector->physical_status == connector_status_connected) { - struct ast_device *ast = to_ast_device(connector->dev); - const struct drm_edid *drm_edid; - + if (ast_connector->physical_status == connector_status_connected) drm_edid = drm_edid_read_custom(connector, ast_astdp_read_edid_block, ast); - drm_edid_connector_update(connector, drm_edid); + + drm_edid_connector_update(connector, drm_edid); + + if (drm_edid) { count = drm_edid_connector_add_modes(connector); drm_edid_free(drm_edid); } else { - drm_edid_connector_update(connector, NULL); - /* * There's no EDID data without a connected monitor. Set BMC- * compatible modes in this case. The XGA default resolution diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 9e19d8c17730..c92db65e3f20 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -504,19 +504,19 @@ static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = { static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector) { struct ast_connector *ast_connector = to_ast_connector(connector); + struct ast_device *ast = to_ast_device(connector->dev); + const struct drm_edid *drm_edid = NULL; int count; - if (ast_connector->physical_status == connector_status_connected) { - struct ast_device *ast = to_ast_device(connector->dev); - const struct drm_edid *drm_edid; - + if (ast_connector->physical_status == connector_status_connected) drm_edid = drm_edid_read_custom(connector, ast_dp512_read_edid_block, ast); - drm_edid_connector_update(connector, drm_edid); + + drm_edid_connector_update(connector, drm_edid); + + if (drm_edid) { count = drm_edid_connector_add_modes(connector); drm_edid_free(drm_edid); } else { - drm_edid_connector_update(connector, NULL); - /* * There's no EDID data without a connected monitor. Set BMC- * compatible modes in this case. The XGA default resolution
The physical_status of ast_dp is not reliable, as it reports as connected even when no monitor is connected. This makes the default BMC resolution to be 640x480 for remote access. So consider that if there is no edid, no monitor is connected, and add the BMC 1024x768 default resolution. I've debugged this regression on ast_dp, but as dp501 is similar, I fixed both in this patch. This regression was likely introduced by commit 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable") But I fixed it in the BMC get_modes handling. Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> Fixes: bbad0090b9f4 ("drm/ast: astdp: Transparently handle BMC support") --- drivers/gpu/drm/ast/ast_dp.c | 14 +++++++------- drivers/gpu/drm/ast/ast_dp501.c | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) base-commit: 798047e63ac970f105c49c22e6d44df901c486b2