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: <b81591c4-934c-4b70-fbf0-b0f4663a6d1c@amd.com>
Date:   Tue, 13 Sep 2022 09:49:07 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH v2] drm/plane-helper: Add a
 drm_plane_helper_atomic_check() helper
To:     Javier Martinez Canillas <javierm@redhat.com>,
        linux-kernel@vger.kernel.org
Cc:     Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org
References: <20220913093339.106625-1-javierm@redhat.com>
Content-Language: en-US
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <20220913093339.106625-1-javierm@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UUFQME1ma3poc3lYMUpOeDVLU1c0K0ZLNzlFdDRzRUp5SzlMendQcFlWMjI4?=
 =?utf-8?B?RVVKbitIbXRVV2g0RzNuMGdobWd1Y0lFSUVEakVLMEpqNzlRM1hXOXI4L0o4?=
 =?utf-8?B?YTBiYktBM0JHZTQ1LzBVVWhUVkhKRElPS0IvVURyVDNhVVBuT1VQTy8xMjVS?=
 =?utf-8?B?MGI5c2NvbTduL2JHVHEzYVFOTE8zd1B1RzhDOUdnaGRBYjN5VmovZ3M1bHUr?=
 =?utf-8?B?TFVwVS9FMlNpMWJhYThkWUxZS1dYQmk0NkZwWWE3TlNoQXR4OFNoYUo2UGww?=
 =?utf-8?B?V3p5SkVFeHc1a2IvUTErSEVORHA4SkJldzF6bm93N1lvSldQRUdSenU4L1Jt?=
 =?utf-8?B?STJTUnpNTS9DL2ttd2pXZldseHlCaEluYWVIbG04NEJ5RmdTdGtwUUtWSzhB?=
 =?utf-8?B?SWxSaUgwbzBaZHdJbXNTdGdmYzFZYkt1QXRTVTZkSGZvenJsM3hmZFRBQXp2?=
 =?utf-8?B?WFdVQ1VOQXZUSHRBaXp1UW9qcnlkK2g5UFZEakJjTVM1VWNuL1V2a1l3RmxO?=
 =?utf-8?B?NnRIb2lFK1I2WVZNU0g1cFZaKzROS2NISkxhNmZrVHdXMHY1UDRLQTR5WTRO?=
 =?utf-8?B?Q2JYNGVMaWMyYmpDcmxLbW1hTmI5ckRrMzQ0SW9HZTdWTFljelNYS3VOVS9j?=
 =?utf-8?B?cVZ3T1VHcHpTY1RtZk9qNXRvTzhBZzMvUXgxWW54TnczbVpZamZSY1VVYVd2?=
 =?utf-8?B?YkZaazU5eGc4cjhuNm9TN0lndnV1YlJjVGZUMDExTU9CbWRaQzdtcWJld3ZE?=
 =?utf-8?B?bGtnQjRpZmdzM0lKNXRpZGN3N0NHRCtudnNQUDFsbld5L3V6UEZMUWJQd01y?=
 =?utf-8?B?bFNlUW9laHFUZXo0YlZKczc2aE1zbU9jcytKdmFnNlRNSzBOQkN4bE1icW9F?=
 =?utf-8?B?SHJWalZHcHZqa2h1QnFhenBvM1BhaGZuWkF4OTdLMzgxZUFkaUhxZjFqbU9N?=
 =?utf-8?B?cmNiTGFCWjFPOHpDTmhSMk1EL3N5eVJwT0dZS0VIc1B0eGVYU1dTYVBZSVpX?=
 =?utf-8?B?SEdWbVQ4YUpVd2NnclRiLzFlZzkrM2NLdEVCVUdmTDlObE1ZQk9QWUJPYlIx?=
 =?utf-8?B?MlB0blhFL0UvOGlENXhzL2JMcnZ4enZYbnlCTFFVekxHemVBU1NlREN0R2Fq?=
 =?utf-8?B?UUFTZ25nUENmWUdwamdVenZtYktRMDJxRmRtaHVpTUdYQUhvcGFZbVdWZnFF?=
 =?utf-8?B?c3RRelBFOGszOXJkUXFnTkpBZFExM0Q4VFBWeVprZjNzU2Q5QjU0T25rUTFF?=
 =?utf-8?B?RmN1QmdmcG5KMG5jQ1ZxQ0lqSUhHMGVvWElXVkdKNFB1OGhCbFRlK1YwTFJO?=
 =?utf-8?B?aWxvUFFvTGU5R3RVc0hNL0FmUFE3Rk5OMDdUWE9McFhHeFM3bE8rYkNhTTVZ?=
 =?utf-8?B?UVVKRDlBY2ZTTXEzVjUwNVhjWVlkL09ST3lWUDQyeGlDVkxwbDVxODF0dUph?=
 =?utf-8?B?WlZCcFFKT2JMeXUveVczakIrZDVzeERVWUdzZi9Jcy8wcmZ6M0syYTZmOGNa?=
 =?utf-8?B?bm0zenhodTd3ZVhJdzg1M1dmcUM1Q3g5RWdQTU1yWmQvWllmWXdaM1Q4VllQ?=
 =?utf-8?B?aUlDNTNSKzhnK1BJcVkyaXF0SWtCN1lxN203czdPODhpRFdKNzlzWHdFeVUw?=
 =?utf-8?B?ZDlrbXozaUlMUzJVSFhzang0VVFzK1pqdGRoK1ozYXV1RjZDb0RnMyt3eWQ0?=
 =?utf-8?B?TXJpWnBadUdJQzU0TGUreFhIZmZmb3Zsdi9VSTR1NzdVOHM2VFRuTHY0cWRi?=
 =?utf-8?B?VlMrZzRlaUxqdktudmwrcXAyWjFLTFMyc3NzRHcwczNReldjN3RPdkluZnBo?=
 =?utf-8?B?TzcwOUYwSHU5UEZiWHdMV1AxQS9tZFdzcHl5T1ErWHRsT2NINnJKa3BSR0U4?=
 =?utf-8?B?VGV2YVFqTEFrVjhZNWp1NmM5dytUYy9YNlRPOEpCL0RNYldMM2JUcHF6eEVB?=
 =?utf-8?B?YytnT2JFNjdyejlLVXhqMk11c0FIT3FMTVp5Tit2RDl0bTJHMHQzb0VFNzVj?=
 =?utf-8?B?alZKWVh0eitia2Z2MG5JT01ROGhUdEVQNVhCeWZZeDJPdnZlWTdtZ2JVRE1R?=
 =?utf-8?B?V2RyQXR6TXhMYk5TQjdzZDhVdHd2d1B0U0dXamoxTXpYMXRNNFFSWFRYME4w?=
 =?utf-8?Q?snLRK8EnETImu+NAguS5qJcwR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 00f68b08-710c-4b97-6691-08da958ebc3d
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2022 13:49:10.4415
 (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: ZN/3QBRb5ZePA+4m19F++BTYca4KLNXJgvdZtjtQzjbn0ksf8CQPjk7VY3EX0uhJp9scHvb1H2g5mQ4PUt0iUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR12MB5506
X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,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 2022-09-13 05:33, Javier Martinez Canillas wrote:
> Provides a default plane state check handler for primary planes that are a
> fullscreen scanout buffer and whose state scale and position can't change.
> 

Even though this might be how some drivers are handling the primary
plane this assumption is not universal. For example, on ChromeOS when
the (default) underlay strategy is selected the compositor will
position the video plane as the bottom-most plane and mark it
as primary, while the desktop plane is marked as overlay and
will contain an alpha cutout to show the underlying video.

In this case the video plane (primary) won't span the screen
and is regularly panned and resized.

No objections to this change as it helps share code between drivers,
but maybe specify that this behavior is a unique requirement of
these drivers.

Harry

> There are some drivers that duplicate this logic in their helpers, such as
> simpledrm and ssd130x. Factor out this common code into a plane helper and
> make drivers use it.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v2:
> - Fix `new_state` field comment (Ville Syrjälä).
> - Rename `new_state` to just `state` (Ville Syrjälä).
> 
>  drivers/gpu/drm/drm_plane_helper.c | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>  drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>  include/drm/drm_plane_helper.h     |  1 +
>  4 files changed, 31 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index c7785967f5bf..2165390863f4 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -278,3 +278,31 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>  	kfree(plane);
>  }
>  EXPORT_SYMBOL(drm_plane_helper_destroy);
> +
> +/**
> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> + * @plane: plane to check
> + * @state: atomic state object
> + *
> + * Provides a default plane state check handler for primary planes whose atomic
> + * state scale and position is not expected to change because the primary plane
> + * is always a fullscreen scanout buffer.
> + *
> + * RETURNS:
> + * Zero on success, or an errno code otherwise.
> + */
> +int drm_plane_helper_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> +	struct drm_crtc_state *new_crtc_state = NULL;
> +
> +	if (new_crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
> +
> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						   DRM_PLANE_NO_SCALING,
> +						   DRM_PLANE_NO_SCALING,
> +						   false, false);
> +}
> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 79e8e2017c68..28cf9c87f86d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	return ret;
>  }
>  
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *new_state)
> -{
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *new_crtc_state = NULL;
> -
> -	if (new_crtc)
> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> -
> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> -						   DRM_PLANE_NO_SCALING,
> -						   DRM_PLANE_NO_SCALING,
> -						   false, false);
> -}
> -
>  static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  						       struct drm_atomic_state *old_state)
>  {
> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>  
>  static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>  	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>  	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>  	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>  };
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 777ccd250871..ea5b3239a659 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						       struct drm_atomic_state *new_state)
> -{
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *new_crtc_state = NULL;
> -	int ret;
> -
> -	if (new_crtc)
> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> -
> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> -						  DRM_PLANE_NO_SCALING,
> -						  DRM_PLANE_NO_SCALING,
> -						  false, false);
> -	if (ret)
> -		return ret;
> -	else if (!new_plane_state->visible)
> -		return 0;
> -
> -	return 0;
> -}
> -
>  static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  							 struct drm_atomic_state *old_state)
>  {
> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>  
>  static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
>  	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>  	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
>  	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
>  };
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 1781fab24dd6..7760b27b0323 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -41,5 +41,6 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>  int drm_plane_helper_disable_primary(struct drm_plane *plane,
>  				     struct drm_modeset_acquire_ctx *ctx);
>  void drm_plane_helper_destroy(struct drm_plane *plane);
> +int drm_plane_helper_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state);
>  
>  #endif