Re: [PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink

[Melissa Wen <mwen@igalia.com>]

On 13/06/2025 15:23, Mario Limonciello wrote:
> On 6/13/2025 12:42 PM, Melissa Wen wrote:
>> On 06/13, Mario Limonciello wrote:
>>> On 6/13/2025 7:58 AM, Melissa Wen wrote:
>>>> Add Linux opaque object to dc_sink for storing edid data cross driver,
>>>> drm_edid. Also include the Linux call to free this object, the
>>>> drm_edid_free()
>>>>
>>>> v3:
>>>> - remove uneccessary include (jani)
>>>>
>>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 6 ++++++
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 1 +
>>>>    drivers/gpu/drm/amd/display/dc/core/dc_sink.c   | 3 +++
>>>>    drivers/gpu/drm/amd/display/dc/dc.h             | 1 +
>>>>    include/drm/drm_edid.h                          | 4 ++--
>>>>    5 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>> index a90545b176cc..9e86dc15557b 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>> @@ -1,6 +1,7 @@
>>>>    // SPDX-License-Identifier: MIT
>>>>    #include "amdgpu_dm/dc_edid.h"
>>>>    #include "dc.h"
>>>> +#include <drm/drm_edid.h>
>>>>    bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>>>                  struct dc_sink *current_sink)
>>>> @@ -25,3 +26,8 @@ void dc_edid_copy_edid_to_dc(struct dc_sink 
>>>> *dc_sink,
>>>>        memmove(dc_sink->dc_edid.raw_edid, edid, len);
>>>>        dc_sink->dc_edid.length = len;
>>>>    }
>>>> +
>>>> +void dc_edid_sink_edid_free(struct dc_sink *sink)
>>>> +{
>>>> +    drm_edid_free(sink->drm_edid);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>> index f42cd5bbc730..2c76768be459 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>> @@ -9,5 +9,6 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>>>                  struct dc_sink *current_sink);
>>>>    void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>>>>                     const void *edid, int len);
>>>> +void dc_edid_sink_edid_free(struct dc_sink *sink);
>>>>    #endif /* __DC_EDID_H__ */
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c 
>>>> b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>>> index 455fa5dd1420..3774a3245506 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>>> @@ -26,6 +26,7 @@
>>>>    #include "dm_services.h"
>>>>    #include "dm_helpers.h"
>>>>    #include "core_types.h"
>>>> +#include "dc_edid.h"
>>>> /*******************************************************************************
>>>>     * Private functions
>>>> @@ -65,6 +66,8 @@ void dc_sink_retain(struct dc_sink *sink)
>>>>    static void dc_sink_free(struct kref *kref)
>>>>    {
>>>>        struct dc_sink *sink = container_of(kref, struct dc_sink, 
>>>> refcount);
>>>> +
>>>> +    dc_edid_sink_edid_free(sink);
>>>>        kfree(sink->dc_container_id);
>>>>        kfree(sink);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h 
>>>> b/drivers/gpu/drm/amd/display/dc/dc.h
>>>> index 86feef038de6..cf56a0405a4f 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dc.h
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
>>>> @@ -2466,6 +2466,7 @@ struct scdc_caps {
>>>>    struct dc_sink {
>>>>        enum signal_type sink_signal;
>>>>        struct dc_edid dc_edid; /* raw edid */
>>>> +    const struct drm_edid *drm_edid; /* Linux DRM edid*/
>>>
>>> Don't you need a forward declaration for 'struct drm_edid' in dc.h 
>>> to be
>>> able to do this?
>>
>> I understand that, as it's just a pointer (the compiler knows the size)
>> and there is no circular dependencies between dc_sink and drm_edid, we
>> don't need a forward declaration. So I think we are fine also because
>> dc_sink->drm_edid dereference only happens in dc_edid.h that already
>> needs to include drm_edid.h for drm_edid helpers... but let me know if
>> I'm missing something.
>
> Did you compile with CONFIG_WERROR or at least W=1?  I feel like I've 
> seen issues with this in the past that the compiler doesn't like having
> unknown types, even if a pointer.

Yeah, I don't see complaints here with the warning as error option enabled.
But let me know if you think forward declaration is safer or better for 
maintainance.
I just tried to reduce the number of linux-related stuff on DC as much 
as possible.

BR,

Melissa
>
> But if it works with W=1 / CONFIG_WERROR I must have been thinking 
> about a dereference case and thus no concerns on my side.
>
>>
>>>
>>> Also you're missing a space at the end of the comment before the '*/'.
>>
>> ack. I'll wait for more comments to send it fixed.
>>
>> Thanks for reviewing.
>>
>> Melissa
>>
>>>
>>>>        struct dc_edid_caps edid_caps; /* parse display caps */
>>>>        struct dc_container_id *dc_container_id;
>>>>        uint32_t dongle_max_pix_clk;
>>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>> index e7a9a4928b97..8617d2285f38 100644
>>>> --- a/include/drm/drm_edid.h
>>>> +++ b/include/drm/drm_edid.h
>>>> @@ -469,8 +469,8 @@ int drm_edid_connector_update(struct 
>>>> drm_connector *connector,
>>>>                      const struct drm_edid *edid);
>>>>    int drm_edid_connector_add_modes(struct drm_connector *connector);
>>>>    bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>>> -bool drm_edid_eq(const struct drm_edid *drm_edid_first,
>>>> -             const struct drm_edid *drm_edid_second);
>>>> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
>>>> +         const struct drm_edid *drm_edid_2);
>>>>    void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>>>                     struct drm_edid_product_id *id);
>>>>    void drm_edid_print_product_id(struct drm_printer *p,
>>>
>