Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <a9503013-989f-47c7-a879-e862868ea23d@amd.com>
Date:   Tue, 26 Sep 2023 11:25:41 -0400
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC v6 00/10] Support for Solid Fill Planes
Content-Language: en-US
To:     Jessica Zhang <quic_jesszhan@quicinc.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Clark <robdclark@gmail.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Sean Paul <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>
Cc:     sebastian.wick@redhat.com, quic_abhinavk@quicinc.com,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        ppaalanen@gmail.com, laurent.pinchart@ideasonboard.com,
        linux-arm-msm@vger.kernel.org, wayland-devel@lists.freedesktop.org,
        freedreno@lists.freedesktop.org
References: <20230828-solid-fill-v6-0-a820efcce852@quicinc.com>
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <20230828-solid-fill-v6-0-a820efcce852@quicinc.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?dE9wSkVKOTh4eU5hcDRYREltSkVGUFdRK241Ukw1c0hhdkVkdkFqZWR0UUZq?=
 =?utf-8?B?TklHVGdmUDA0MWZnT0hJV2JlNHNzL0M1RkZjK25rSnVNMzZ1dlI4WUxKQzla?=
 =?utf-8?B?SkJLcm1GTXhsL3I1RUpDdWhWcnprZWNtcjBybkM5RWY1UWMrd2xvakFJT04y?=
 =?utf-8?B?SHFWVzdNeTRjY0xDVmgyaGRpQWo3VEUvTnowK3Rodkt4K1ZJTVdVeEhzTU1T?=
 =?utf-8?B?Z2RLTDAvN3pYRkE0ZWJibklTUEJ3dDZoZytiWEt1MjF0TFFvNW5sTDVDTzIr?=
 =?utf-8?B?TTFFSFFUZEVqSU5Gb2pkUWFIRTZmRnhIdG1NRTVRZytUVEZ0QTF0QUFwVVVU?=
 =?utf-8?B?cDVxREZTRnBsZW9Jd055eVBNOHI0SUMvMHFyeEJkbzJtbFBBQWpLT1hPbXQy?=
 =?utf-8?B?UXVYSlFmZTUveHZod0oySVBTVDRuZDZ5Rkp4dzRNZ1IwK3RYTFdIaWl3ay9l?=
 =?utf-8?B?TGx5UCtrNlhHK3g4RXk4UFI2Tk9nUWlGaVh6VXRZMnJLWmpVVGFXbkYyeFJa?=
 =?utf-8?B?YzlHUWlEanhyYURQTnozRDFIMmtjcUFzYUJIKzY3OTJTOGpBakY5dU9iN1pK?=
 =?utf-8?B?QVg5WUNkV0x6ZzYvRG4vWU1CZGhQTHZJMDRjZ0hwaEtYMENSSmM4azd3MmJw?=
 =?utf-8?B?dk9ad1ZyVXZXK0dyS1BqWFk1ajNXZVZHYnVqMVZXM0tPM3Z4NEZXQXRUVHhF?=
 =?utf-8?B?dk5PdVEva0o2d1IzNEZEVHFOZGhSYjdHSEtMMkZ2Ky84aW9LV1ZOWm1nNTFL?=
 =?utf-8?B?SGRMbWM4YlV6eXgwcXFXaXQxRFZsNW9XdTFZK1liYTlCTVcySVlBTkNBa1ZL?=
 =?utf-8?B?bFNqd2o2T0dHNzNSeld1aUN5Tkt4VG1NMlh2eEFicXNtR0EwQ3VFL0NkSlNk?=
 =?utf-8?B?YU80dDllOENLL1RqdW9vMDg2bnRnYzhtWVB1emEwRmVLK0JqMytzWitzZG1w?=
 =?utf-8?B?K0Y3ZHpGRHdmVWdUVjBVMUpCYTFmRXFDdU9kRGtFdkVXTkJpa3d4NGJlcHor?=
 =?utf-8?B?K1N3VXZibE9iTWRQU0NSQi9NM1RJRTJXd1BBK3BUSEpRSVFiNUVFaTRlWFhM?=
 =?utf-8?B?Z1loNEh2YkJFVnoybDV4eWFkY2dmem4zTGdDTTFlVVRqUisxU0VTZ1IwWW9L?=
 =?utf-8?B?NUtyUXdBeGlIZm83enkzQmVoWmtqbjFNTlBIRVJ2QVc0Z05kUVNnc1RhTFlo?=
 =?utf-8?B?NHN4QzE4NDdubEtGVE10cTFRMy9mZm9wZEpOWmVxaU1yNHp1MHNWdTlKNkxu?=
 =?utf-8?B?NGZib2U1eE5aYUJZOENoLzU4MWk4MEtvdEV4VFlMN3VqclJaRlJGRWVxMTJh?=
 =?utf-8?B?L1NNQmdKMy91QXRCanpsNDNzVnZRdVdkMEFOTlVhSmw0c3lqMzRJVjFxRmNp?=
 =?utf-8?B?cjl0TWczK1NqR2pSVzYvazZtY1NGOWNzUDIxdnB2V2VEaTlnL21sdEE2OExG?=
 =?utf-8?B?N2diQndIVGVxNUVUQmtXaysrYXozcmxrRCt4Nk93akZ2RWMxMmFaNkY0ZVh3?=
 =?utf-8?B?dVN6TnRsYi9qeHVvbUEzY3N2VHgrT0ZiYXRwUkxZbHNOUytnSzlzQjVhZXk1?=
 =?utf-8?B?V3lTOU5ycmxwSmZ3ZzRwS210Mmo1VmROWnVJeFV0MEJwN08weFJLbGpRdUpj?=
 =?utf-8?B?bTljblNaZVdpZFZ2cnR4aTh6SWQ3UGhZUmt5Sm44MzREWnlwaW9jVEVmeDJD?=
 =?utf-8?B?Q0t1THJ1SlhXREQ4Z08rU0poMHo0aTJkUEhFWkN2cUc2K3V4NDkxRDZpbWt1?=
 =?utf-8?B?Y296QkNPaEZ2OTJrcmd4M3RXWU41N0Q4NW9IUXZwdHhpQjl0YU16K1l2OGpt?=
 =?utf-8?B?b2xXdzBXUUw0QXF0OFdnTHNsQlQ3ZlZxN0dQdlZIOCtmNUMwa3J2QW5MbTZ3?=
 =?utf-8?B?cHVhc3IyMWhYQmM2c21oSm9jZThpSEJoVkFsNFZ6a3lTTmxIS3JSN1ZrOHVy?=
 =?utf-8?B?M3VZbjEzaDAySTlOb0tEaUFqOEl0WVBkVEtyK3phTmlXTmdrUDF3K1hKM2RP?=
 =?utf-8?B?VkE2K2htVmNsc3dEcnZCTWt0MDc3SmRaZE4vRmRDd2xDSWZad0ZKaXJZTjlY?=
 =?utf-8?B?Nk5PRWxXUEZkNFdQcjE0c0lDY0ZRWEVuU1I2aFM1aVNGc014M3M5UHhSRUh0?=
 =?utf-8?Q?h6Pg3LDNQBnHd5N/miiTR12f2?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9feb17cc-2dc1-4936-3048-08dbbea4db63
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 15:25:46.9314
 (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: 9qMgudK45y6bQCK3y28UcYsMUtWq1aMaYxkPJPiKRSXooiM7R74jYJarYl4eBlK+i/p6fBw7p+2L3eIQUxuu4w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB9398
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no 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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 26 Sep 2023 08:26:05 -0700 (PDT)


On 2023-08-28 20:05, Jessica Zhang wrote:
> Some drivers support hardware that have optimizations for solid fill
> planes. This series aims to expose these capabilities to userspace as
> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> hardware composer HAL) that can be set by apps like the Android Gears
> app.
> 
> In order to expose this capability to userspace, this series will:
> 
> - Introduce solid_fill and pixel_source properties to allow userspace to
>   toggle between FB and solid fill sources
> - Loosen NULL FB checks within the DRM atomic commit callstack to allow
>   for NULL FB when solid fill is enabled.
> - Add NULL FB checks in methods where FB was previously assumed to be
>   non-NULL
> - Have MSM DPU driver use drm_plane_state.solid_fill instead of
>   dpu_plane_state.color_fill
> 
> Note: The solid fill planes feature depends on both the solid_fill *and*
> pixel_source properties.
> 
> To use this feature, userspace can set the solid_fill property to a blob
> containing the appropriate version number and solid fill color (in
> RGB323232 format) and and setting the pixel_source property to
> DRM_PLANE_PIXEL_SOURCE_COLOR. This will disable memory fetch and the
> resulting plane will display the color specified by the solid_fill blob.
> 
> Currently, there's only one version of the solid_fill blob property.
> However if other drivers want to support a similar feature, but require
> more than just the solid fill color, they can extend this feature by
> creating additional versions of the drm_solid_fill struct.
> 
> This 2 property approach was chosen because passing in a special 1x1 FB
> with the necessary color information would have unecessary overhead that
> does not reflect the behavior of the solid fill feature. In addition,
> assigning the solid fill blob to FB_ID would require loosening some core
> drm_property checks that might cause unwanted side effects elsewhere.
> 

