From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EB997C71159 for ; Mon, 16 Jun 2025 19:13:10 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4D0DC10E28A; Mon, 16 Jun 2025 19:13:10 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=igalia.com header.i=@igalia.com header.b="gS15LYbB"; dkim-atps=neutral Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) by gabe.freedesktop.org (Postfix) with ESMTPS id 094FB10E28A; Mon, 16 Jun 2025 19:13:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=33k/FvPvMaC+hf6YPX4/vljLuOlQN/BmgQAwGZPy+g8=; b=gS15LYbBuC+HZ2Vav8uE/GGJng pLDvPWCuey5EcyjrqAIlpS0vwvHZ6SERuOo1vjy66/NOP8xETaFmlvBdXbIFH77Gba0JryQOW5wd1 +jAa0KT23h+3aRbEv6bFsJSlWYdGrpithfkdozKlpawOypur21IDDkOeojhUvBaT1yJOIoAfFIJ8D CGl+77RRLvfCSyV2zB2krHzyDg/vqgNBU/gVPJ62w5hXxBk77YEdV0HIOhlKfj768Buf/1Kb8X+GP VWeDwCqmvnmThQl/kUXiL+AC2j27ttCglYTYK+E/glR7YC2Veicgh+0x1Gz4hS7x1jiTU8MhVHxCh t1yyqt5Q==; Received: from [189.6.13.79] (helo=[192.168.0.55]) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_128_GCM:128) (Exim) id 1uRFGJ-004FqP-Hm; Mon, 16 Jun 2025 21:12:55 +0200 Message-ID: Date: Mon, 16 Jun 2025 16:12:48 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink To: Mario Limonciello Cc: Alex Hung , Rodrigo Siqueira , harry.wentland@amd.com, sunpeng.li@amd.com, alexander.deucher@amd.com, christian.koenig@amd.com, airlied@gmail.com, simona@ffwll.ch, maarten.lankhorst@linux.intel.com, mripard@kernel.org, tzimmermann@suse.de, Jani Nikula , Michel Daenzer , amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, kernel-dev@igalia.com References: <20250613150015.245917-1-mwen@igalia.com> <20250613150015.245917-14-mwen@igalia.com> <4e5b6615-6ef6-41fa-8794-e274d509b37a@amd.com> Content-Language: en-US From: Melissa Wen In-Reply-To: <4e5b6615-6ef6-41fa-8794-e274d509b37a@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 >>>> --- >>>>    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 >>>>    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, >>> >