Re: [PATCH] drm/ast: Fix default resolution when no monitor is connected on DP

From: Jocelyn Falempe <jfalempe@redhat.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
	tzimmermann@suse.de, airlied@redhat.com,
	kuohsiang_chou@aspeedtech.com, jammy_huang@aspeedtech.com
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/ast: Fix default resolution when no monitor is connected on DP
Date: Thu, 6 Jul 2023 11:16:33 +0200	[thread overview]
Message-ID: <ed075f2f-861d-74d1-efc0-5baa2cd601fd@redhat.com> (raw)
In-Reply-To: <13e35996-914d-37a6-1697-ac0c3c75cad1@redhat.com>

On 04/07/2023 18:45, Jocelyn Falempe wrote:
> On 04/07/2023 16:54, Jani Nikula wrote:
>> On Fri, 23 Jun 2023, Jocelyn Falempe <jfalempe@redhat.com> wrote:
>>> 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" resolution to 640x480, 
>>> which is
>>> inconvenient, and breaks the anaconda installer.
>>> So when no EDID is present, set 1024x768 as preferred resolution.
>>
>> This conflates "monitor connected" and "EDID present", which are not
>> necessarily the same thing.
>>
>> The fallback in drm_helper_probe_single_connector_modes() is for no
>> modes, but connector status is connected or unknown.
> 
> When debugging the issue, I found it surprising that the status is 
> "connected" when nothing is plugged in the DP port.
>>
>> You could add a connector ->detect callback that returns disconnected
>> when there's no display, and the problem should go away. If there's no
>> ->detect callback, it'll default to connected.
> 
> ok, I'll try that. I don't know how the hardware detect something is 
> connected, but looking at the dp code, maybe checking 
> "AST_IO_CRTC_PORT,0xDC, ASTDP_LINK_SUCCESS" would be good enough.

I've tested this approach, and it works. But on the server I'm testing, 
there are VGA and DP output. I think on a server that has only one DP 
output, if no monitor is connected, then no modes will be reported to 
userspace, and the remote BMC may not work ?

Also I don't have physical access to the server, so I only tested when 
no monitor is plugged.

I will send shortly a v2 with this change, so others can help me test 
this case.

Best regards,

-- 

Jocelyn

> 
>>
>>> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID 
>>> on DP")
>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>>> ---
>>>   drivers/gpu/drm/ast/ast_mode.c | 26 ++++++++++++++++++++++++--
>>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>>> b/drivers/gpu/drm/ast/ast_mode.c
>>> index 36374828f6c8..8f7b7cc021c7 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -1589,9 +1589,31 @@ static const struct drm_connector_helper_funcs 
>>> ast_dp501_connector_helper_funcs
>>>       .get_modes = ast_dp501_connector_helper_get_modes,
>>>   };
>>> +static int ast_dp_probe_single_connector_modes(struct drm_connector 
>>> *connector,
>>> +                           uint32_t maxX, uint32_t maxY)
>>> +{
>>> +    int ret;
>>> +    struct drm_display_mode *mode;
>>> +
>>> +    ret = drm_helper_probe_single_connector_modes(connector, maxX, 
>>> maxY);
>>> +    /*
>>> +     * When no monitor are detected, DP now default to 640x480
>>> +     * As aspeed is mostly used in remote server, and DP monitors are
>>> +     * rarely attached, it's better to default to 1024x768
>>> +     */
>>> +    if (!connector->edid_blob_ptr) {
>>
>> Please try not to use connector->edid_blob_ptr for anything in
>> drivers. The logic is complicated enough as it is, with the firmware and
>> override EDIDs and everything, and makes future refactoring of EDID
>> handling harder.
> 
> Ok, I will try your other suggestion, and remove this.
> 
> Thanks a lot for your comments.
> 