I didn't have a detailed review of this patchset but at a high-level this
change makes sense to me.

Feel free to add my
Acked-by: Harry Wentland <harry.wentland@amd.com>
to patches 1-5.

Harry

> ---
> Changes in v6:
> - Have _dpu_plane_color_fill() take in a single ABGR8888 color instead
>   of having separate alpha and BGR color parameters (Dmitry)
> - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check
>   in SetPlane ioctl (Dmitry)
> - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian)
> - Dropped versioning from solid fill property blob (Dmitry)
> - Use DRM_ENUM_NAME_FN (Dmitry)
> - Use drm_atomic_replace_property_blob_from_id() (Dmitry)
> - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry)
> - Group redundant NULL FB checks (Dmitry)
> - Squashed drm_plane_needs_disable() implementation with 
>   DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian)
> - Add comment to support RGBA solid fill color in the future (Dmitry)
> - Link to v5: https://lore.kernel.org/r/20230728-solid-fill-v5-0-053dbefa909c@quicinc.com
> 
> Changes in v5:
> - Added support for PIXEL_SOURCE_NONE (Sebastian)
> - Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't
>   set (Dmitry)
> - Added debugfs support for both properties (Dmitry)
> - Corrected u32 to u8 conversion (Pekka)
> - Moved drm_solid_fill_info struct and related documentation to
>   include/uapi (Pekka)
> - Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka)
> - Added more detailed UAPI and kernel documentation (Pekka)
> - Reordered patch series so that the pixel_source property is introduced
>   before solid_fill (Dmitry)
> - Fixed inconsistent ABGR8888/RGBA8888 format declaration (Pekka)
> - Reset pixel_source to FB in drm_mode_setplane() (Dmitry)
> - Rename supported_sources to extra_sources (Dmitry)
> - Only destroy old solid_fill blob state if new state is valid (Pekka)
> - Link to v4: https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa742d@quicinc.com
> 
> Changes in v4:
> - Rebased onto latest kernel
> - Reworded cover letter for clarity (Dmitry)
> - Reworded commit messages for clarity
> - Split existing changes into smaller commits
> - Added pixel_source enum property (Dmitry, Pekka, Ville)
> - Updated drm-kms comment docs with pixel_source and solid_fill
>   properties (Dmitry)
> - Inlined drm_atomic_convert_solid_fill_info() (Dmitry)
> - Passed in plane state alpha value to _dpu_plane_color_fill_pipe()
> - Link to v3: https://lore.kernel.org/r/20230104234036.636-1-quic_jesszhan@quicinc.com
> 
> Changes in v3:
> - Fixed some logic errors in atomic checks (Dmitry)
> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
>   methods (Dmitry)
> - Fixed typo in drm_solid_fill struct documentation
> - Created drm_plane_has_visible_data() helper and corrected CRTC and FB
>   NULL-check logic (Dmitry)
> - Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted
>   them into helper method (Dmitry)
> - Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry)
> - Fixed indentation (Dmitry)
> 
> Changes in v2:
> - Dropped SOLID_FILL_FORMAT property (Simon)
> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> - Added drm_solid_fill and drm_solid_fill_info structs (Simon)
> - Changed to checks for if solid_fill_blob is set (Dmitry)
> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>   (Dmitry)
> - Removed DPU_PLANE_COLOR_FILL_FLAG
> - Fixed whitespace and indentation issues (Dmitry)
> - Changed to checks for if solid_fill_blob is set (Dmitry)
> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>   (Dmitry)
> - Fixed dropped 'const' warning
> - Added helper to convert color fill to BGR888 (Rob)
> - Fixed indentation issue (Dmitry)
> - Added support for solid fill on planes of varying sizes
> 
> ---
> Jessica Zhang (10):
>       drm: Introduce pixel_source DRM plane property
>       drm: Introduce solid fill DRM plane property
>       drm: Add solid fill pixel source
>       drm/atomic: Add pixel source to plane state dump
>       drm/atomic: Add solid fill data to plane state dump
>       drm/atomic: Move framebuffer checks to helper
>       drm/atomic: Loosen FB atomic checks
>       drm/msm/dpu: Allow NULL FBs in atomic commit
>       drm/msm/dpu: Use DRM solid_fill property
>       drm/msm/dpu: Add solid fill and pixel source properties
> 
>  drivers/gpu/drm/drm_atomic.c              | 147 +++++++++++++++++-------------
>  drivers/gpu/drm/drm_atomic_helper.c       |  36 ++++----
>  drivers/gpu/drm/drm_atomic_state_helper.c |  10 ++
>  drivers/gpu/drm/drm_atomic_uapi.c         |  30 ++++++
>  drivers/gpu/drm/drm_blend.c               | 129 ++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h       |   1 +
>  drivers/gpu/drm/drm_plane.c               |  27 +++++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  80 ++++++++++------
>  include/drm/drm_atomic_helper.h           |   4 +-
>  include/drm/drm_blend.h                   |   3 +
>  include/drm/drm_plane.h                   |  90 ++++++++++++++++++
>  include/uapi/drm/drm_mode.h               |  24 +++++
>  13 files changed, 478 insertions(+), 112 deletions(-)
> ---
> base-commit: 00ee72279c963989ab435b0bc90b5dc05a9aab79
> change-id: 20230404-solid-fill-05016175db36
> 
> Best regards,