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

[Mario Limonciello <mario.limonciello@amd.com>]

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.

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,
>>