| Message ID | 20230104234036.636-1-quic_jesszhan@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support for Solid Fill Planes | expand |
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. -Daniel > > 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) > > Jessica Zhang (3): > drm: Introduce solid fill property for drm plane > drm: Adjust atomic checks for solid fill color > drm/msm/dpu: Use color_fill property for DPU planes > > drivers/gpu/drm/drm_atomic.c | 136 +++++++++++++--------- > drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- > drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ > drivers/gpu/drm/drm_atomic_uapi.c | 59 ++++++++++ > drivers/gpu/drm/drm_blend.c | 17 +++ > drivers/gpu/drm/drm_plane.c | 8 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++++++---- > include/drm/drm_atomic_helper.h | 5 +- > include/drm/drm_blend.h | 1 + > include/drm/drm_plane.h | 62 ++++++++++ > 11 files changed, 302 insertions(+), 103 deletions(-) > > -- > 2.38.1 >
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. [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 Thanks, Jessica Zhang > -Daniel > >> >> 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) >> >> Jessica Zhang (3): >> drm: Introduce solid fill property for drm plane >> drm: Adjust atomic checks for solid fill color >> drm/msm/dpu: Use color_fill property for DPU planes >> >> drivers/gpu/drm/drm_atomic.c | 136 +++++++++++++--------- >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ >> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++++++++++ >> drivers/gpu/drm/drm_blend.c | 17 +++ >> drivers/gpu/drm/drm_plane.c | 8 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++++++---- >> include/drm/drm_atomic_helper.h | 5 +- >> include/drm/drm_blend.h | 1 + >> include/drm/drm_plane.h | 62 ++++++++++ >> 11 files changed, 302 insertions(+), 103 deletions(-) >> >> -- >> 2.38.1 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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? > > [1] > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 > > Thanks, > > Jessica Zhang > > > -Daniel > > > >> > >> 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) > >> > >> Jessica Zhang (3): > >> drm: Introduce solid fill property for drm plane > >> drm: Adjust atomic checks for solid fill color > >> drm/msm/dpu: Use color_fill property for DPU planes > >> > >> drivers/gpu/drm/drm_atomic.c | 136 +++++++++++++--------- > >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- > >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ > >> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++++++++++ > >> drivers/gpu/drm/drm_blend.c | 17 +++ > >> drivers/gpu/drm/drm_plane.c | 8 +- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++++++---- > >> include/drm/drm_atomic_helper.h | 5 +- > >> include/drm/drm_blend.h | 1 + > >> include/drm/drm_plane.h | 62 ++++++++++ > >> 11 files changed, 302 insertions(+), 103 deletions(-) > >> > >> -- > >> 2.38.1 > >> > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
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. -Daniel > > > > > [1] > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 > > > > Thanks, > > > > Jessica Zhang > > > > > -Daniel > > > > > >> > > >> 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) > > >> > > >> Jessica Zhang (3): > > >> drm: Introduce solid fill property for drm plane > > >> drm: Adjust atomic checks for solid fill color > > >> drm/msm/dpu: Use color_fill property for DPU planes > > >> > > >> drivers/gpu/drm/drm_atomic.c | 136 +++++++++++++--------- > > >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- > > >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ > > >> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++++++++++ > > >> drivers/gpu/drm/drm_blend.c | 17 +++ > > >> drivers/gpu/drm/drm_plane.c | 8 +- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++++++---- > > >> include/drm/drm_atomic_helper.h | 5 +- > > >> include/drm/drm_blend.h | 1 + > > >> include/drm/drm_plane.h | 62 ++++++++++ > > >> 11 files changed, 302 insertions(+), 103 deletions(-) > > >> > > >> -- > > >> 2.38.1 > > >> > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > With best wishes > Dmitry
On 1/5/2023 7:43 PM, 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? Hi Dmitry, PROP_BLOB is defined as a legacy type here [1], but FB_ID is a PROP_OBJECT which is defined as an extended type [2]. So, setting a property blob as the FB would fail drm_property_flags_valid() due to this check [3]. [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/include/uapi/drm/drm_mode.h#L523 [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/include/uapi/drm/drm_mode.h#L534 [3] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 Thanks, Jessica Zhang > >> >> [1] >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 >> >> Thanks, >> >> Jessica Zhang >> >>> -Daniel >>> >>>> >>>> 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) >>>> >>>> Jessica Zhang (3): >>>> drm: Introduce solid fill property for drm plane >>>> drm: Adjust atomic checks for solid fill color >>>> drm/msm/dpu: Use color_fill property for DPU planes >>>> >>>> drivers/gpu/drm/drm_atomic.c | 136 +++++++++++++--------- >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- >>>> drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ >>>> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++++++++++ >>>> drivers/gpu/drm/drm_blend.c | 17 +++ >>>> drivers/gpu/drm/drm_plane.c | 8 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++++++---- >>>> include/drm/drm_atomic_helper.h | 5 +- >>>> include/drm/drm_blend.h | 1 + >>>> include/drm/drm_plane.h | 62 ++++++++++ >>>> 11 files changed, 302 insertions(+), 103 deletions(-) >>>> >>>> -- >>>> 2.38.1 >>>> >>> >>> -- >>> Daniel Vetter >>> Software Engineer, Intel Corporation >>> http://blog.ffwll.ch > > > > -- > With best wishes > Dmitry
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. 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
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 >
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@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. 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. 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. -Daniel > 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 > >
On Wednesday, January 11th, 2023 at 23:29, Daniel Vetter <daniel@ffwll.ch> 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@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. > > 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.
On Fri, 6 Jan 2023 23:49:34 +0200 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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. > > 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. Hi, indeed, what about simply using a 1x1 framebuffer for real? Why was that approach rejected? Is there some problem with drivers just special-casing 1x1 framebuffers and hitting the solid-fill hardware path instead of framebuffer-with-scaling hardware path? If needed, the KMS plane could have a property that tells userspace that if you set a 1x1 RGB FB here without any color ops, I am able to scale that to *any* size without any limits very efficiently. Thanks, pq > > > > [1] > > > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 >
On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote: > indeed, what about simply using a 1x1 framebuffer for real? Why was that > approach rejected? Ideally we don't want to allocate any GPU memory for the solid-fill stuff. And if we special-case 1x1 FB creation to not be backed by real GPU memory then we hit several situations where user-space expects a real FB but there isn't: for instance, GETFB2 converts from FB object ID to GEM handles. Even if we make GETFB2 fail and accept that this breaks user-space, then there is no way for user-space to recover the FB color for flicker-free transitions and such. This is all purely from a uAPI PoV, completely ignoring the potential issues with the internal kernel abstractions which might not be suitable for this either.
On Tue, 31 Jan 2023 10:06:39 +0000 Simon Ser <contact@emersion.fr> wrote: > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > indeed, what about simply using a 1x1 framebuffer for real? Why was that > > approach rejected? > > Ideally we don't want to allocate any GPU memory for the solid-fill > stuff. And if we special-case 1x1 FB creation to not be backed by real > GPU memory then we hit several situations where user-space expects a > real FB but there isn't: for instance, GETFB2 converts from FB object > ID to GEM handles. Even if we make GETFB2 fail and accept that this > breaks user-space, then there is no way for user-space to recover the > FB color for flicker-free transitions and such. > > This is all purely from a uAPI PoV, completely ignoring the potential > issues with the internal kernel abstractions which might not be suitable > for this either. I mean a real 1x1 buffer: a dumb buffer. It would be absolutely compatible with anything existing, because it is a real FB. As a dumb buffer it would be trivial to write into and read out. As 1x1 it would be tiny (one page?). Even if something needs to raw-access uncached memory over 33 MHz PCI bus or whatever the worst case is, it's just one pixel, so it's fast enough, right? And it only needs to be read once when set, like USB display drivers do. The driver does not need to manually apply any color operations, because none are supported in this special case. One can put all these limitations and even pixel format in the plane property that tells userspace that a 1x1 FB works here. To recap, the other alternatives under discussion I see right now are: - this proposal of dedicated fill color property - stuffing something new into FB_ID property There is also the question of other kinds of plane content sources like live camera feeds where userspace won't be shovelling each frame individually like we do now. 1x1 dumb buffer is not as small and lean as a dedicated fill color property, but the UAPI design questions seem to be much less. What's the best trade-off and for whom? Thanks, pq
On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote: > On Tue, 31 Jan 2023 10:06:39 +0000 > Simon Ser <contact@emersion.fr> wrote: > > > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > indeed, what about simply using a 1x1 framebuffer for real? Why was that > > > approach rejected? > > > > Ideally we don't want to allocate any GPU memory for the solid-fill > > stuff. And if we special-case 1x1 FB creation to not be backed by real > > GPU memory then we hit several situations where user-space expects a > > real FB but there isn't: for instance, GETFB2 converts from FB object > > ID to GEM handles. Even if we make GETFB2 fail and accept that this > > breaks user-space, then there is no way for user-space to recover the > > FB color for flicker-free transitions and such. > > > > This is all purely from a uAPI PoV, completely ignoring the potential > > issues with the internal kernel abstractions which might not be suitable > > for this either. > > I mean a real 1x1 buffer: a dumb buffer. > > It would be absolutely compatible with anything existing, because it is > a real FB. As a dumb buffer it would be trivial to write into and read > out. As 1x1 it would be tiny (one page?). Even if something needs to > raw-access uncached memory over 33 MHz PCI bus or whatever the worst > case is, it's just one pixel, so it's fast enough, right? And it only > needs to be read once when set, like USB display drivers do. The driver > does not need to manually apply any color operations, because none are > supported in this special case. > > One can put all these limitations and even pixel format in the plane > property that tells userspace that a 1x1 FB works here. > > To recap, the other alternatives under discussion I see right now are: > > - this proposal of dedicated fill color property > - stuffing something new into FB_ID property > > There is also the question of other kinds of plane content sources like > live camera feeds where userspace won't be shovelling each frame > individually like we do now. > > 1x1 dumb buffer is not as small and lean as a dedicated fill color > property, but the UAPI design questions seem to be much less. What's > the best trade-off and for whom? By "real memory" yes I mean the 1 page. Using a real buffer also brings back other discussions, e.g. the one about which pixel formats to accept.
On Tue, 31 Jan 2023 11:21:18 +0000 Simon Ser <contact@emersion.fr> wrote: > On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > On Tue, 31 Jan 2023 10:06:39 +0000 > > Simon Ser <contact@emersion.fr> wrote: > > > > > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > > indeed, what about simply using a 1x1 framebuffer for real? Why was that > > > > approach rejected? > > > > > > Ideally we don't want to allocate any GPU memory for the solid-fill > > > stuff. And if we special-case 1x1 FB creation to not be backed by real > > > GPU memory then we hit several situations where user-space expects a > > > real FB but there isn't: for instance, GETFB2 converts from FB object > > > ID to GEM handles. Even if we make GETFB2 fail and accept that this > > > breaks user-space, then there is no way for user-space to recover the > > > FB color for flicker-free transitions and such. > > > > > > This is all purely from a uAPI PoV, completely ignoring the potential > > > issues with the internal kernel abstractions which might not be suitable > > > for this either. > > > > I mean a real 1x1 buffer: a dumb buffer. > > > > It would be absolutely compatible with anything existing, because it is > > a real FB. As a dumb buffer it would be trivial to write into and read > > out. As 1x1 it would be tiny (one page?). Even if something needs to > > raw-access uncached memory over 33 MHz PCI bus or whatever the worst > > case is, it's just one pixel, so it's fast enough, right? And it only > > needs to be read once when set, like USB display drivers do. The driver > > does not need to manually apply any color operations, because none are > > supported in this special case. > > > > One can put all these limitations and even pixel format in the plane > > property that tells userspace that a 1x1 FB works here. > > > > To recap, the other alternatives under discussion I see right now are: > > > > - this proposal of dedicated fill color property > > - stuffing something new into FB_ID property > > > > There is also the question of other kinds of plane content sources like > > live camera feeds where userspace won't be shovelling each frame > > individually like we do now. > > > > 1x1 dumb buffer is not as small and lean as a dedicated fill color > > property, but the UAPI design questions seem to be much less. What's > > the best trade-off and for whom? > > By "real memory" yes I mean the 1 page. > > Using a real buffer also brings back other discussions, e.g. the one about > which pixel formats to accept. Yeah, which is why I wrote: "One can put all these limitations and even pixel format in the plane property". It doesn't even need to be a variable in the UAPI, it can be hardcoded in the UAPI doc. Please, do not understand this as me strongly advocating for the real FB approach! I just don't want that option to be misunderstood. I don't really care which design is chosen, but I do care about documenting why other designs were rejected. If the rejection reasons were false, they should be revised, even if the decision does not change. Thanks, pq
On 1/31/2023 4:49 AM, Pekka Paalanen wrote: > On Tue, 31 Jan 2023 11:21:18 +0000 > Simon Ser <contact@emersion.fr> wrote: > >> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote: >> >>> On Tue, 31 Jan 2023 10:06:39 +0000 >>> Simon Ser <contact@emersion.fr> wrote: >>> >>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote: >>>> >>>>> indeed, what about simply using a 1x1 framebuffer for real? Why was that >>>>> approach rejected? >>>> >>>> Ideally we don't want to allocate any GPU memory for the solid-fill >>>> stuff. And if we special-case 1x1 FB creation to not be backed by real >>>> GPU memory then we hit several situations where user-space expects a >>>> real FB but there isn't: for instance, GETFB2 converts from FB object >>>> ID to GEM handles. Even if we make GETFB2 fail and accept that this >>>> breaks user-space, then there is no way for user-space to recover the >>>> FB color for flicker-free transitions and such. >>>> >>>> This is all purely from a uAPI PoV, completely ignoring the potential >>>> issues with the internal kernel abstractions which might not be suitable >>>> for this either. >>> >>> I mean a real 1x1 buffer: a dumb buffer. >>> >>> It would be absolutely compatible with anything existing, because it is >>> a real FB. As a dumb buffer it would be trivial to write into and read >>> out. As 1x1 it would be tiny (one page?). Even if something needs to >>> raw-access uncached memory over 33 MHz PCI bus or whatever the worst >>> case is, it's just one pixel, so it's fast enough, right? And it only >>> needs to be read once when set, like USB display drivers do. The driver >>> does not need to manually apply any color operations, because none are >>> supported in this special case. >>> >>> One can put all these limitations and even pixel format in the plane >>> property that tells userspace that a 1x1 FB works here. >>> >>> To recap, the other alternatives under discussion I see right now are: >>> >>> - this proposal of dedicated fill color property >>> - stuffing something new into FB_ID property >>> >>> There is also the question of other kinds of plane content sources like >>> live camera feeds where userspace won't be shovelling each frame >>> individually like we do now. >>> >>> 1x1 dumb buffer is not as small and lean as a dedicated fill color >>> property, but the UAPI design questions seem to be much less. What's >>> the best trade-off and for whom? >> >> By "real memory" yes I mean the 1 page. >> >> Using a real buffer also brings back other discussions, e.g. the one about >> which pixel formats to accept. > > Yeah, which is why I wrote: "One can put all these limitations and even > pixel format in the plane property". It doesn't even need to be a > variable in the UAPI, it can be hardcoded in the UAPI doc. > > Please, do not understand this as me strongly advocating for the real FB > approach! I just don't want that option to be misunderstood. > > I don't really care which design is chosen, but I do care about > documenting why other designs were rejected. If the rejection reasons > were false, they should be revised, even if the decision does not > change. Hi Pekka/Daniel, Looks like the general sentiment is to keep solid fill as a separate property, so I will stick with that implementation for v4. I can document the reason why we chose this approach over 1x1 FB in the cover letter, but to summarize here: Allocating an FB for solid_fill brings in unnecessary overhead (ex. having to allocate memory for the FB). In addition, since memory fetch is disabled when solid fill is enabled, having a separate property that doesn't do any memory allocation for solid fill better reflects the behavior of this feature within driver. We also wanted to avoid having FB_ID accept a property blob as it would involve loosening some drm_property checks, which could cause issues with other property ioctls. Also, re: other plane sources -- FWIW, I have tried implementing a source enum as Ville suggested, but ultimately dropped the change as it would require userspace to set properties in a specific order (i.e. to enable solid_fill, userspace would have to first set FB_ID to NULL then set SOLID_FILL). I'm not sure how much of a can of worms that would be for userspace, but if you're fine with having that as a requirement the I can re-add the code. Thanks, Jessica Zhang > > > Thanks, > pq
On Wed, 1 Feb 2023 18:06:41 -0800 Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > On 1/31/2023 4:49 AM, Pekka Paalanen wrote: > > On Tue, 31 Jan 2023 11:21:18 +0000 > > Simon Ser <contact@emersion.fr> wrote: > > > >> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote: > >> > >>> On Tue, 31 Jan 2023 10:06:39 +0000 > >>> Simon Ser <contact@emersion.fr> wrote: > >>> > >>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote: > >>>> > >>>>> indeed, what about simply using a 1x1 framebuffer for real? Why was that > >>>>> approach rejected? > >>>> > >>>> Ideally we don't want to allocate any GPU memory for the solid-fill > >>>> stuff. And if we special-case 1x1 FB creation to not be backed by real > >>>> GPU memory then we hit several situations where user-space expects a > >>>> real FB but there isn't: for instance, GETFB2 converts from FB object > >>>> ID to GEM handles. Even if we make GETFB2 fail and accept that this > >>>> breaks user-space, then there is no way for user-space to recover the > >>>> FB color for flicker-free transitions and such. > >>>> > >>>> This is all purely from a uAPI PoV, completely ignoring the potential > >>>> issues with the internal kernel abstractions which might not be suitable > >>>> for this either. > >>> > >>> I mean a real 1x1 buffer: a dumb buffer. > >>> > >>> It would be absolutely compatible with anything existing, because it is > >>> a real FB. As a dumb buffer it would be trivial to write into and read > >>> out. As 1x1 it would be tiny (one page?). Even if something needs to > >>> raw-access uncached memory over 33 MHz PCI bus or whatever the worst > >>> case is, it's just one pixel, so it's fast enough, right? And it only > >>> needs to be read once when set, like USB display drivers do. The driver > >>> does not need to manually apply any color operations, because none are > >>> supported in this special case. > >>> > >>> One can put all these limitations and even pixel format in the plane > >>> property that tells userspace that a 1x1 FB works here. > >>> > >>> To recap, the other alternatives under discussion I see right now are: > >>> > >>> - this proposal of dedicated fill color property > >>> - stuffing something new into FB_ID property > >>> > >>> There is also the question of other kinds of plane content sources like > >>> live camera feeds where userspace won't be shovelling each frame > >>> individually like we do now. > >>> > >>> 1x1 dumb buffer is not as small and lean as a dedicated fill color > >>> property, but the UAPI design questions seem to be much less. What's > >>> the best trade-off and for whom? > >> > >> By "real memory" yes I mean the 1 page. > >> > >> Using a real buffer also brings back other discussions, e.g. the one about > >> which pixel formats to accept. > > > > Yeah, which is why I wrote: "One can put all these limitations and even > > pixel format in the plane property". It doesn't even need to be a > > variable in the UAPI, it can be hardcoded in the UAPI doc. > > > > Please, do not understand this as me strongly advocating for the real FB > > approach! I just don't want that option to be misunderstood. > > > > I don't really care which design is chosen, but I do care about > > documenting why other designs were rejected. If the rejection reasons > > were false, they should be revised, even if the decision does not > > change. > > Hi Pekka/Daniel, > > Looks like the general sentiment is to keep solid fill as a separate > property, so I will stick with that implementation for v4. > > I can document the reason why we chose this approach over 1x1 FB in the > cover letter, but to summarize here: > > Allocating an FB for solid_fill brings in unnecessary overhead (ex. > having to allocate memory for the FB). In addition, since memory fetch > is disabled when solid fill is enabled, having a separate property that > doesn't do any memory allocation for solid fill better reflects the > behavior of this feature within driver. > > We also wanted to avoid having FB_ID accept a property blob as it would > involve loosening some drm_property checks, which could cause issues > with other property ioctls. > That's fine by me, thanks! > Also, re: other plane sources -- FWIW, I have tried implementing a > source enum as Ville suggested, but ultimately dropped the change as it > would require userspace to set properties in a specific order (i.e. to > enable solid_fill, userspace would have to first set FB_ID to NULL then > set SOLID_FILL). > > I'm not sure how much of a can of worms that would be for userspace, but > if you're fine with having that as a requirement the I can re-add the code. There is no ordering between properties set in a single atomic commit, they all apply at the same time. Therefore the kernel code needs to consider the whole new state set as a single entity. If userspace splits changing those two properties into different atomic commits, that's a userspace bug. It would not work with atomic properties already today, where you need to set half a dozen properties to update one KMS plane. The only complication I can see is the legacy KMS UAPI, non-atomic. They will change FB_ID, but they cannot touch the solid fill property. I guess that needs to be special-cased somehow. Thanks, pq