Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f8871b8f-af40-61df-7a77-a632015c9201@amd.com>
Date:   Mon, 8 May 2023 17:24:49 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
From:   Harry Wentland <harry.wentland@amd.com>
Subject: Re: [RFC PATCH 09/40] drm/amd/display: move replace blob func to dm
 plane
To:     Melissa Wen <mwen@igalia.com>, amd-gfx@lists.freedesktop.org,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        sunpeng.li@amd.com, Alex Deucher <alexander.deucher@amd.com>,
        dri-devel@lists.freedesktop.org, christian.koenig@amd.com,
        Xinhui.Pan@amd.com, airlied@gmail.com, daniel@ffwll.ch
Cc:     Joshua Ashton <joshua@froggi.es>,
        Sebastian Wick <sebastian.wick@redhat.com>,
        Xaver Hugl <xaver.hugl@gmail.com>,
        Shashank Sharma <Shashank.Sharma@amd.com>,
        Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>,
        sungjoon.kim@amd.com, Alex Hung <alex.hung@amd.com>,
        linux-kernel@vger.kernel.org
References: <20230423141051.702990-1-mwen@igalia.com>
 <20230423141051.702990-10-mwen@igalia.com>
Content-Language: en-US
In-Reply-To: <20230423141051.702990-10-mwen@igalia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djlZSldENjV2ZlZZWjFVZTVsYzZ4c1BiVnpVTWNMWXJsNlphakxRYm1wMCt6?=
 =?utf-8?B?Z0dCS2U2T3djSk9uSW41OVhiZ1VXbkpIWjJlVkc5dDdQWk54MS9uYnV0QnNP?=
 =?utf-8?B?TDV3OTdJd05xbStra3l5T0Z3VU5QWWVoWlhTTmhGOE1jMDcxR2s1Um1vSGgy?=
 =?utf-8?B?QXRFczFQZzlQeTBCVVNLWTZaUmtSYmd4aVNkOFBsL05nbWVwM1lHWmIrNlEr?=
 =?utf-8?B?UXo0T3BJeXI4aU1lOVpHbEFLbzdVbTMrLzhFK1hzT2RoQjliNmV6K1lxUUFq?=
 =?utf-8?B?c2hsZTkxYVU0RWdLZEttMkpNMWVBUnloNGFGM0d0WE1VSXh4dHJYZDVRTVpN?=
 =?utf-8?B?NVhjSjEwWXkzWUhpcmN2Y3BpbWZtcGtRYVRra1BKcm5JaTlMTG1oVEpscm9D?=
 =?utf-8?B?UndnWmpwQ25BRUVqZjdIazd2RVFrODlzWnYxdngraTl2ZGV0cGNhTkFySVIz?=
 =?utf-8?B?YVg1ZUlkZXNCQ0Rta2dla2dqZmM3amJJSEUrM3VXN1p3Q1h6TzhacW5Rdi81?=
 =?utf-8?B?V1dxbVlSN2ZGMk01NXAvMUxWV2dWMFB4UFhsWHVWay8wMXM0ZmJvMDRpQzVV?=
 =?utf-8?B?R3NpWmthaU5oU2JWME40cWNtdm9IbkVONGlZN25YODhyS09PdzRtWFNzbHFN?=
 =?utf-8?B?ZXFPQ0QrZDRFMUJTVGJrRGhpMnBKRURTUlRJd0ZNUGpCeWZlVytZZDZpVEZu?=
 =?utf-8?B?UVhKUFZXZjRHdFFXWjBZdldmNUw3cTZvWnQ4bUlWSGtsSmNnbmpXbXBOeEI0?=
 =?utf-8?B?S1RkbkloTVdyREk0TUtPWGx0UGZxNzFSbzh1YXN1a2hIK1JPRURsM1JnWkFF?=
 =?utf-8?B?QmdCVDBmeWVPMSs4MitpTDhDRERWYW9uZ3BKUmV6dVJtM0FnL291NzdUQzF2?=
 =?utf-8?B?K1RSRzZkb29haktXbHZCMFkvZ3EyOFd0anEyOEsrd0NyZWhTRzN4bEJiT2RS?=
 =?utf-8?B?WFJCYUdNelhyN1gxTVFMM3JxcUVvWEl5THR3eTBzbVJnSzFuaVNBSm9QMW8v?=
 =?utf-8?B?c2UzSldYWm5HZElPMWVkZEl4N29zdmVYcFBhZG5ZQy9lVDFoWFNhL0JvN29h?=
 =?utf-8?B?NVNUNGxoRGxHaUdGSWYxMldGdHJNeEFmdEdLNGNiSVZWTzc5UjZBUndFazdL?=
 =?utf-8?B?UmVrZ2d2aHQrSHZuekIyUFQrV1d1dGJ2b0ExeGVVbDNxc2R1dWZlcHlhRHN5?=
 =?utf-8?B?NmcyakpJR2o5SURjVGswTmdMdDdyR2NyQkRNYjNaVTQ4bXJ4OFVvL0hRUzRt?=
 =?utf-8?B?emtEanpjZmNwRURkTCt0djJLWVNGYzJxZmpNdVR3YjhYZFJ5R3ArQ1ZOOUg3?=
 =?utf-8?B?eisvU1VvM21hRXVUUmhlYkp4Y3IvdUJhbGMvaE1FQ281UEtOTEQ0N2czaTd1?=
 =?utf-8?B?VzBrTXU2c1pvekJUb0hPR3NTaThjL05tbFA4ZVdEdzZHM0RxdklxVllwUWFY?=
 =?utf-8?B?eVU1bkFHdXU1Y0FNU1pUNXh2U0JKaFA2WWNSZGE4Y292emhWdjd1ZmJoREc0?=
 =?utf-8?B?V0czaGRCSk5tL04zQXIvblhaUWluQ09YNlc3eTZtV0NiWlZZRHBsVkJBc3ZG?=
 =?utf-8?B?dEV2cmJwTW9hMWlMdTNHOUNncGpBYzE0MThPRm9EcnJJOTNVNWp0U0ZYY3hN?=
 =?utf-8?B?VXRwWXBpVzRsTElneFlEd2xZY2tJSTNZVnZYQlA3Sk90eVEwaVZ6QXRsNGQr?=
 =?utf-8?B?b2tEMGJvaUwzeTduaUNLUldMdHFVMWErUmZhTU5DSTJ5L0lsVk9ESStHQWc3?=
 =?utf-8?B?dGkzbTBxd01NMkdHWlovOTdBaGp1aVVHNkdSeGpyTm15TVJUL0ZXck8rbTBM?=
 =?utf-8?B?MUJDb0Y3QkxKQ2RlQ2dHdytPZUZFUnl3dlFsc2x5MkppQkRyQlk0RWY1NFlz?=
 =?utf-8?B?MUptMlRlVHljdi9NYzdpUFRXVjVjd2ZjRmV4SlRFcUlrcWNrMVFhMnpkRkcr?=
 =?utf-8?B?TUoxWTN4OVljTE1USUNOSVBrTElNR3M5TWVVd3RzM1JqYlBsN29aeHZRa2s0?=
 =?utf-8?B?b291czhWMXVJWG1JMmkvajl0NU1BWGNTbkV6Qld4MFFTcjJBMTBUdUZMTlVD?=
 =?utf-8?B?NUJUKzF1YTliK0xZcFppamI4VUphYmxWR1pYSWFEUHIzTmowSHZyUnhlZ3JV?=
 =?utf-8?Q?+zkuOQVBrDu5FpJt805pQhZBa?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85444088-5448-47f9-2240-08db500aaa65
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 May 2023 21:24:54.3745
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: S4xSWn8vSqYCyVZbI3GKnREy76bIqTCGjTQi7Cz1OoHg9TyoRUiUoSN1lAoqy8h+THBivvzz9YWfZkcM179C1w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5269
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/23/23 10:10, Melissa Wen wrote:
> From amdgpu_dm_plane we can get it for both CRTC and plane color
> properties. We are adding new plane properties for AMD driver-private
> color mgmt.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 37 +------------------
>  .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 35 ++++++++++++++++++
>  .../amd/display/amdgpu_dm/amdgpu_dm_plane.h   |  7 ++++
>  3 files changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> index 79324fbab1f1..27d7a8b18013 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> @@ -344,39 +344,6 @@ dm_crtc_additional_color_mgmt(struct drm_crtc *crtc)
>  					   DRM_TRANSFER_FUNCTION_DEFAULT);
>  }
>  
> -static int
> -atomic_replace_property_blob_from_id(struct drm_device *dev,
> -					 struct drm_property_blob **blob,
> -					 uint64_t blob_id,
> -					 ssize_t expected_size,
> -					 ssize_t expected_elem_size,
> -					 bool *replaced)
> -{
> -	struct drm_property_blob *new_blob = NULL;
> -
> -	if (blob_id != 0) {
> -		new_blob = drm_property_lookup_blob(dev, blob_id);
> -		if (new_blob == NULL)
> -			return -EINVAL;
> -
> -		if (expected_size > 0 &&
> -		    new_blob->length != expected_size) {
> -			drm_property_blob_put(new_blob);
> -			return -EINVAL;
> -		}
> -		if (expected_elem_size > 0 &&
> -		    new_blob->length % expected_elem_size != 0) {
> -			drm_property_blob_put(new_blob);
> -			return -EINVAL;
> -		}
> -	}
> -
> -	*replaced |= drm_property_replace_blob(blob, new_blob);
> -	drm_property_blob_put(new_blob);
> -
> -	return 0;
> -}
> -
>  static int
>  amdgpu_dm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *state,
> @@ -389,7 +356,7 @@ amdgpu_dm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  	int ret;
>  
>  	if (property == adev->mode_info.shaper_lut_property) {
> -		ret = atomic_replace_property_blob_from_id(crtc->dev,
> +		ret = amdgpu_dm_replace_property_blob_from_id(crtc->dev,
>  					&acrtc_state->shaper_lut,
>  					val,
>  					-1, sizeof(struct drm_color_lut),
> @@ -397,7 +364,7 @@ amdgpu_dm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		acrtc_state->base.color_mgmt_changed |= replaced;
>  		return ret;
>  	} else if (property == adev->mode_info.lut3d_property) {
> -		ret = atomic_replace_property_blob_from_id(crtc->dev,
> +		ret = amdgpu_dm_replace_property_blob_from_id(crtc->dev,
>  					&acrtc_state->lut3d,
>  					val,
>  					-1, sizeof(struct drm_color_lut),
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 322668973747..4e5498153be2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1411,6 +1411,41 @@ static void dm_drm_plane_destroy_state(struct drm_plane *plane,
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> +#ifdef CONFIG_STEAM_DECK
> +int
> +amdgpu_dm_replace_property_blob_from_id(struct drm_device *dev,
> +					       struct drm_property_blob **blob,
> +					       uint64_t blob_id,
> +					       ssize_t expected_size,
> +					       ssize_t expected_elem_size,
> +					       bool *replaced)
> +{
> +	struct drm_property_blob *new_blob = NULL;
> +
> +	if (blob_id != 0) {
> +		new_blob = drm_property_lookup_blob(dev, blob_id);
> +		if (new_blob == NULL)
> +			return -EINVAL;
> +
> +		if (expected_size > 0 &&
> +		    new_blob->length != expected_size) {
> +			drm_property_blob_put(new_blob);
> +			return -EINVAL;
> +		}
> +		if (expected_elem_size > 0 &&
> +		    new_blob->length % expected_elem_size != 0) {
> +			drm_property_blob_put(new_blob);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	*replaced |= drm_property_replace_blob(blob, new_blob);
> +	drm_property_blob_put(new_blob);
> +
> +	return 0;
> +}

amdgpu_dm_plane doesn't seem the right place for it either. Maybe a new
amdgpu_dm_helper.c/h?

Alternatively would this make sense to live in DRM core as a helper?

Harry

> +#endif
> +
>  static const struct drm_plane_funcs dm_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h
> index 930f1572f898..1b05ac4c15f6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h
> @@ -51,6 +51,13 @@ int amdgpu_dm_plane_fill_plane_buffer_attributes(struct amdgpu_device *adev,
>  				 bool tmz_surface,
>  				 bool force_disable_dcc);
>  
> +int amdgpu_dm_replace_property_blob_from_id(struct drm_device *dev,
> +					    struct drm_property_blob **blob,
> +					    uint64_t blob_id,
> +					    ssize_t expected_size,
> +					    ssize_t expected_elem_size,
> +					    bool *replaced);
> +
>  int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  			 struct drm_plane *plane,
>  			 unsigned long possible_crtcs,