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

[Jocelyn Falempe]

On 04/07/2023 16:54, Jani Nikula wrote:
> On Fri, 23 Jun 2023, Jocelyn Falempe <jfalempe at 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.

> 
>> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
>> Signed-off-by: Jocelyn Falempe <jfalempe at 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.

-- 

Jocelyn
> 
> 
> BR,
> Jani.
> 
>> +		list_for_each_entry(mode, &connector->modes, head) {
>> +			if (mode->hdisplay == 1024 && mode->vdisplay == 768)
>> +				mode->type |= DRM_MODE_TYPE_PREFERRED;
>> +			drm_mode_sort(&connector->modes);
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +
>>   static const struct drm_connector_funcs ast_dp501_connector_funcs = {
>>   	.reset = drm_atomic_helper_connector_reset,
>> -	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.fill_modes = ast_dp_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,
>> @@ -1678,7 +1700,7 @@ static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs
>>   
>>   static const struct drm_connector_funcs ast_astdp_connector_funcs = {
>>   	.reset = drm_atomic_helper_connector_reset,
>> -	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.fill_modes = ast_dp_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,
>>
>> base-commit: 0adec22702d497385dbdc52abb165f379a00efba
>