Re: [PATCH 4/4] radeon: fall back to ACPI EDID retrieval

[Daniel Dadap <ddadap-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 7/28/20 1:50 AM, Christian König wrote:
>
> Am 27.07.20 um 22:53 schrieb Daniel Dadap:
>> Fall back to retrieving the EDID via the ACPI _DDC method, when present
>> for notebook internal panels, when retrieving BIOS-embedded EDIDs.
>>
>> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon_combios.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_combios.c 
>> b/drivers/gpu/drm/radeon/radeon_combios.c
>> index c3e49c973812..de801d9fca54 100644
>> --- a/drivers/gpu/drm/radeon/radeon_combios.c
>> +++ b/drivers/gpu/drm/radeon/radeon_combios.c
>> @@ -401,9 +401,8 @@ bool radeon_combios_check_hardcoded_edid(struct 
>> radeon_device *rdev)
>>   struct edid *
>>   radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
>>   {
>> -     struct edid *edid;
>> -
>>       if (rdev->mode_info.bios_hardcoded_edid) {
>> +             struct edid *edid;
>
> That's an unrelated an incorrect style change. You need a blank line
> after declaration.


Ah, yes, that doesn't really need to be changed. I'll remove it from 
this patch. Would a separate patch to change the scope of that 
declaration (with a blank line after) be welcome, or should I just leave 
it alone?


>
>>               edid = 
>> kmalloc(rdev->mode_info.bios_hardcoded_edid_size, GFP_KERNEL);
>>               if (edid) {
>>                       memcpy((unsigned char *)edid,
>> @@ -412,7 +411,8 @@ radeon_bios_get_hardcoded_edid(struct 
>> radeon_device *rdev)
>>                       return edid;
>>               }
>>       }
>> -     return NULL;
>> +
>> +     return drm_get_edid_acpi();
>
> In general a good idea, but I'm wondering if we should really do this so
> unconditionally here.
>

I'm not personally aware of any AMD notebook designs that require the 
ACPI _DDC EDID retrieval. I've only seen it on NVIDIA+Intel hybrid 
systems and on a small number of NVIDIA discrete-only systems. I just 
figured I'd update the radeon DRM-KMS driver while updating i915 and 
Nouveau, for completeness, as it could be helpful should such a design 
exist. As for whether there should be some condition around this, I 
suppose that's reasonable, but I'm not really sure what would make sense 
as a condition. As it stands, drm_edid_acpi() only returns a value if at 
least one of the VGA or 3D controllers on the system provides an ACPI 
_DDC method, and if that ACPI method successfully returns an EDID.

On the caller's end, it's currently part of the path where the radeon 
driver is already trying to fall back to a hardcoded EDID provided by 
the system. Perhaps instead if we call it within the LVDS || eDP 
condition here, instead?


         if (rdev->is_atom_bios) {
             /* some laptops provide a hardcoded edid in rom for LCDs */
             if (((connector->connector_type == DRM_MODE_CONNECTOR_LVDS) ||
                  (connector->connector_type == DRM_MODE_CONNECTOR_eDP)))
                 radeon_connector->edid = 
radeon_bios_get_hardcoded_edid(rdev);
         } else {
             /* some servers provide a hardcoded edid in rom for KVMs */
             radeon_connector->edid = radeon_bios_get_hardcoded_edid(rdev);
}

That would be more in line with the changes in this patchset for i915 
and nouveau.


> Regards,
> Christian.
>
>>   }
>>
>>   static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct 
>> radeon_device *rdev,
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx