Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

Hi Daniel

Thanks for looking into this series.

On 1/6/2023 1:49 PM, Dmitry Baryshkov wrote:
> On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
>>> On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/5/2023 3:33 AM, Daniel Vetter wrote:
>>>>> On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
>>>>>> Introduce and add support for a solid_fill property. When the solid_fill
>>>>>> property is set, and the framebuffer is set to NULL, memory fetch will be
>>>>>> disabled.
>>>>>>
>>>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>>>> to allow a NULL FB when the solid_fill property is set and add FB checks
>>>>>> in methods where the FB was previously assumed to be non-NULL.
>>>>>>
>>>>>> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
>>>>>> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
>>>>>> callstack to account for a NULL FB in cases where solid_fill is set.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Userspace can set the solid_fill property to a blob containing the
>>>>>> appropriate version number and solid fill color (in RGB323232 format) and
>>>>>> setting the framebuffer to NULL.
>>>>>>
>>>>>> Note: 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.
>>>>>>
>>>>>> Changes in V2:
>>>>>> - Dropped SOLID_FILL_FORMAT property (Simon)
>>>>>> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
>>>>>> - 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)
>>>>>
>>>>> Now that this is a blob, I do wonder again whether it's not cleaner to set
>>>>> the blob as the FB pointer. Or create some kind other kind of special data
>>>>> source objects (because solid fill is by far not the only such thing).
>>>>>
>>>>> We'd still end up in special cases like when userspace that doesn't
>>>>> understand solid fill tries to read out such a framebuffer, but these
>>>>> cases already exist anyway for lack of priviledges.
>>>>>
>>>>> So I still think that feels like the more consistent way to integrate this
>>>>> feature. Which doesn't mean it has to happen like that, but the
>>>>> patches/cover letter should at least explain why we don't do it like this.
>>>>
>>>> Hi Daniel,
>>>>
>>>> IIRC we were facing some issues with this check [1] when trying to set
>>>> FB to a PROP_BLOB instead. Which is why we went with making it a
>>>> separate property instead. Will mention this in the cover letter.
>>>
>>> What kind of issues? Could you please describe them?
>>
>> We switched from bitmask to enum style for prop types, which means it's
>> not possible to express with the current uapi a property which accepts
>> both an object or a blob.
>>
>> Which yeah sucks a bit ...
>>
>> But!
>>
>> blob properties are kms objects (like framebuffers), so it should be
>> possible to stuff a blob into an object property as-is. Of course you need
>> to update the validation code to make sure we accept either an fb or a
>> blob for the internal representation. But that kind of split internally is
>> required no matter what I think.
> 
> I checked your idea and notes from Jessica. So while we can pass blobs
> to property objects, the prop_fb_id is created as an object property
> with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
> fail a check in drm_property_change_valid_get() ->
> __drm_mode_object_find(). And I don't think that we should break the
> existing validation code for this special case.
> 

Like Jessica wrote, re-using the FB_ID property to pass solid fill 
information will need modification of existing checks shown in [1] OR 
the property creation itself would fail.

We just went with this approach, as it was less intrusive and would not 
affect the existing FB_ID path.

Since both approaches need modifications of validation checks, adding a 
new property is less intrusive and safer than the already convoluted 
checks in drm_property_flags_valid().

Let us know if its a strong preference on your side to re-use FB_ID and 
if so why.

Thanks

Abhinav

> If you insist on using FB_ID for passing solid_fill information, I'd
> ask you to reconsider using a 1x1 framebuffer. It would be fully
> compatible with the existing userspace, which can then treat it
> seamlessly.
> 
>> -Daniel
>>
>>>
>>>>
>>>> [1]
>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
>