On Wednesday, January 11th, 2023 at 23:29, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Fri, Jan 06, 2023 at 04:33:04PM -0800, Abhinav Kumar wrote: > > 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@xxxxxxxx> 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@xxxxxxxxxxx> 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. > > Yeah modifications there are needed. Or well, we need to extend that code > to allow bitmask for objects iff the driver allows that. What I meant is > that from an uapi pov it's doable, which is the part that matters because > that is the part that we wont ever be able to change once it's baked in. > > > 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(). > > It's easier, but it's also the part that we cannot ever change (because > it's uapi). If the NULL fb on an active plane makes some compositor die on > vt switch, we then have a pretty serious problem. The new compositor might try to GETFB on the blob which isn't actually an FB, and gets confused even more... > I'm honestly not sure which is the approach that blows up less, just that > justifying uapi with the implementation work on the kernel side is not > really the right approach :-) > > > Let us know if its a strong preference on your side to re-use FB_ID and if > > so why. > > I just want to make sure we're picking the right uapi option, and that > we're not just excluding them because it looks like we can't implement > them in the kernel. Uapi is forever, the implemenation is not. IMHO re-using FB_ID for this is not a good idea. FB_ID is described by the kernel as a property which accepts FB objects (DRM_MODE_OBJECT_FB). Mixing up something else will just result in more confusion.
