| Message ID | 20221028225952.160-1-quic_jesszhan@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support for Solid Fill Planes | expand |
On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > properties. When the color fill value is set, and the framebuffer is set > to NULL, memory fetch will be disabled. Thinking a bit more universally I wonder if there should be some kind of enum property: enum plane_pixel_source { FB, COLOR, LIVE_FOO, LIVE_BAR, ... } > In addition, loosen the NULL FB checks within the atomic commit callstack > to allow a NULL FB when color_fill is nonzero 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.color_fill and > drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a > DRM format, setting COLOR_FILL to a color fill value, and setting the > framebuffer to NULL. Is there some real reason for the format property? Ie. why not just do what was the plan for the crttc background color and specify the color in full 16bpc format and just pick as many msbs from that as the hw can use? > > Jessica Zhang (3): > drm: Introduce color fill properties 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 | 68 ++++++++++++----------- > drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- > drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ > drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ > drivers/gpu/drm/drm_plane.c | 8 +-- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- > include/drm/drm_atomic_helper.h | 5 +- > include/drm/drm_blend.h | 2 + > include/drm/drm_plane.h | 28 ++++++++++ > 10 files changed, 188 insertions(+), 76 deletions(-) > > -- > 2.38.0
On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >> properties. When the color fill value is set, and the framebuffer is set >> to NULL, memory fetch will be disabled. > > Thinking a bit more universally I wonder if there should be > some kind of enum property: > > enum plane_pixel_source { > FB, > COLOR, > LIVE_FOO, > LIVE_BAR, > ... > } Hi Ville, Makes sense -- this way, we'd also lay some groundwork for cases where drivers want to use other non-FB sources. > >> In addition, loosen the NULL FB checks within the atomic commit callstack >> to allow a NULL FB when color_fill is nonzero 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.color_fill and >> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a >> DRM format, setting COLOR_FILL to a color fill value, and setting the >> framebuffer to NULL. > > Is there some real reason for the format property? Ie. why not > just do what was the plan for the crttc background color and > specify the color in full 16bpc format and just pick as many > msbs from that as the hw can use? The format property was added because we can't assume that all hardware will support/use the same color format for solid fill planes. Even for just MSM devices, the hardware supports different variations of RGB formats [1]. Thanks, Jessica Zhang [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697 > >> >> Jessica Zhang (3): >> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >> drivers/gpu/drm/drm_plane.c | 8 +-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- >> include/drm/drm_atomic_helper.h | 5 +- >> include/drm/drm_blend.h | 2 + >> include/drm/drm_plane.h | 28 ++++++++++ >> 10 files changed, 188 insertions(+), 76 deletions(-) >> >> -- >> 2.38.0 > > -- > Ville Syrjälä > Intel
On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > > On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > >> properties. When the color fill value is set, and the framebuffer is set > >> to NULL, memory fetch will be disabled. > > > > Thinking a bit more universally I wonder if there should be > > some kind of enum property: > > > > enum plane_pixel_source { > > FB, > > COLOR, > > LIVE_FOO, > > LIVE_BAR, > > ... > > } > > Hi Ville, > > Makes sense -- this way, we'd also lay some groundwork for cases where > drivers want to use other non-FB sources. > > > > >> In addition, loosen the NULL FB checks within the atomic commit callstack > >> to allow a NULL FB when color_fill is nonzero 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.color_fill and > >> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a > >> DRM format, setting COLOR_FILL to a color fill value, and setting the > >> framebuffer to NULL. > > > > Is there some real reason for the format property? Ie. why not > > just do what was the plan for the crttc background color and > > specify the color in full 16bpc format and just pick as many > > msbs from that as the hw can use? > > The format property was added because we can't assume that all hardware > will support/use the same color format for solid fill planes. Even for > just MSM devices, the hardware supports different variations of RGB > formats [1]. Sure, but the driver can convert the format into whatever the hw wants. A 1x1 color conversion is not going to be problematic ;-) BR, -R > Thanks, > > Jessica Zhang > > [1] > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697 > > > > >> > >> Jessica Zhang (3): > >> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- > >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- > >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ > >> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ > >> drivers/gpu/drm/drm_plane.c | 8 +-- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- > >> include/drm/drm_atomic_helper.h | 5 +- > >> include/drm/drm_blend.h | 2 + > >> include/drm/drm_plane.h | 28 ++++++++++ > >> 10 files changed, 188 insertions(+), 76 deletions(-) > >> > >> -- > >> 2.38.0 > > > > -- > > Ville Syrjälä > > Intel
On Mon, Nov 07, 2022 at 09:37:08PM +0200, Ville Syrjälä wrote: > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > > Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > > properties. When the color fill value is set, and the framebuffer is set > > to NULL, memory fetch will be disabled. > > Thinking a bit more universally I wonder if there should be > some kind of enum property: > > enum plane_pixel_source { > FB, > COLOR, > LIVE_FOO, > LIVE_BAR, > ... > } That's something I could use on (older) Renesas hardware, where planes can be fed with a live source. The issue there is that the live source is a compositor with multiple inputs, handled through the V4L2 API. How to synchronize the configuration of the compositor and the display engine is an unsolved problem at the moment, but it could be solved another day. > > In addition, loosen the NULL FB checks within the atomic commit callstack > > to allow a NULL FB when color_fill is nonzero 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.color_fill and > > drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a > > DRM format, setting COLOR_FILL to a color fill value, and setting the > > framebuffer to NULL. > > Is there some real reason for the format property? Ie. why not > just do what was the plan for the crttc background color and > specify the color in full 16bpc format and just pick as many > msbs from that as the hw can use? > > > Jessica Zhang (3): > > drm: Introduce color fill properties 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 | 68 ++++++++++++----------- > > drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- > > drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ > > drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ > > drivers/gpu/drm/drm_plane.c | 8 +-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- > > include/drm/drm_atomic_helper.h | 5 +- > > include/drm/drm_blend.h | 2 + > > include/drm/drm_plane.h | 28 ++++++++++ > > 10 files changed, 188 insertions(+), 76 deletions(-)
On 11/7/2022 2:09 PM, Rob Clark wrote: > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >> >> >> >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>> properties. When the color fill value is set, and the framebuffer is set >>>> to NULL, memory fetch will be disabled. >>> >>> Thinking a bit more universally I wonder if there should be >>> some kind of enum property: >>> >>> enum plane_pixel_source { >>> FB, >>> COLOR, >>> LIVE_FOO, >>> LIVE_BAR, >>> ... >>> } >> >> Hi Ville, >> >> Makes sense -- this way, we'd also lay some groundwork for cases where >> drivers want to use other non-FB sources. >> >>> >>>> In addition, loosen the NULL FB checks within the atomic commit callstack >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>>> framebuffer to NULL. >>> >>> Is there some real reason for the format property? Ie. why not >>> just do what was the plan for the crttc background color and >>> specify the color in full 16bpc format and just pick as many >>> msbs from that as the hw can use? >> >> The format property was added because we can't assume that all hardware >> will support/use the same color format for solid fill planes. Even for >> just MSM devices, the hardware supports different variations of RGB >> formats [1]. > > Sure, but the driver can convert the format into whatever the hw > wants. A 1x1 color conversion is not going to be problematic ;-) Hi Rob, Hm... that's also a fair point. Just wondering, is there any advantage of having the driver convert the format, other than not having to implement an extra format property? (In case we end up wrapping everything into a prop blob or something) Thanks, Jessica Zhang > > BR, > -R > >> Thanks, >> >> Jessica Zhang >> >> [1] >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697 >> >>> >>>> >>>> Jessica Zhang (3): >>>> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>>> drivers/gpu/drm/drm_plane.c | 8 +-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- >>>> include/drm/drm_atomic_helper.h | 5 +- >>>> include/drm/drm_blend.h | 2 + >>>> include/drm/drm_plane.h | 28 ++++++++++ >>>> 10 files changed, 188 insertions(+), 76 deletions(-) >>>> >>>> -- >>>> 2.38.0 >>> >>> -- >>> Ville Syrjälä >>> Intel
On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > > On 11/7/2022 2:09 PM, Rob Clark wrote: > > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> > >> > >> > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > >>>> properties. When the color fill value is set, and the framebuffer is set > >>>> to NULL, memory fetch will be disabled. > >>> > >>> Thinking a bit more universally I wonder if there should be > >>> some kind of enum property: > >>> > >>> enum plane_pixel_source { > >>> FB, > >>> COLOR, > >>> LIVE_FOO, > >>> LIVE_BAR, > >>> ... > >>> } > >> > >> Hi Ville, > >> > >> Makes sense -- this way, we'd also lay some groundwork for cases where > >> drivers want to use other non-FB sources. > >> > >>> > >>>> In addition, loosen the NULL FB checks within the atomic commit callstack > >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and > >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a > >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the > >>>> framebuffer to NULL. > >>> > >>> Is there some real reason for the format property? Ie. why not > >>> just do what was the plan for the crttc background color and > >>> specify the color in full 16bpc format and just pick as many > >>> msbs from that as the hw can use? > >> > >> The format property was added because we can't assume that all hardware > >> will support/use the same color format for solid fill planes. Even for > >> just MSM devices, the hardware supports different variations of RGB > >> formats [1]. > > > > Sure, but the driver can convert the format into whatever the hw > > wants. A 1x1 color conversion is not going to be problematic ;-) > > Hi Rob, > > Hm... that's also a fair point. Just wondering, is there any advantage > of having the driver convert the format, other than not having to > implement an extra format property? > > (In case we end up wrapping everything into a prop blob or something) > It keeps the uabi simpler.. for obvious reasons you don't want the driver to do cpu color conversion for an arbitrary size plane, which is why we go to all the complexity to expose formats and modifiers for "real" planes, but we are dealing with a single pixel value here, let's not make the uabi more complex than we need to. I'd propose making it float32[4] if float weren't a pita for kernel/uabi, but u16[4] or u32[4] should be fine, and drivers can translate that easily into whatever weird formats their hw wants for solid-fill. BR, -R
On Mon, Nov 07, 2022 at 07:34:43PM -0800, Rob Clark wrote: > On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > > > > > > On 11/7/2022 2:09 PM, Rob Clark wrote: > > > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > >> > > >> > > >> > > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > > >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > > >>>> properties. When the color fill value is set, and the framebuffer is set > > >>>> to NULL, memory fetch will be disabled. > > >>> > > >>> Thinking a bit more universally I wonder if there should be > > >>> some kind of enum property: > > >>> > > >>> enum plane_pixel_source { > > >>> FB, > > >>> COLOR, > > >>> LIVE_FOO, > > >>> LIVE_BAR, > > >>> ... > > >>> } > > >> > > >> Hi Ville, > > >> > > >> Makes sense -- this way, we'd also lay some groundwork for cases where > > >> drivers want to use other non-FB sources. > > >> > > >>> > > >>>> In addition, loosen the NULL FB checks within the atomic commit callstack > > >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and > > >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a > > >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the > > >>>> framebuffer to NULL. > > >>> > > >>> Is there some real reason for the format property? Ie. why not > > >>> just do what was the plan for the crttc background color and > > >>> specify the color in full 16bpc format and just pick as many > > >>> msbs from that as the hw can use? > > >> > > >> The format property was added because we can't assume that all hardware > > >> will support/use the same color format for solid fill planes. Even for > > >> just MSM devices, the hardware supports different variations of RGB > > >> formats [1]. > > > > > > Sure, but the driver can convert the format into whatever the hw > > > wants. A 1x1 color conversion is not going to be problematic ;-) > > > > Hi Rob, > > > > Hm... that's also a fair point. Just wondering, is there any advantage > > of having the driver convert the format, other than not having to > > implement an extra format property? > > > > (In case we end up wrapping everything into a prop blob or something) > > > > It keeps the uabi simpler.. for obvious reasons you don't want the > driver to do cpu color conversion for an arbitrary size plane, which > is why we go to all the complexity to expose formats and modifiers for > "real" planes, but we are dealing with a single pixel value here, > let's not make the uabi more complex than we need to. I'd propose > making it float32[4] if float weren't a pita for kernel/uabi, but > u16[4] or u32[4] should be fine, and drivers can translate that easily > into whatever weird formats their hw wants for solid-fill. u16[4] fits into a single u64 property value. That was the plan for the background prop as well: https://lore.kernel.org/all/20190703125442.GW5942@intel.com/T/
On 11/8/2022 12:52 AM, Ville Syrjälä wrote: > On Mon, Nov 07, 2022 at 07:34:43PM -0800, Rob Clark wrote: >> On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com>wrote: >>> >>> >>> >>> On 11/7/2022 2:09 PM, Rob Clark wrote: >>>> On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>>>> >>>>> >>>>> >>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>>>>> properties. When the color fill value is set, and the framebuffer is set >>>>>>> to NULL, memory fetch will be disabled. >>>>>> >>>>>> Thinking a bit more universally I wonder if there should be >>>>>> some kind of enum property: >>>>>> >>>>>> enum plane_pixel_source { >>>>>> FB, >>>>>> COLOR, >>>>>> LIVE_FOO, >>>>>> LIVE_BAR, >>>>>> ... >>>>>> } >>>>> >>>>> Hi Ville, >>>>> >>>>> Makes sense -- this way, we'd also lay some groundwork for cases where >>>>> drivers want to use other non-FB sources. >>>>> >>>>>> >>>>>>> In addition, loosen the NULL FB checks within the atomic commit callstack >>>>>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>>>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMATto a >>>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>>>>>> framebuffer to NULL. >>>>>> >>>>>> Is there some real reason for the format property? Ie. why not >>>>>> just do what was the plan for the crttc background color and >>>>>> specify the color in full 16bpc format and just pick as many >>>>>> msbs from that as the hw can use? >>>>> >>>>> The format property was added because we can't assume that all hardware >>>>> will support/use the same color format for solid fill planes. Even for >>>>> just MSM devices, the hardware supports different variations of RGB >>>>> formats [1]. >>>> >>>> Sure, but the driver can convert the format into whatever the hw >>>> wants. A 1x1 color conversion is not going to be problematic ;-) >>> >>> Hi Rob, >>> >>> Hm... that's also a fair point. Just wondering, is there any advantage >>> of having the driver convert the format, other than not having to >>> implement an extra format property? >>> >>> (In case we end up wrapping everything into a prop blob or something) >>> >> >> It keeps the uabi simpler.. for obvious reasons you don't want the >> driver to do cpu color conversion for an arbitrary size plane, which >> is why we go to all the complexity to expose formats and modifiers for >> "real" planes, but we are dealing with a single pixel value here, >> let's not make the uabi more complex than we need to. I'd propose >> making it float32[4] if float weren't a pita for kernel/uabi, but >> u16[4] or u32[4] should be fine, and drivers can translate that easily >> into whatever weird formats their hw wants for solid-fill. > > u16[4] fits into a single u64 property value. > > That was the plan for the background prop as well: > https://lore.kernel.org/all/20190703125442.GW5942@intel.com/T/ Got it, I think that's pretty reasonable then. Will probably use u32[4] instead since that is what Pekka and Simon are recommending to match Wayland's single-buffer protocol [1]. Thanks, Jessica Zhang [1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104 > > -- > Ville Syrjälä > Intel
On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >> properties. When the color fill value is set, and the framebuffer is set >> to NULL, memory fetch will be disabled. > > Thinking a bit more universally I wonder if there should be > some kind of enum property: > > enum plane_pixel_source { > FB, > COLOR, > LIVE_FOO, > LIVE_BAR, > ... > } Reviving this thread as this was the initial comment suggesting to implement pixel_source as an enum. I think the issue with having pixel_source as an enum is how to decide what counts as a NULL commit. Currently, setting the FB to NULL will disable the plane. So I'm guessing we will extend that logic to "if there's no pixel_source set for the plane, then it will be a NULL commit and disable the plane". In that case, the question then becomes when to set the pixel_source to NONE. Because if we do that when setting a NULL FB (or NULL solid_fill blob), it then forces userspace to set one property before the other. Because of that, I'm thinking of having pixel_source be represented by a bitmask instead. That way, we will simply unset the corresponding pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in order to detect whether a commit is NULL or has a valid pixel source, we can just check if pixel_source == 0. Would be interested in any feedback on this. Thanks, Jessica Zhang > >> In addition, loosen the NULL FB checks within the atomic commit callstack >> to allow a NULL FB when color_fill is nonzero 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.color_fill and >> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a >> DRM format, setting COLOR_FILL to a color fill value, and setting the >> framebuffer to NULL. > > Is there some real reason for the format property? Ie. why not > just do what was the plan for the crttc background color and > specify the color in full 16bpc format and just pick as many > msbs from that as the hw can use? > >> >> Jessica Zhang (3): >> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >> drivers/gpu/drm/drm_plane.c | 8 +-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- >> include/drm/drm_atomic_helper.h | 5 +- >> include/drm/drm_blend.h | 2 + >> include/drm/drm_plane.h | 28 ++++++++++ >> 10 files changed, 188 insertions(+), 76 deletions(-) >> >> -- >> 2.38.0 > > -- > Ville Syrjälä > Intel
On 27/06/2023 02:02, Jessica Zhang wrote: > > > On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>> properties. When the color fill value is set, and the framebuffer is set >>> to NULL, memory fetch will be disabled. >> >> Thinking a bit more universally I wonder if there should be >> some kind of enum property: >> >> enum plane_pixel_source { >> FB, >> COLOR, >> LIVE_FOO, >> LIVE_BAR, >> ... >> } > > Reviving this thread as this was the initial comment suggesting to > implement pixel_source as an enum. > > I think the issue with having pixel_source as an enum is how to decide > what counts as a NULL commit. > > Currently, setting the FB to NULL will disable the plane. So I'm > guessing we will extend that logic to "if there's no pixel_source set > for the plane, then it will be a NULL commit and disable the plane". > > In that case, the question then becomes when to set the pixel_source to > NONE. Because if we do that when setting a NULL FB (or NULL solid_fill > blob), it then forces userspace to set one property before the other. Why? The userspace should use atomic commits and as such it should all properties at the same time. > Because of that, I'm thinking of having pixel_source be represented by a > bitmask instead. That way, we will simply unset the corresponding > pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in > order to detect whether a commit is NULL or has a valid pixel source, we > can just check if pixel_source == 0. > > Would be interested in any feedback on this. This is an interesting idea. Frankly speaking, I'd consider it counter-intuitive at the first glance. Consider it to act as a curtain. Setup the curtain (by writing the fill colour property). Then one can close the curtain (by switching source to colour), or open it (by switching to any other source). Bitmask wouldn't complicate this. > > Thanks, > > Jessica Zhang > >> >>> In addition, loosen the NULL FB checks within the atomic commit >>> callstack >>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a >>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>> framebuffer to NULL. >> >> Is there some real reason for the format property? Ie. why not >> just do what was the plan for the crttc background color and >> specify the color in full 16bpc format and just pick as many >> msbs from that as the hw can use? >> >>> >>> Jessica Zhang (3): >>> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- >>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>> drivers/gpu/drm/drm_plane.c | 8 +-- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- >>> include/drm/drm_atomic_helper.h | 5 +- >>> include/drm/drm_blend.h | 2 + >>> include/drm/drm_plane.h | 28 ++++++++++ >>> 10 files changed, 188 insertions(+), 76 deletions(-) >>> >>> -- >>> 2.38.0 >> >> -- >> Ville Syrjälä >> Intel
On 6/26/2023 5:06 PM, Dmitry Baryshkov wrote: > On 27/06/2023 02:02, Jessica Zhang wrote: >> >> >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>> properties. When the color fill value is set, and the framebuffer is >>>> set >>>> to NULL, memory fetch will be disabled. >>> >>> Thinking a bit more universally I wonder if there should be >>> some kind of enum property: >>> >>> enum plane_pixel_source { >>> FB, >>> COLOR, >>> LIVE_FOO, >>> LIVE_BAR, >>> ... >>> } >> >> Reviving this thread as this was the initial comment suggesting to >> implement pixel_source as an enum. >> >> I think the issue with having pixel_source as an enum is how to decide >> what counts as a NULL commit. >> >> Currently, setting the FB to NULL will disable the plane. So I'm >> guessing we will extend that logic to "if there's no pixel_source set >> for the plane, then it will be a NULL commit and disable the plane". >> >> In that case, the question then becomes when to set the pixel_source >> to NONE. Because if we do that when setting a NULL FB (or NULL >> solid_fill blob), it then forces userspace to set one property before >> the other. > > Why? The userspace should use atomic commits and as such it should all > properties at the same time. Correct, userspace will set all the properties within the same atomic commit. The issue happens when the driver iterates through each property within the MODE_ATOMIC ioctl [1]. For reference, I'm thinking of an implementation where we're setting the pixel_source within drm_atomic_plane_set_property(). So something like: drm_atomic_plane_set_property( ... ) { if (property == config->prop_fb_id) { if (fb) state->pixel_source = FB; else state->pixel_source = NONE; } else if (property == config->prop_solid_fill) { if (solid_fill_blob) state->pixel_source = SOLID_FILL; } // ... } If userspace sets solid_fill to a valid blob and FB to NULL, it's possible for driver to first set the solid_fill property then set the fb_id property later. This would cause pixel_source to be set to NONE after all properties have been set. I've also considered an implementation without the `pixel_source = NONE` line in the prop_fb_id case, but we would still need to find somewhere to set the pixel_source to NONE in order to allow userspace to disable a plane. [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_uapi.c#L1385 > >> Because of that, I'm thinking of having pixel_source be represented by >> a bitmask instead. That way, we will simply unset the corresponding >> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >> order to detect whether a commit is NULL or has a valid pixel source, >> we can just check if pixel_source == 0. >> >> Would be interested in any feedback on this. > > This is an interesting idea. Frankly speaking, I'd consider it > counter-intuitive at the first glance. > > Consider it to act as a curtain. Setup the curtain (by writing the fill > colour property). Then one can close the curtain (by switching source to > colour), or open it (by switching to any other source). Bitmask wouldn't > complicate this. So if I'm understanding your analogy correctly, pixel_source won't necessarily be set whenever the FB or solid_fill properties are set. So that way we can have both FB *and* solid_fill set at the same time, but only the source that pixel_source is set to would be displayed. Thanks, Jessica Zhang > >> >> Thanks, >> >> Jessica Zhang >> >>> >>>> In addition, loosen the NULL FB checks within the atomic commit >>>> callstack >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT >>>> to a >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>>> framebuffer to NULL. >>> >>> Is there some real reason for the format property? Ie. why not >>> just do what was the plan for the crttc background color and >>> specify the color in full 16bpc format and just pick as many >>> msbs from that as the hw can use? >>> >>>> >>>> Jessica Zhang (3): >>>> drm: Introduce color fill properties 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 | 68 >>>> ++++++++++++----------- >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>>> drivers/gpu/drm/drm_plane.c | 8 +-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- >>>> include/drm/drm_atomic_helper.h | 5 +- >>>> include/drm/drm_blend.h | 2 + >>>> include/drm/drm_plane.h | 28 ++++++++++ >>>> 10 files changed, 188 insertions(+), 76 deletions(-) >>>> >>>> -- >>>> 2.38.0 >>> >>> -- >>> Ville Syrjälä >>> Intel > > -- > With best wishes > Dmitry >
On Tue, 27 Jun 2023 at 03:45, Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > > On 6/26/2023 5:06 PM, Dmitry Baryshkov wrote: > > On 27/06/2023 02:02, Jessica Zhang wrote: > >> > >> > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > >>>> properties. When the color fill value is set, and the framebuffer is > >>>> set > >>>> to NULL, memory fetch will be disabled. > >>> > >>> Thinking a bit more universally I wonder if there should be > >>> some kind of enum property: > >>> > >>> enum plane_pixel_source { > >>> FB, > >>> COLOR, > >>> LIVE_FOO, > >>> LIVE_BAR, > >>> ... > >>> } > >> > >> Reviving this thread as this was the initial comment suggesting to > >> implement pixel_source as an enum. > >> > >> I think the issue with having pixel_source as an enum is how to decide > >> what counts as a NULL commit. > >> > >> Currently, setting the FB to NULL will disable the plane. So I'm > >> guessing we will extend that logic to "if there's no pixel_source set > >> for the plane, then it will be a NULL commit and disable the plane". > >> > >> In that case, the question then becomes when to set the pixel_source > >> to NONE. Because if we do that when setting a NULL FB (or NULL > >> solid_fill blob), it then forces userspace to set one property before > >> the other. > > > > Why? The userspace should use atomic commits and as such it should all > > properties at the same time. > > Correct, userspace will set all the properties within the same atomic > commit. The issue happens when the driver iterates through each property > within the MODE_ATOMIC ioctl [1]. > > For reference, I'm thinking of an implementation where we're setting the > pixel_source within drm_atomic_plane_set_property(). > > So something like: > > drm_atomic_plane_set_property( ... ) > { > if (property == config->prop_fb_id) { > if (fb) > state->pixel_source = FB; > else > state->pixel_source = NONE; > } else if (property == config->prop_solid_fill) { > if (solid_fill_blob) > state->pixel_source = SOLID_FILL; > } > > // ... > } I think this is somewhat overcomplicated. Allow userspace to set these properties as it sees fit and then in drm_atomic_helper_check_plane_state() consider all of them to set plane_state->visible. We still have to remain compatible with older userspace (esp. with a non-atomic one). It expects that a plane is enabled after setting both CRTC and FB. So maybe you are right and we should force pixel_source to FB if FB is set. > > If userspace sets solid_fill to a valid blob and FB to NULL, it's > possible for driver to first set the solid_fill property then set the > fb_id property later. This would cause pixel_source to be set to NONE > after all properties have been set. > > I've also considered an implementation without the `pixel_source = NONE` > line in the prop_fb_id case, but we would still need to find somewhere > to set the pixel_source to NONE in order to allow userspace to disable a > plane. Good point. I don't think we would need NONE (just setting CRTC to none or FB to none and pixel_source to FB would disable the plane), but I might be missing something here. > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_uapi.c#L1385 > > > > >> Because of that, I'm thinking of having pixel_source be represented by > >> a bitmask instead. That way, we will simply unset the corresponding > >> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in > >> order to detect whether a commit is NULL or has a valid pixel source, > >> we can just check if pixel_source == 0. > >> > >> Would be interested in any feedback on this. > > > > This is an interesting idea. Frankly speaking, I'd consider it > > counter-intuitive at the first glance. > > > > Consider it to act as a curtain. Setup the curtain (by writing the fill > > colour property). Then one can close the curtain (by switching source to > > colour), or open it (by switching to any other source). Bitmask wouldn't > > complicate this. > > So if I'm understanding your analogy correctly, pixel_source won't > necessarily be set whenever the FB or solid_fill properties are set. So > that way we can have both FB *and* solid_fill set at the same time, but > only the source that pixel_source is set to would be displayed. Yes. And if the source is not configured, the plane will be marked as not visible. > > Thanks, > > Jessica Zhang > > > > >> > >> Thanks, > >> > >> Jessica Zhang > >> > >>> > >>>> In addition, loosen the NULL FB checks within the atomic commit > >>>> callstack > >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and > >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT > >>>> to a > >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the > >>>> framebuffer to NULL. > >>> > >>> Is there some real reason for the format property? Ie. why not > >>> just do what was the plan for the crttc background color and > >>> specify the color in full 16bpc format and just pick as many > >>> msbs from that as the hw can use? > >>> > >>>> > >>>> Jessica Zhang (3): > >>>> drm: Introduce color fill properties 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 | 68 > >>>> ++++++++++++----------- > >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- > >>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ > >>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ > >>>> drivers/gpu/drm/drm_plane.c | 8 +-- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- > >>>> include/drm/drm_atomic_helper.h | 5 +- > >>>> include/drm/drm_blend.h | 2 + > >>>> include/drm/drm_plane.h | 28 ++++++++++ > >>>> 10 files changed, 188 insertions(+), 76 deletions(-)
On Mon, 26 Jun 2023 16:02:50 -0700 Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > >> properties. When the color fill value is set, and the framebuffer is set > >> to NULL, memory fetch will be disabled. > > > > Thinking a bit more universally I wonder if there should be > > some kind of enum property: > > > > enum plane_pixel_source { > > FB, > > COLOR, > > LIVE_FOO, > > LIVE_BAR, > > ... > > } > > Reviving this thread as this was the initial comment suggesting to > implement pixel_source as an enum. > > I think the issue with having pixel_source as an enum is how to decide > what counts as a NULL commit. > > Currently, setting the FB to NULL will disable the plane. So I'm > guessing we will extend that logic to "if there's no pixel_source set > for the plane, then it will be a NULL commit and disable the plane". > > In that case, the question then becomes when to set the pixel_source to > NONE. Because if we do that when setting a NULL FB (or NULL solid_fill > blob), it then forces userspace to set one property before the other. Right, that won't work. There is no ordering between each property being set inside a single atomic commit. They can all be applied to kernel-internal state theoretically simultaneously, or any arbitrary random order, and the end result must always be the same. Hence, setting one property cannot change the state of another mutable property. I believe that doing otherwise would make userspace fragile and hard to get right. I guess there might be an exception to that rule when the same property is set multiple times in a single atomic commit; the last setting in the array prevails. That's universal and not a special-case between two specific properties. > Because of that, I'm thinking of having pixel_source be represented by a > bitmask instead. That way, we will simply unset the corresponding > pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in > order to detect whether a commit is NULL or has a valid pixel source, we > can just check if pixel_source == 0. Sounds fine to me at first hand, but isn't there the enum property that says if the kernel must look at solid_fill blob *or* FB_ID? If enum prop says "use solid_fill prop", the why would changes to FB_ID do anything? Is it for backwards-compatibility with KMS clients that do not know about the enum prop? It seems like that kind of backwards-compatiblity will cause problems in trying to reason about the atomic state, as explained above, leading to very delicate and fragile conditions where things work intuitively. Hence, I'm not sure backwards-compatibility is wanted. This won't be the first or the last KMS property where an unexpected value left over will make old atomic KMS clients silently malfunction up to showing no recognisable picture at all. *If* that problem needs solving, there have been ideas floating around about resetting everything to nice values so that userspace can ignore what it does not understand. So far there has been no real interest in solving that problem in the kernel though. Legacy non-atomic UAPI wrappers can do whatever they want, and program any (new) properties they want in order to implement the legacy expectations, so that does not seem to be a problem. Thanks, pq > > Would be interested in any feedback on this. > > Thanks, > > Jessica Zhang > > > > >> In addition, loosen the NULL FB checks within the atomic commit callstack > >> to allow a NULL FB when color_fill is nonzero 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.color_fill and > >> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a > >> DRM format, setting COLOR_FILL to a color fill value, and setting the > >> framebuffer to NULL. > > > > Is there some real reason for the format property? Ie. why not > > just do what was the plan for the crttc background color and > > specify the color in full 16bpc format and just pick as many > > msbs from that as the hw can use? > > > >> > >> Jessica Zhang (3): > >> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- > >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- > >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ > >> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ > >> drivers/gpu/drm/drm_plane.c | 8 +-- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- > >> include/drm/drm_atomic_helper.h | 5 +- > >> include/drm/drm_blend.h | 2 + > >> include/drm/drm_plane.h | 28 ++++++++++ > >> 10 files changed, 188 insertions(+), 76 deletions(-) > >> > >> -- > >> 2.38.0 > > > > -- > > Ville Syrjälä > > Intel
On 6/27/2023 12:58 AM, Pekka Paalanen wrote: > On Mon, 26 Jun 2023 16:02:50 -0700 > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>> properties. When the color fill value is set, and the framebuffer is set >>>> to NULL, memory fetch will be disabled. >>> >>> Thinking a bit more universally I wonder if there should be >>> some kind of enum property: >>> >>> enum plane_pixel_source { >>> FB, >>> COLOR, >>> LIVE_FOO, >>> LIVE_BAR, >>> ... >>> } >> >> Reviving this thread as this was the initial comment suggesting to >> implement pixel_source as an enum. >> >> I think the issue with having pixel_source as an enum is how to decide >> what counts as a NULL commit. >> >> Currently, setting the FB to NULL will disable the plane. So I'm >> guessing we will extend that logic to "if there's no pixel_source set >> for the plane, then it will be a NULL commit and disable the plane". >> >> In that case, the question then becomes when to set the pixel_source to >> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill >> blob), it then forces userspace to set one property before the other. > > Right, that won't work. > > There is no ordering between each property being set inside a single > atomic commit. They can all be applied to kernel-internal state > theoretically simultaneously, or any arbitrary random order, and the > end result must always be the same. Hence, setting one property cannot > change the state of another mutable property. I believe that doing > otherwise would make userspace fragile and hard to get right. > > I guess there might be an exception to that rule when the same property > is set multiple times in a single atomic commit; the last setting in > the array prevails. That's universal and not a special-case between two > specific properties. > >> Because of that, I'm thinking of having pixel_source be represented by a >> bitmask instead. That way, we will simply unset the corresponding >> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >> order to detect whether a commit is NULL or has a valid pixel source, we >> can just check if pixel_source == 0. > > Sounds fine to me at first hand, but isn't there the enum property that > says if the kernel must look at solid_fill blob *or* FB_ID? > > If enum prop says "use solid_fill prop", the why would changes to FB_ID > do anything? Is it for backwards-compatibility with KMS clients that do > not know about the enum prop? > > It seems like that kind of backwards-compatiblity will cause problems > in trying to reason about the atomic state, as explained above, leading > to very delicate and fragile conditions where things work intuitively. > Hence, I'm not sure backwards-compatibility is wanted. This won't be > the first or the last KMS property where an unexpected value left over > will make old atomic KMS clients silently malfunction up to showing no > recognisable picture at all. *If* that problem needs solving, there > have been ideas floating around about resetting everything to nice > values so that userspace can ignore what it does not understand. So far > there has been no real interest in solving that problem in the kernel > though. > > Legacy non-atomic UAPI wrappers can do whatever they want, and program > any (new) properties they want in order to implement the legacy > expectations, so that does not seem to be a problem. Hi Pekka and Dmitry, After reading through both of your comments, I think I have a better understanding of the pixel_source implementation now. So to summarize, we want to expose another property called "pixel_source" to userspace that will default to FB (as to not break legacy userspace). If userspace wants to use solid fill planes, it will set both the solid_fill *and* pixel_source properties to a valid blob and COLOR respectively. If it wants to use FB, it will set FB_ID and pixel_source to a valid FB and FB. Here's a table illustrating what I've described above: +-----------------+-------------------------+-------------------------+ | Use Case | Legacy Userspace | solid_fill-aware | | | | Userspace | +=================+=========================+=========================+ | Valid FB | pixel_source = FB | pixel_source = FB | | | FB_ID = valid FB | FB_ID = valid FB | +-----------------+-------------------------+-------------------------+ | Valid | pixel_source = COLOR | N/A | | solid_fill blob | solid_fill = valid blob | | +-----------------+-------------------------+-------------------------+ | NULL commit | pixel_source = FB | pixel_source = FB | | | FB_ID = NULL | FB_ID = NULL | +-----------------+-------------------------+-------------------------+ Is there anything I'm missing or needs to be clarified? Thanks, Jessica Zhang > > > Thanks, > pq > > >> >> Would be interested in any feedback on this. >> >> Thanks, >> >> Jessica Zhang >> >>> >>>> In addition, loosen the NULL FB checks within the atomic commit callstack >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>>> framebuffer to NULL. >>> >>> Is there some real reason for the format property? Ie. why not >>> just do what was the plan for the crttc background color and >>> specify the color in full 16bpc format and just pick as many >>> msbs from that as the hw can use? >>> >>>> >>>> Jessica Zhang (3): >>>> drm: Introduce color fill properties 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 | 68 ++++++++++++----------- >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>>> drivers/gpu/drm/drm_plane.c | 8 +-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++-------- >>>> include/drm/drm_atomic_helper.h | 5 +- >>>> include/drm/drm_blend.h | 2 + >>>> include/drm/drm_plane.h | 28 ++++++++++ >>>> 10 files changed, 188 insertions(+), 76 deletions(-) >>>> >>>> -- >>>> 2.38.0 >>> >>> -- >>> Ville Syrjälä >>> Intel >
On 28/06/2023 00:27, Jessica Zhang wrote: > > > On 6/27/2023 12:58 AM, Pekka Paalanen wrote: >> On Mon, 26 Jun 2023 16:02:50 -0700 >> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >> >>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>>> properties. When the color fill value is set, and the framebuffer >>>>> is set >>>>> to NULL, memory fetch will be disabled. >>>> >>>> Thinking a bit more universally I wonder if there should be >>>> some kind of enum property: >>>> >>>> enum plane_pixel_source { >>>> FB, >>>> COLOR, >>>> LIVE_FOO, >>>> LIVE_BAR, >>>> ... >>>> } >>> >>> Reviving this thread as this was the initial comment suggesting to >>> implement pixel_source as an enum. >>> >>> I think the issue with having pixel_source as an enum is how to decide >>> what counts as a NULL commit. >>> >>> Currently, setting the FB to NULL will disable the plane. So I'm >>> guessing we will extend that logic to "if there's no pixel_source set >>> for the plane, then it will be a NULL commit and disable the plane". >>> >>> In that case, the question then becomes when to set the pixel_source to >>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill >>> blob), it then forces userspace to set one property before the other. >> >> Right, that won't work. >> >> There is no ordering between each property being set inside a single >> atomic commit. They can all be applied to kernel-internal state >> theoretically simultaneously, or any arbitrary random order, and the >> end result must always be the same. Hence, setting one property cannot >> change the state of another mutable property. I believe that doing >> otherwise would make userspace fragile and hard to get right. >> >> I guess there might be an exception to that rule when the same property >> is set multiple times in a single atomic commit; the last setting in >> the array prevails. That's universal and not a special-case between two >> specific properties. >> >>> Because of that, I'm thinking of having pixel_source be represented by a >>> bitmask instead. That way, we will simply unset the corresponding >>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >>> order to detect whether a commit is NULL or has a valid pixel source, we >>> can just check if pixel_source == 0. >> >> Sounds fine to me at first hand, but isn't there the enum property that >> says if the kernel must look at solid_fill blob *or* FB_ID? >> >> If enum prop says "use solid_fill prop", the why would changes to FB_ID >> do anything? Is it for backwards-compatibility with KMS clients that do >> not know about the enum prop? >> >> It seems like that kind of backwards-compatiblity will cause problems >> in trying to reason about the atomic state, as explained above, leading >> to very delicate and fragile conditions where things work intuitively. >> Hence, I'm not sure backwards-compatibility is wanted. This won't be >> the first or the last KMS property where an unexpected value left over >> will make old atomic KMS clients silently malfunction up to showing no >> recognisable picture at all. *If* that problem needs solving, there >> have been ideas floating around about resetting everything to nice >> values so that userspace can ignore what it does not understand. So far >> there has been no real interest in solving that problem in the kernel >> though. >> >> Legacy non-atomic UAPI wrappers can do whatever they want, and program >> any (new) properties they want in order to implement the legacy >> expectations, so that does not seem to be a problem. > > Hi Pekka and Dmitry, > > After reading through both of your comments, I think I have a better > understanding of the pixel_source implementation now. > > So to summarize, we want to expose another property called > "pixel_source" to userspace that will default to FB (as to not break > legacy userspace). > > If userspace wants to use solid fill planes, it will set both the > solid_fill *and* pixel_source properties to a valid blob and COLOR > respectively. If it wants to use FB, it will set FB_ID and pixel_source > to a valid FB and FB. > > Here's a table illustrating what I've described above: > > +-----------------+-------------------------+-------------------------+ > | Use Case | Legacy Userspace | solid_fill-aware | > | | | Userspace | > +=================+=========================+=========================+ > | Valid FB | pixel_source = FB | pixel_source = FB | > | | FB_ID = valid FB | FB_ID = valid FB | > +-----------------+-------------------------+-------------------------+ > | Valid | pixel_source = COLOR | N/A | > | solid_fill blob | solid_fill = valid blob | | Probably these two cells were swapped. > +-----------------+-------------------------+-------------------------+ > | NULL commit | pixel_source = FB | pixel_source = FB | > | | FB_ID = NULL | FB_ID = NULL | > +-----------------+-------------------------+-------------------------+ | or: | pixel_source = COLOR | solid_fill = NULL > > Is there anything I'm missing or needs to be clarified? > LGTM otherwise > Thanks, > > Jessica Zhang > >> >> >> Thanks, >> pq >> >> >>> >>> Would be interested in any feedback on this. >>> >>> Thanks, >>> >>> Jessica Zhang >>> >>>>> In addition, loosen the NULL FB checks within the atomic commit >>>>> callstack >>>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT >>>>> to a >>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>>>> framebuffer to NULL. >>>> >>>> Is there some real reason for the format property? Ie. why not >>>> just do what was the plan for the crttc background color and >>>> specify the color in full 16bpc format and just pick as many >>>> msbs from that as the hw can use? >>>>> >>>>> Jessica Zhang (3): >>>>> drm: Introduce color fill properties 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 | 68 >>>>> ++++++++++++----------- >>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>>>> drivers/gpu/drm/drm_plane.c | 8 +-- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 >>>>> ++++++++++++++-------- >>>>> include/drm/drm_atomic_helper.h | 5 +- >>>>> include/drm/drm_blend.h | 2 + >>>>> include/drm/drm_plane.h | 28 ++++++++++ >>>>> 10 files changed, 188 insertions(+), 76 deletions(-) >>>>> >>>>> -- >>>>> 2.38.0 >>>> >>>> -- >>>> Ville Syrjälä >>>> Intel >>
On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: > On 28/06/2023 00:27, Jessica Zhang wrote: >> >> >> On 6/27/2023 12:58 AM, Pekka Paalanen wrote: >>> On Mon, 26 Jun 2023 16:02:50 -0700 >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>> >>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>>>> properties. When the color fill value is set, and the framebuffer >>>>>> is set >>>>>> to NULL, memory fetch will be disabled. >>>>> >>>>> Thinking a bit more universally I wonder if there should be >>>>> some kind of enum property: >>>>> >>>>> enum plane_pixel_source { >>>>> FB, >>>>> COLOR, >>>>> LIVE_FOO, >>>>> LIVE_BAR, >>>>> ... >>>>> } >>>> >>>> Reviving this thread as this was the initial comment suggesting to >>>> implement pixel_source as an enum. >>>> >>>> I think the issue with having pixel_source as an enum is how to decide >>>> what counts as a NULL commit. >>>> >>>> Currently, setting the FB to NULL will disable the plane. So I'm >>>> guessing we will extend that logic to "if there's no pixel_source set >>>> for the plane, then it will be a NULL commit and disable the plane". >>>> >>>> In that case, the question then becomes when to set the pixel_source to >>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill >>>> blob), it then forces userspace to set one property before the other. >>> >>> Right, that won't work. >>> >>> There is no ordering between each property being set inside a single >>> atomic commit. They can all be applied to kernel-internal state >>> theoretically simultaneously, or any arbitrary random order, and the >>> end result must always be the same. Hence, setting one property cannot >>> change the state of another mutable property. I believe that doing >>> otherwise would make userspace fragile and hard to get right. >>> >>> I guess there might be an exception to that rule when the same property >>> is set multiple times in a single atomic commit; the last setting in >>> the array prevails. That's universal and not a special-case between two >>> specific properties. >>> >>>> Because of that, I'm thinking of having pixel_source be represented >>>> by a >>>> bitmask instead. That way, we will simply unset the corresponding >>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >>>> order to detect whether a commit is NULL or has a valid pixel >>>> source, we >>>> can just check if pixel_source == 0. >>> >>> Sounds fine to me at first hand, but isn't there the enum property that >>> says if the kernel must look at solid_fill blob *or* FB_ID? >>> >>> If enum prop says "use solid_fill prop", the why would changes to FB_ID >>> do anything? Is it for backwards-compatibility with KMS clients that do >>> not know about the enum prop? >>> >>> It seems like that kind of backwards-compatiblity will cause problems >>> in trying to reason about the atomic state, as explained above, leading >>> to very delicate and fragile conditions where things work intuitively. >>> Hence, I'm not sure backwards-compatibility is wanted. This won't be >>> the first or the last KMS property where an unexpected value left over >>> will make old atomic KMS clients silently malfunction up to showing no >>> recognisable picture at all. *If* that problem needs solving, there >>> have been ideas floating around about resetting everything to nice >>> values so that userspace can ignore what it does not understand. So far >>> there has been no real interest in solving that problem in the kernel >>> though. >>> >>> Legacy non-atomic UAPI wrappers can do whatever they want, and program >>> any (new) properties they want in order to implement the legacy >>> expectations, so that does not seem to be a problem. >> >> Hi Pekka and Dmitry, >> >> After reading through both of your comments, I think I have a better >> understanding of the pixel_source implementation now. >> >> So to summarize, we want to expose another property called >> "pixel_source" to userspace that will default to FB (as to not break >> legacy userspace). >> >> If userspace wants to use solid fill planes, it will set both the >> solid_fill *and* pixel_source properties to a valid blob and COLOR >> respectively. If it wants to use FB, it will set FB_ID and >> pixel_source to a valid FB and FB. >> >> Here's a table illustrating what I've described above: >> >> +-----------------+-------------------------+-------------------------+ >> | Use Case | Legacy Userspace | solid_fill-aware | >> | | | Userspace | >> +=================+=========================+=========================+ >> | Valid FB | pixel_source = FB | pixel_source = FB | >> | | FB_ID = valid FB | FB_ID = valid FB | >> +-----------------+-------------------------+-------------------------+ >> | Valid | pixel_source = COLOR | N/A | >> | solid_fill blob | solid_fill = valid blob | | > > Probably these two cells were swapped. > Ack, yes they were swapped. >> +-----------------+-------------------------+-------------------------+ >> | NULL commit | pixel_source = FB | pixel_source = FB | >> | | FB_ID = NULL | FB_ID = NULL | >> +-----------------+-------------------------+-------------------------+ > > | or: > | pixel_source = COLOR > | solid_fill = NULL >> >> Is there anything I'm missing or needs to be clarified? >> > > LGTM otherwise > LGTM too. >> Thanks, >> >> Jessica Zhang >> >>> >>> >>> Thanks, >>> pq >>> >>> >>>> >>>> Would be interested in any feedback on this. >>>> >>>> Thanks, >>>> >>>> Jessica Zhang >>>> >>>>>> In addition, loosen the NULL FB checks within the atomic commit >>>>>> callstack >>>>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting >>>>>> COLOR_FILL_FORMAT to a >>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the >>>>>> framebuffer to NULL. >>>>> >>>>> Is there some real reason for the format property? Ie. why not >>>>> just do what was the plan for the crttc background color and >>>>> specify the color in full 16bpc format and just pick as many >>>>> msbs from that as the hw can use? >>>>>> >>>>>> Jessica Zhang (3): >>>>>> drm: Introduce color fill properties 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 | 68 >>>>>> ++++++++++++----------- >>>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>>>>> drivers/gpu/drm/drm_plane.c | 8 +-- >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 >>>>>> ++++++++++++++-------- >>>>>> include/drm/drm_atomic_helper.h | 5 +- >>>>>> include/drm/drm_blend.h | 2 + >>>>>> include/drm/drm_plane.h | 28 ++++++++++ >>>>>> 10 files changed, 188 insertions(+), 76 deletions(-) >>>>>> >>>>>> -- >>>>>> 2.38.0 >>>>> >>>>> -- >>>>> Ville Syrjälä >>>>> Intel >>> >
On Tue, 27 Jun 2023 15:10:19 -0700 Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: > > On 28/06/2023 00:27, Jessica Zhang wrote: > >> > >> > >> On 6/27/2023 12:58 AM, Pekka Paalanen wrote: > >>> On Mon, 26 Jun 2023 16:02:50 -0700 > >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >>> > >>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > >>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > >>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > >>>>>> properties. When the color fill value is set, and the framebuffer > >>>>>> is set > >>>>>> to NULL, memory fetch will be disabled. > >>>>> > >>>>> Thinking a bit more universally I wonder if there should be > >>>>> some kind of enum property: > >>>>> > >>>>> enum plane_pixel_source { > >>>>> FB, > >>>>> COLOR, > >>>>> LIVE_FOO, > >>>>> LIVE_BAR, > >>>>> ... > >>>>> } > >>>> > >>>> Reviving this thread as this was the initial comment suggesting to > >>>> implement pixel_source as an enum. > >>>> > >>>> I think the issue with having pixel_source as an enum is how to decide > >>>> what counts as a NULL commit. > >>>> > >>>> Currently, setting the FB to NULL will disable the plane. So I'm > >>>> guessing we will extend that logic to "if there's no pixel_source set > >>>> for the plane, then it will be a NULL commit and disable the plane". > >>>> > >>>> In that case, the question then becomes when to set the pixel_source to > >>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill > >>>> blob), it then forces userspace to set one property before the other. > >>> > >>> Right, that won't work. > >>> > >>> There is no ordering between each property being set inside a single > >>> atomic commit. They can all be applied to kernel-internal state > >>> theoretically simultaneously, or any arbitrary random order, and the > >>> end result must always be the same. Hence, setting one property cannot > >>> change the state of another mutable property. I believe that doing > >>> otherwise would make userspace fragile and hard to get right. > >>> > >>> I guess there might be an exception to that rule when the same property > >>> is set multiple times in a single atomic commit; the last setting in > >>> the array prevails. That's universal and not a special-case between two > >>> specific properties. > >>> > >>>> Because of that, I'm thinking of having pixel_source be represented > >>>> by a > >>>> bitmask instead. That way, we will simply unset the corresponding > >>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in > >>>> order to detect whether a commit is NULL or has a valid pixel > >>>> source, we > >>>> can just check if pixel_source == 0. > >>> > >>> Sounds fine to me at first hand, but isn't there the enum property that > >>> says if the kernel must look at solid_fill blob *or* FB_ID? > >>> > >>> If enum prop says "use solid_fill prop", the why would changes to FB_ID > >>> do anything? Is it for backwards-compatibility with KMS clients that do > >>> not know about the enum prop? > >>> > >>> It seems like that kind of backwards-compatiblity will cause problems > >>> in trying to reason about the atomic state, as explained above, leading > >>> to very delicate and fragile conditions where things work intuitively. > >>> Hence, I'm not sure backwards-compatibility is wanted. This won't be > >>> the first or the last KMS property where an unexpected value left over > >>> will make old atomic KMS clients silently malfunction up to showing no > >>> recognisable picture at all. *If* that problem needs solving, there > >>> have been ideas floating around about resetting everything to nice > >>> values so that userspace can ignore what it does not understand. So far > >>> there has been no real interest in solving that problem in the kernel > >>> though. > >>> > >>> Legacy non-atomic UAPI wrappers can do whatever they want, and program > >>> any (new) properties they want in order to implement the legacy > >>> expectations, so that does not seem to be a problem. > >> > >> Hi Pekka and Dmitry, > >> > >> After reading through both of your comments, I think I have a better > >> understanding of the pixel_source implementation now. > >> > >> So to summarize, we want to expose another property called > >> "pixel_source" to userspace that will default to FB (as to not break > >> legacy userspace). > >> > >> If userspace wants to use solid fill planes, it will set both the > >> solid_fill *and* pixel_source properties to a valid blob and COLOR > >> respectively. If it wants to use FB, it will set FB_ID and > >> pixel_source to a valid FB and FB. > >> > >> Here's a table illustrating what I've described above: > >> > >> +-----------------+-------------------------+-------------------------+ > >> | Use Case | Legacy Userspace | solid_fill-aware | > >> | | | Userspace | > >> +=================+=========================+=========================+ > >> | Valid FB | pixel_source = FB | pixel_source = FB | > >> | | FB_ID = valid FB | FB_ID = valid FB | > >> +-----------------+-------------------------+-------------------------+ > >> | Valid | pixel_source = COLOR | N/A | > >> | solid_fill blob | solid_fill = valid blob | | > > > > Probably these two cells were swapped. > > > > Ack, yes they were swapped. > > >> +-----------------+-------------------------+-------------------------+ > >> | NULL commit | pixel_source = FB | pixel_source = FB | > >> | | FB_ID = NULL | FB_ID = NULL | > >> +-----------------+-------------------------+-------------------------+ > > > > | or: > > | pixel_source = COLOR > > | solid_fill = NULL > >> > >> Is there anything I'm missing or needs to be clarified? > >> > > > > LGTM otherwise > > > > LGTM too. Hi, yeah, that sounds fine to me, if everyone thinks that adding a new property for pixel_source is a good idea. I just assumed it was already agreed, and based my comments on that. I don't really remember much of the discussion about a special FB that is actually a solid fill vs. this two new properties design, so I cannot currently give an opinion on that, or any other design. Btw. there may be some confusion about "legacy userspace" which usually refers to pre-atomic userspace, and old atomic userspace that does not understand the new properties. That makes no difference in the table here though, the legacy ioctls should just smash pixel_source. Thanks, pq
On 6/28/2023 12:34 AM, Pekka Paalanen wrote: > On Tue, 27 Jun 2023 15:10:19 -0700 > Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: >>> On 28/06/2023 00:27, Jessica Zhang wrote: >>>> >>>> >>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote: >>>>> On Mon, 26 Jun 2023 16:02:50 -0700 >>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>>>> >>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>>>>>> properties. When the color fill value is set, and the framebuffer >>>>>>>> is set >>>>>>>> to NULL, memory fetch will be disabled. >>>>>>> >>>>>>> Thinking a bit more universally I wonder if there should be >>>>>>> some kind of enum property: >>>>>>> >>>>>>> enum plane_pixel_source { >>>>>>> FB, >>>>>>> COLOR, >>>>>>> LIVE_FOO, >>>>>>> LIVE_BAR, >>>>>>> ... >>>>>>> } >>>>>> >>>>>> Reviving this thread as this was the initial comment suggesting to >>>>>> implement pixel_source as an enum. >>>>>> >>>>>> I think the issue with having pixel_source as an enum is how to decide >>>>>> what counts as a NULL commit. >>>>>> >>>>>> Currently, setting the FB to NULL will disable the plane. So I'm >>>>>> guessing we will extend that logic to "if there's no pixel_source set >>>>>> for the plane, then it will be a NULL commit and disable the plane". >>>>>> >>>>>> In that case, the question then becomes when to set the pixel_source to >>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill >>>>>> blob), it then forces userspace to set one property before the other. >>>>> >>>>> Right, that won't work. >>>>> >>>>> There is no ordering between each property being set inside a single >>>>> atomic commit. They can all be applied to kernel-internal state >>>>> theoretically simultaneously, or any arbitrary random order, and the >>>>> end result must always be the same. Hence, setting one property cannot >>>>> change the state of another mutable property. I believe that doing >>>>> otherwise would make userspace fragile and hard to get right. >>>>> >>>>> I guess there might be an exception to that rule when the same property >>>>> is set multiple times in a single atomic commit; the last setting in >>>>> the array prevails. That's universal and not a special-case between two >>>>> specific properties. >>>>> >>>>>> Because of that, I'm thinking of having pixel_source be represented >>>>>> by a >>>>>> bitmask instead. That way, we will simply unset the corresponding >>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >>>>>> order to detect whether a commit is NULL or has a valid pixel >>>>>> source, we >>>>>> can just check if pixel_source == 0. >>>>> >>>>> Sounds fine to me at first hand, but isn't there the enum property that >>>>> says if the kernel must look at solid_fill blob *or* FB_ID? >>>>> >>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID >>>>> do anything? Is it for backwards-compatibility with KMS clients that do >>>>> not know about the enum prop? >>>>> >>>>> It seems like that kind of backwards-compatiblity will cause problems >>>>> in trying to reason about the atomic state, as explained above, leading >>>>> to very delicate and fragile conditions where things work intuitively. >>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be >>>>> the first or the last KMS property where an unexpected value left over >>>>> will make old atomic KMS clients silently malfunction up to showing no >>>>> recognisable picture at all. *If* that problem needs solving, there >>>>> have been ideas floating around about resetting everything to nice >>>>> values so that userspace can ignore what it does not understand. So far >>>>> there has been no real interest in solving that problem in the kernel >>>>> though. >>>>> >>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program >>>>> any (new) properties they want in order to implement the legacy >>>>> expectations, so that does not seem to be a problem. >>>> >>>> Hi Pekka and Dmitry, >>>> >>>> After reading through both of your comments, I think I have a better >>>> understanding of the pixel_source implementation now. >>>> >>>> So to summarize, we want to expose another property called >>>> "pixel_source" to userspace that will default to FB (as to not break >>>> legacy userspace). >>>> >>>> If userspace wants to use solid fill planes, it will set both the >>>> solid_fill *and* pixel_source properties to a valid blob and COLOR >>>> respectively. If it wants to use FB, it will set FB_ID and >>>> pixel_source to a valid FB and FB. >>>> >>>> Here's a table illustrating what I've described above: >>>> >>>> +-----------------+-------------------------+-------------------------+ >>>> | Use Case | Legacy Userspace | solid_fill-aware | >>>> | | | Userspace | >>>> +=================+=========================+=========================+ >>>> | Valid FB | pixel_source = FB | pixel_source = FB | >>>> | | FB_ID = valid FB | FB_ID = valid FB | >>>> +-----------------+-------------------------+-------------------------+ >>>> | Valid | pixel_source = COLOR | N/A | >>>> | solid_fill blob | solid_fill = valid blob | | >>> >>> Probably these two cells were swapped. >>> >> >> Ack, yes they were swapped. >> >>>> +-----------------+-------------------------+-------------------------+ >>>> | NULL commit | pixel_source = FB | pixel_source = FB | >>>> | | FB_ID = NULL | FB_ID = NULL | >>>> +-----------------+-------------------------+-------------------------+ >>> >>> | or: >>> | pixel_source = COLOR >>> | solid_fill = NULL >>>> >>>> Is there anything I'm missing or needs to be clarified? >>>> >>> >>> LGTM otherwise >>> >> >> LGTM too. > > Hi, > > yeah, that sounds fine to me, if everyone thinks that adding a new > property for pixel_source is a good idea. I just assumed it was already > agreed, and based my comments on that. > > I don't really remember much of the discussion about a special FB that > is actually a solid fill vs. this two new properties design, so I > cannot currently give an opinion on that, or any other design. Hi Pekka, It was discussed in the v3 of this series, but we came to the conclusion that allocating an FB for solid_fill was unnecessary since solid fill does not require memory fetch. Thanks, Jessica Zhang > > Btw. there may be some confusion about "legacy userspace" which usually > refers to pre-atomic userspace, and old atomic userspace that does not > understand the new properties. That makes no difference in the table > here though, the legacy ioctls should just smash pixel_source. > > > Thanks, > pq
On Wed, 28 Jun 2023 09:40:21 -0700 Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > On 6/28/2023 12:34 AM, Pekka Paalanen wrote: > > On Tue, 27 Jun 2023 15:10:19 -0700 > > Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > >> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: > >>> On 28/06/2023 00:27, Jessica Zhang wrote: > >>>> > >>>> > >>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote: > >>>>> On Mon, 26 Jun 2023 16:02:50 -0700 > >>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >>>>> > >>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > >>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > >>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > >>>>>>>> properties. When the color fill value is set, and the framebuffer > >>>>>>>> is set > >>>>>>>> to NULL, memory fetch will be disabled. > >>>>>>> > >>>>>>> Thinking a bit more universally I wonder if there should be > >>>>>>> some kind of enum property: > >>>>>>> > >>>>>>> enum plane_pixel_source { > >>>>>>> FB, > >>>>>>> COLOR, > >>>>>>> LIVE_FOO, > >>>>>>> LIVE_BAR, > >>>>>>> ... > >>>>>>> } > >>>>>> > >>>>>> Reviving this thread as this was the initial comment suggesting to > >>>>>> implement pixel_source as an enum. > >>>>>> > >>>>>> I think the issue with having pixel_source as an enum is how to decide > >>>>>> what counts as a NULL commit. > >>>>>> > >>>>>> Currently, setting the FB to NULL will disable the plane. So I'm > >>>>>> guessing we will extend that logic to "if there's no pixel_source set > >>>>>> for the plane, then it will be a NULL commit and disable the plane". > >>>>>> > >>>>>> In that case, the question then becomes when to set the pixel_source to > >>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill > >>>>>> blob), it then forces userspace to set one property before the other. > >>>>> > >>>>> Right, that won't work. > >>>>> > >>>>> There is no ordering between each property being set inside a single > >>>>> atomic commit. They can all be applied to kernel-internal state > >>>>> theoretically simultaneously, or any arbitrary random order, and the > >>>>> end result must always be the same. Hence, setting one property cannot > >>>>> change the state of another mutable property. I believe that doing > >>>>> otherwise would make userspace fragile and hard to get right. > >>>>> > >>>>> I guess there might be an exception to that rule when the same property > >>>>> is set multiple times in a single atomic commit; the last setting in > >>>>> the array prevails. That's universal and not a special-case between two > >>>>> specific properties. > >>>>> > >>>>>> Because of that, I'm thinking of having pixel_source be represented > >>>>>> by a > >>>>>> bitmask instead. That way, we will simply unset the corresponding > >>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in > >>>>>> order to detect whether a commit is NULL or has a valid pixel > >>>>>> source, we > >>>>>> can just check if pixel_source == 0. > >>>>> > >>>>> Sounds fine to me at first hand, but isn't there the enum property that > >>>>> says if the kernel must look at solid_fill blob *or* FB_ID? > >>>>> > >>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID > >>>>> do anything? Is it for backwards-compatibility with KMS clients that do > >>>>> not know about the enum prop? > >>>>> > >>>>> It seems like that kind of backwards-compatiblity will cause problems > >>>>> in trying to reason about the atomic state, as explained above, leading > >>>>> to very delicate and fragile conditions where things work intuitively. > >>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be > >>>>> the first or the last KMS property where an unexpected value left over > >>>>> will make old atomic KMS clients silently malfunction up to showing no > >>>>> recognisable picture at all. *If* that problem needs solving, there > >>>>> have been ideas floating around about resetting everything to nice > >>>>> values so that userspace can ignore what it does not understand. So far > >>>>> there has been no real interest in solving that problem in the kernel > >>>>> though. > >>>>> > >>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program > >>>>> any (new) properties they want in order to implement the legacy > >>>>> expectations, so that does not seem to be a problem. > >>>> > >>>> Hi Pekka and Dmitry, > >>>> > >>>> After reading through both of your comments, I think I have a better > >>>> understanding of the pixel_source implementation now. > >>>> > >>>> So to summarize, we want to expose another property called > >>>> "pixel_source" to userspace that will default to FB (as to not break > >>>> legacy userspace). > >>>> > >>>> If userspace wants to use solid fill planes, it will set both the > >>>> solid_fill *and* pixel_source properties to a valid blob and COLOR > >>>> respectively. If it wants to use FB, it will set FB_ID and > >>>> pixel_source to a valid FB and FB. > >>>> > >>>> Here's a table illustrating what I've described above: > >>>> > >>>> +-----------------+-------------------------+-------------------------+ > >>>> | Use Case | Legacy Userspace | solid_fill-aware | > >>>> | | | Userspace | > >>>> +=================+=========================+=========================+ > >>>> | Valid FB | pixel_source = FB | pixel_source = FB | > >>>> | | FB_ID = valid FB | FB_ID = valid FB | > >>>> +-----------------+-------------------------+-------------------------+ > >>>> | Valid | pixel_source = COLOR | N/A | > >>>> | solid_fill blob | solid_fill = valid blob | | > >>> > >>> Probably these two cells were swapped. > >>> > >> > >> Ack, yes they were swapped. > >> > >>>> +-----------------+-------------------------+-------------------------+ > >>>> | NULL commit | pixel_source = FB | pixel_source = FB | > >>>> | | FB_ID = NULL | FB_ID = NULL | > >>>> +-----------------+-------------------------+-------------------------+ > >>> > >>> | or: > >>> | pixel_source = COLOR > >>> | solid_fill = NULL > >>>> > >>>> Is there anything I'm missing or needs to be clarified? > >>>> > >>> > >>> LGTM otherwise > >>> > >> > >> LGTM too. > > > > Hi, > > > > yeah, that sounds fine to me, if everyone thinks that adding a new > > property for pixel_source is a good idea. I just assumed it was already > > agreed, and based my comments on that. > > > > I don't really remember much of the discussion about a special FB that > > is actually a solid fill vs. this two new properties design, so I > > cannot currently give an opinion on that, or any other design. > > Hi Pekka, > > It was discussed in the v3 of this series, but we came to the conclusion > that allocating an FB for solid_fill was unnecessary since solid fill > does not require memory fetch. Hi, it just occurred to me that the pixel_source property could be replaced with the rule that if a solid_fill blob id is 0, then use FD_IB. Or vice versa. But if someone then adds a third way of setting content on a plane, it would result in a chain of "if this is 0, then use the next one" and only if all are 0, there is no content. I'm not sure if that's better or worse. Both designs seem to have the same backwards compatibility issues, and the exact same impact to legacy SetCrtc ioctl. Maybe pixel_source property is easier to document and understand though when there is no "if this does not exist or is 0 then ..." chain. So, pixel_source is fine by me. Thanks, pq
On 6/27/2023 3:10 PM, Abhinav Kumar wrote: > > > On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: >> On 28/06/2023 00:27, Jessica Zhang wrote: >>> >>> >>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote: >>>> On Mon, 26 Jun 2023 16:02:50 -0700 >>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>>> >>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>>>>> properties. When the color fill value is set, and the framebuffer >>>>>>> is set >>>>>>> to NULL, memory fetch will be disabled. >>>>>> >>>>>> Thinking a bit more universally I wonder if there should be >>>>>> some kind of enum property: >>>>>> >>>>>> enum plane_pixel_source { >>>>>> FB, >>>>>> COLOR, >>>>>> LIVE_FOO, >>>>>> LIVE_BAR, >>>>>> ... >>>>>> } >>>>> >>>>> Reviving this thread as this was the initial comment suggesting to >>>>> implement pixel_source as an enum. >>>>> >>>>> I think the issue with having pixel_source as an enum is how to decide >>>>> what counts as a NULL commit. >>>>> >>>>> Currently, setting the FB to NULL will disable the plane. So I'm >>>>> guessing we will extend that logic to "if there's no pixel_source set >>>>> for the plane, then it will be a NULL commit and disable the plane". >>>>> >>>>> In that case, the question then becomes when to set the >>>>> pixel_source to >>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill >>>>> blob), it then forces userspace to set one property before the other. >>>> >>>> Right, that won't work. >>>> >>>> There is no ordering between each property being set inside a single >>>> atomic commit. They can all be applied to kernel-internal state >>>> theoretically simultaneously, or any arbitrary random order, and the >>>> end result must always be the same. Hence, setting one property cannot >>>> change the state of another mutable property. I believe that doing >>>> otherwise would make userspace fragile and hard to get right. >>>> >>>> I guess there might be an exception to that rule when the same property >>>> is set multiple times in a single atomic commit; the last setting in >>>> the array prevails. That's universal and not a special-case between two >>>> specific properties. >>>> >>>>> Because of that, I'm thinking of having pixel_source be represented >>>>> by a >>>>> bitmask instead. That way, we will simply unset the corresponding >>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >>>>> order to detect whether a commit is NULL or has a valid pixel >>>>> source, we >>>>> can just check if pixel_source == 0. >>>> >>>> Sounds fine to me at first hand, but isn't there the enum property that >>>> says if the kernel must look at solid_fill blob *or* FB_ID? >>>> >>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID >>>> do anything? Is it for backwards-compatibility with KMS clients that do >>>> not know about the enum prop? >>>> >>>> It seems like that kind of backwards-compatiblity will cause problems >>>> in trying to reason about the atomic state, as explained above, leading >>>> to very delicate and fragile conditions where things work intuitively. >>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be >>>> the first or the last KMS property where an unexpected value left over >>>> will make old atomic KMS clients silently malfunction up to showing no >>>> recognisable picture at all. *If* that problem needs solving, there >>>> have been ideas floating around about resetting everything to nice >>>> values so that userspace can ignore what it does not understand. So far >>>> there has been no real interest in solving that problem in the kernel >>>> though. >>>> >>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program >>>> any (new) properties they want in order to implement the legacy >>>> expectations, so that does not seem to be a problem. >>> >>> Hi Pekka and Dmitry, >>> >>> After reading through both of your comments, I think I have a better >>> understanding of the pixel_source implementation now. >>> >>> So to summarize, we want to expose another property called >>> "pixel_source" to userspace that will default to FB (as to not break >>> legacy userspace). >>> >>> If userspace wants to use solid fill planes, it will set both the >>> solid_fill *and* pixel_source properties to a valid blob and COLOR >>> respectively. If it wants to use FB, it will set FB_ID and >>> pixel_source to a valid FB and FB. >>> >>> Here's a table illustrating what I've described above: >>> >>> +-----------------+-------------------------+-------------------------+ >>> | Use Case | Legacy Userspace | solid_fill-aware | >>> | | | Userspace | >>> +=================+=========================+=========================+ >>> | Valid FB | pixel_source = FB | pixel_source = FB | >>> | | FB_ID = valid FB | FB_ID = valid FB | >>> +-----------------+-------------------------+-------------------------+ >>> | Valid | pixel_source = COLOR | N/A | >>> | solid_fill blob | solid_fill = valid blob | | >> >> Probably these two cells were swapped. >> > > Ack, yes they were swapped. > >>> +-----------------+-------------------------+-------------------------+ >>> | NULL commit | pixel_source = FB | pixel_source = FB | >>> | | FB_ID = NULL | FB_ID = NULL | >>> +-----------------+-------------------------+-------------------------+ >> >> | or: >> | pixel_source = COLOR >> | solid_fill = NULL Hi Dmitry and Abhinav, Sorry, forgot to respond to this comment. Yep, if pixel_source is set to COLOR, then a NULL solid_fill blob will count as a NULL commit. >>> >>> Is there anything I'm missing or needs to be clarified? >>> >> >> LGTM otherwise >> > > LGTM too. Thanks for the feedback! BR, Jessica Zhang > >>> Thanks, >>> >>> Jessica Zhang >>> >>>> >>>> >>>> Thanks, >>>> pq >>>> >>>> >>>>> >>>>> Would be interested in any feedback on this. >>>>> >>>>> Thanks, >>>>> >>>>> Jessica Zhang >>>>> >>>>>>> In addition, loosen the NULL FB checks within the atomic commit >>>>>>> callstack >>>>>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and >>>>>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting >>>>>>> COLOR_FILL_FORMAT to a >>>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting >>>>>>> the >>>>>>> framebuffer to NULL. >>>>>> >>>>>> Is there some real reason for the format property? Ie. why not >>>>>> just do what was the plan for the crttc background color and >>>>>> specify the color in full 16bpc format and just pick as many >>>>>> msbs from that as the hw can use? >>>>>>> >>>>>>> Jessica Zhang (3): >>>>>>> drm: Introduce color fill properties 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 | 68 >>>>>>> ++++++++++++----------- >>>>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++----- >>>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++ >>>>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++ >>>>>>> drivers/gpu/drm/drm_plane.c | 8 +-- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 >>>>>>> ++++++++++++++-------- >>>>>>> include/drm/drm_atomic_helper.h | 5 +- >>>>>>> include/drm/drm_blend.h | 2 + >>>>>>> include/drm/drm_plane.h | 28 ++++++++++ >>>>>>> 10 files changed, 188 insertions(+), 76 deletions(-) >>>>>>> >>>>>>> -- >>>>>>> 2.38.0 >>>>>> >>>>>> -- >>>>>> Ville Syrjälä >>>>>> Intel >>>> >>
On 6/29/2023 12:29 AM, Pekka Paalanen wrote: > On Wed, 28 Jun 2023 09:40:21 -0700 > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> On 6/28/2023 12:34 AM, Pekka Paalanen wrote: >>> On Tue, 27 Jun 2023 15:10:19 -0700 >>> Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>> >>>> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: >>>>> On 28/06/2023 00:27, Jessica Zhang wrote: >>>>>> >>>>>> >>>>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote: >>>>>>> On Mon, 26 Jun 2023 16:02:50 -0700 >>>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>>>>>> >>>>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: >>>>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: >>>>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT >>>>>>>>>> properties. When the color fill value is set, and the framebuffer >>>>>>>>>> is set >>>>>>>>>> to NULL, memory fetch will be disabled. >>>>>>>>> >>>>>>>>> Thinking a bit more universally I wonder if there should be >>>>>>>>> some kind of enum property: >>>>>>>>> >>>>>>>>> enum plane_pixel_source { >>>>>>>>> FB, >>>>>>>>> COLOR, >>>>>>>>> LIVE_FOO, >>>>>>>>> LIVE_BAR, >>>>>>>>> ... >>>>>>>>> } >>>>>>>> >>>>>>>> Reviving this thread as this was the initial comment suggesting to >>>>>>>> implement pixel_source as an enum. >>>>>>>> >>>>>>>> I think the issue with having pixel_source as an enum is how to decide >>>>>>>> what counts as a NULL commit. >>>>>>>> >>>>>>>> Currently, setting the FB to NULL will disable the plane. So I'm >>>>>>>> guessing we will extend that logic to "if there's no pixel_source set >>>>>>>> for the plane, then it will be a NULL commit and disable the plane". >>>>>>>> >>>>>>>> In that case, the question then becomes when to set the pixel_source to >>>>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill >>>>>>>> blob), it then forces userspace to set one property before the other. >>>>>>> >>>>>>> Right, that won't work. >>>>>>> >>>>>>> There is no ordering between each property being set inside a single >>>>>>> atomic commit. They can all be applied to kernel-internal state >>>>>>> theoretically simultaneously, or any arbitrary random order, and the >>>>>>> end result must always be the same. Hence, setting one property cannot >>>>>>> change the state of another mutable property. I believe that doing >>>>>>> otherwise would make userspace fragile and hard to get right. >>>>>>> >>>>>>> I guess there might be an exception to that rule when the same property >>>>>>> is set multiple times in a single atomic commit; the last setting in >>>>>>> the array prevails. That's universal and not a special-case between two >>>>>>> specific properties. >>>>>>> >>>>>>>> Because of that, I'm thinking of having pixel_source be represented >>>>>>>> by a >>>>>>>> bitmask instead. That way, we will simply unset the corresponding >>>>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in >>>>>>>> order to detect whether a commit is NULL or has a valid pixel >>>>>>>> source, we >>>>>>>> can just check if pixel_source == 0. >>>>>>> >>>>>>> Sounds fine to me at first hand, but isn't there the enum property that >>>>>>> says if the kernel must look at solid_fill blob *or* FB_ID? >>>>>>> >>>>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID >>>>>>> do anything? Is it for backwards-compatibility with KMS clients that do >>>>>>> not know about the enum prop? >>>>>>> >>>>>>> It seems like that kind of backwards-compatiblity will cause problems >>>>>>> in trying to reason about the atomic state, as explained above, leading >>>>>>> to very delicate and fragile conditions where things work intuitively. >>>>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be >>>>>>> the first or the last KMS property where an unexpected value left over >>>>>>> will make old atomic KMS clients silently malfunction up to showing no >>>>>>> recognisable picture at all. *If* that problem needs solving, there >>>>>>> have been ideas floating around about resetting everything to nice >>>>>>> values so that userspace can ignore what it does not understand. So far >>>>>>> there has been no real interest in solving that problem in the kernel >>>>>>> though. >>>>>>> >>>>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program >>>>>>> any (new) properties they want in order to implement the legacy >>>>>>> expectations, so that does not seem to be a problem. >>>>>> >>>>>> Hi Pekka and Dmitry, >>>>>> >>>>>> After reading through both of your comments, I think I have a better >>>>>> understanding of the pixel_source implementation now. >>>>>> >>>>>> So to summarize, we want to expose another property called >>>>>> "pixel_source" to userspace that will default to FB (as to not break >>>>>> legacy userspace). >>>>>> >>>>>> If userspace wants to use solid fill planes, it will set both the >>>>>> solid_fill *and* pixel_source properties to a valid blob and COLOR >>>>>> respectively. If it wants to use FB, it will set FB_ID and >>>>>> pixel_source to a valid FB and FB. >>>>>> >>>>>> Here's a table illustrating what I've described above: >>>>>> >>>>>> +-----------------+-------------------------+-------------------------+ >>>>>> | Use Case | Legacy Userspace | solid_fill-aware | >>>>>> | | | Userspace | >>>>>> +=================+=========================+=========================+ >>>>>> | Valid FB | pixel_source = FB | pixel_source = FB | >>>>>> | | FB_ID = valid FB | FB_ID = valid FB | >>>>>> +-----------------+-------------------------+-------------------------+ >>>>>> | Valid | pixel_source = COLOR | N/A | >>>>>> | solid_fill blob | solid_fill = valid blob | | >>>>> >>>>> Probably these two cells were swapped. >>>>> >>>> >>>> Ack, yes they were swapped. >>>> >>>>>> +-----------------+-------------------------+-------------------------+ >>>>>> | NULL commit | pixel_source = FB | pixel_source = FB | >>>>>> | | FB_ID = NULL | FB_ID = NULL | >>>>>> +-----------------+-------------------------+-------------------------+ >>>>> >>>>> | or: >>>>> | pixel_source = COLOR >>>>> | solid_fill = NULL >>>>>> >>>>>> Is there anything I'm missing or needs to be clarified? >>>>>> >>>>> >>>>> LGTM otherwise >>>>> >>>> >>>> LGTM too. >>> >>> Hi, >>> >>> yeah, that sounds fine to me, if everyone thinks that adding a new >>> property for pixel_source is a good idea. I just assumed it was already >>> agreed, and based my comments on that. >>> >>> I don't really remember much of the discussion about a special FB that >>> is actually a solid fill vs. this two new properties design, so I >>> cannot currently give an opinion on that, or any other design. >> >> Hi Pekka, >> >> It was discussed in the v3 of this series, but we came to the conclusion >> that allocating an FB for solid_fill was unnecessary since solid fill >> does not require memory fetch. > > Hi, > > it just occurred to me that the pixel_source property could be replaced > with the rule that if a solid_fill blob id is 0, then use FD_IB. Or > vice versa. But if someone then adds a third way of setting content on > a plane, it would result in a chain of "if this is 0, then use the next > one" and only if all are 0, there is no content. Hi Pekka, Agreed. I would prefer having a pixel_source property as it will be more extendable than having arbitrary checks. > > I'm not sure if that's better or worse. Both designs seem to have the > same backwards compatibility issues, and the exact same impact to > legacy SetCrtc ioctl. Maybe pixel_source property is easier to document > and understand though when there is no "if this does not exist or is 0 > then ..." chain. FWIW, the solid_fill feature will require both the solid_fill and pixel_source properties. I'll make a note of this in the cover letter. > > So, pixel_source is fine by me. Awesome, thanks for the feedback! Will push a v4 with these changes. Thanks, Jessica Zhang > > > Thanks, > pq