| Message ID | 20230828-solid-fill-v6-7-a820efcce852@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for Solid Fill Planes | expand |
On Mon, 28 Aug 2023 17:05:13 -0700 Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > Loosen the requirements for atomic and legacy commit so that, in cases > where pixel_source != FB, the commit can still go through. > > This includes adding framebuffer NULL checks in other areas to account for > FB being NULL when non-FB pixel sources are enabled. > > To disable a plane, the pixel_source must be NONE or the FB must be NULL > if pixel_source == FB. > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- > drivers/gpu/drm/drm_atomic_helper.c | 36 ++++++++++++++++++++---------------- > include/drm/drm_atomic_helper.h | 4 ++-- > include/drm/drm_plane.h | 29 +++++++++++++++++++++++++++++ > 4 files changed, 62 insertions(+), 27 deletions(-) ... > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index a58f84b6bd5e..4c5b7bcdb25c 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -992,6 +992,35 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev, > #define drm_for_each_plane(plane, dev) \ > list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) > > +/** > + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane > + * @state: plane state > + * > + * Returns: > + * Whether the plane has been assigned a solid_fill_blob > + */ > +static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state *state) > +{ > + if (!state) > + return false; > + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL && state->solid_fill_blob; > +} > + > +static inline bool drm_plane_has_visible_data(const struct drm_plane_state *state) > +{ > + switch (state->pixel_source) { > + case DRM_PLANE_PIXEL_SOURCE_NONE: > + return false; > + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: > + return state->solid_fill_blob != NULL; This reminds me, new UAPI docs did not say what the requirements are for choosing solid fill pixel source. Is the atomic commit rejected if pixel source is solid fill, but solid_fill property has no blob? This should be doc'd. Thanks, pq > + case DRM_PLANE_PIXEL_SOURCE_FB: > + default: > + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); > + } > + > + return state->fb != NULL; > +} > + > bool drm_any_plane_has_format(struct drm_device *dev, > u32 format, u64 modifier); > >
On 8/29/2023 1:22 AM, Pekka Paalanen wrote: > On Mon, 28 Aug 2023 17:05:13 -0700 > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> Loosen the requirements for atomic and legacy commit so that, in cases >> where pixel_source != FB, the commit can still go through. >> >> This includes adding framebuffer NULL checks in other areas to account for >> FB being NULL when non-FB pixel sources are enabled. >> >> To disable a plane, the pixel_source must be NONE or the FB must be NULL >> if pixel_source == FB. >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- >> drivers/gpu/drm/drm_atomic_helper.c | 36 ++++++++++++++++++++---------------- >> include/drm/drm_atomic_helper.h | 4 ++-- >> include/drm/drm_plane.h | 29 +++++++++++++++++++++++++++++ >> 4 files changed, 62 insertions(+), 27 deletions(-) > > ... > >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index a58f84b6bd5e..4c5b7bcdb25c 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -992,6 +992,35 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev, >> #define drm_for_each_plane(plane, dev) \ >> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) >> >> +/** >> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane >> + * @state: plane state >> + * >> + * Returns: >> + * Whether the plane has been assigned a solid_fill_blob >> + */ >> +static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state *state) >> +{ >> + if (!state) >> + return false; >> + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL && state->solid_fill_blob; >> +} >> + >> +static inline bool drm_plane_has_visible_data(const struct drm_plane_state *state) >> +{ >> + switch (state->pixel_source) { >> + case DRM_PLANE_PIXEL_SOURCE_NONE: >> + return false; >> + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: >> + return state->solid_fill_blob != NULL; > > This reminds me, new UAPI docs did not say what the requirements are for > choosing solid fill pixel source. Is the atomic commit rejected if > pixel source is solid fill, but solid_fill property has no blob? Hi Pekka, Yes, if pixel_source is solid_fill and the solid_fill property blob isn't set, the atomic commit should throw an error. Will document this in the UAPI. Thanks, Jessica Zhang > > This should be doc'd. > > > Thanks, > pq > >> + case DRM_PLANE_PIXEL_SOURCE_FB: >> + default: >> + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); >> + } >> + >> + return state->fb != NULL; >> +} >> + >> bool drm_any_plane_has_format(struct drm_device *dev, >> u32 format, u64 modifier); >> >> >
On 29/08/2023 03:05, Jessica Zhang wrote: > Loosen the requirements for atomic and legacy commit so that, in cases > where pixel_source != FB, the commit can still go through. > > This includes adding framebuffer NULL checks in other areas to account for > FB being NULL when non-FB pixel sources are enabled. > > To disable a plane, the pixel_source must be NONE or the FB must be NULL > if pixel_source == FB. > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- > drivers/gpu/drm/drm_atomic_helper.c | 36 ++++++++++++++++++++---------------- > include/drm/drm_atomic_helper.h | 4 ++-- > include/drm/drm_plane.h | 29 +++++++++++++++++++++++++++++ > 4 files changed, 62 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index cc0e93d19e15..cdc6cfedd433 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -668,14 +668,14 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, > const struct drm_framebuffer *fb = new_plane_state->fb; > int ret; > > - /* either *both* CRTC and FB must be set, or neither */ > - if (crtc && !fb) { > - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n", > + /* either *both* CRTC and pixel source must be set, or neither */ > + if (crtc && !drm_plane_has_visible_data(new_plane_state)) { > + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no visible data\n", > plane->base.id, plane->name); > return -EINVAL; > - } else if (fb && !crtc) { > - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] FB set but no CRTC\n", > - plane->base.id, plane->name); > + } else if (drm_plane_has_visible_data(new_plane_state) && !crtc) { > + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Source %d has visible data but no CRTC\n", > + plane->base.id, plane->name, new_plane_state->pixel_source); > return -EINVAL; > } > > @@ -706,9 +706,11 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, > } > > > - ret = drm_atomic_plane_check_fb(new_plane_state); > - if (ret) > - return ret; > + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { Nit: could you please be more specific here? Drop the fb variable and use new_plane_state->fb directly. > + ret = drm_atomic_plane_check_fb(new_plane_state); > + if (ret) > + return ret; > + } > > if (plane_switching_crtc(old_plane_state, new_plane_state)) { > drm_dbg_atomic(plane->dev, > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 41b8066f61ff..a176064ee27e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > *src = drm_plane_state_src(plane_state); > *dst = drm_plane_state_dest(plane_state); > > - if (!fb) { > + if (!drm_plane_has_visible_data(plane_state)) { > plane_state->visible = false; > return 0; > } > @@ -881,25 +881,29 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > return -EINVAL; > } > > - drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); > + if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { And here too. Could you please move fb var into the condition? Other than that LGTM > + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); > > - /* Check scaling */ > - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); > - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); > - if (hscale < 0 || vscale < 0) { > - drm_dbg_kms(plane_state->plane->dev, > - "Invalid scaling of plane\n"); > - drm_rect_debug_print("src: ", &plane_state->src, true); > - drm_rect_debug_print("dst: ", &plane_state->dst, false); > - return -ERANGE; > - } > + /* Check scaling */ > + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); > + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); > > - if (crtc_state->enable) > - drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); > + if (hscale < 0 || vscale < 0) { > + drm_dbg_kms(plane_state->plane->dev, > + "Invalid scaling of plane\n"); > + drm_rect_debug_print("src: ", &plane_state->src, true); > + drm_rect_debug_print("dst: ", &plane_state->dst, false); > + return -ERANGE; > + } > > - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); > + if (crtc_state->enable) > + drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); > > - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); > + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); > + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); > + } else if (drm_plane_solid_fill_enabled(plane_state)) { > + plane_state->visible = true; > + } > > if (!plane_state->visible) > /* > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 536a0b0091c3..6d97f38ac1f6 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -256,8 +256,8 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state, > * Anything else should be considered a bug in the atomic core, so we > * gently warn about it. > */ > - WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) || > - (new_plane_state->crtc != NULL && new_plane_state->fb == NULL)); > + WARN_ON((new_plane_state->crtc == NULL && drm_plane_has_visible_data(new_plane_state)) || > + (new_plane_state->crtc != NULL && !drm_plane_has_visible_data(new_plane_state))); > > return old_plane_state->crtc && !new_plane_state->crtc; > } > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index a58f84b6bd5e..4c5b7bcdb25c 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -992,6 +992,35 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev, > #define drm_for_each_plane(plane, dev) \ > list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) > > +/** > + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane > + * @state: plane state > + * > + * Returns: > + * Whether the plane has been assigned a solid_fill_blob > + */ > +static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state *state) > +{ > + if (!state) > + return false; > + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL && state->solid_fill_blob; > +} > + > +static inline bool drm_plane_has_visible_data(const struct drm_plane_state *state) > +{ > + switch (state->pixel_source) { > + case DRM_PLANE_PIXEL_SOURCE_NONE: > + return false; > + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: > + return state->solid_fill_blob != NULL; > + case DRM_PLANE_PIXEL_SOURCE_FB: > + default: > + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); > + } > + > + return state->fb != NULL; > +} > + > bool drm_any_plane_has_format(struct drm_device *dev, > u32 format, u64 modifier); > >
On 22/09/2023 20:49, Jessica Zhang wrote: > > > On 8/29/2023 1:22 AM, Pekka Paalanen wrote: >> On Mon, 28 Aug 2023 17:05:13 -0700 >> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >> >>> Loosen the requirements for atomic and legacy commit so that, in cases >>> where pixel_source != FB, the commit can still go through. >>> >>> This includes adding framebuffer NULL checks in other areas to >>> account for >>> FB being NULL when non-FB pixel sources are enabled. >>> >>> To disable a plane, the pixel_source must be NONE or the FB must be NULL >>> if pixel_source == FB. >>> >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- >>> drivers/gpu/drm/drm_atomic_helper.c | 36 >>> ++++++++++++++++++++---------------- >>> include/drm/drm_atomic_helper.h | 4 ++-- >>> include/drm/drm_plane.h | 29 +++++++++++++++++++++++++++++ >>> 4 files changed, 62 insertions(+), 27 deletions(-) >> >> ... >> >>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>> index a58f84b6bd5e..4c5b7bcdb25c 100644 >>> --- a/include/drm/drm_plane.h >>> +++ b/include/drm/drm_plane.h >>> @@ -992,6 +992,35 @@ static inline struct drm_plane >>> *drm_plane_find(struct drm_device *dev, >>> #define drm_for_each_plane(plane, dev) \ >>> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) >>> +/** >>> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on >>> plane >>> + * @state: plane state >>> + * >>> + * Returns: >>> + * Whether the plane has been assigned a solid_fill_blob >>> + */ >>> +static inline bool drm_plane_solid_fill_enabled(struct >>> drm_plane_state *state) >>> +{ >>> + if (!state) >>> + return false; >>> + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL >>> && state->solid_fill_blob; >>> +} >>> + >>> +static inline bool drm_plane_has_visible_data(const struct >>> drm_plane_state *state) >>> +{ >>> + switch (state->pixel_source) { >>> + case DRM_PLANE_PIXEL_SOURCE_NONE: >>> + return false; >>> + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: >>> + return state->solid_fill_blob != NULL; >> >> This reminds me, new UAPI docs did not say what the requirements are for >> choosing solid fill pixel source. Is the atomic commit rejected if >> pixel source is solid fill, but solid_fill property has no blob? > > Hi Pekka, > > Yes, if pixel_source is solid_fill and the solid_fill property blob > isn't set, the atomic commit should throw an error. > > Will document this in the UAPI. I don't see a corresponding error check in atomic_check() functions. Could you please check that there is one, as you are updating the uAPI. > > Thanks, > > Jessica Zhang > >> >> This should be doc'd. >> >> >> Thanks, >> pq >> >>> + case DRM_PLANE_PIXEL_SOURCE_FB: >>> + default: >>> + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); >>> + } >>> + >>> + return state->fb != NULL; >>> +} >>> + >>> bool drm_any_plane_has_format(struct drm_device *dev, >>> u32 format, u64 modifier); >>> >>
On 9/24/2023 3:23 AM, Dmitry Baryshkov wrote: > On 22/09/2023 20:49, Jessica Zhang wrote: >> >> >> On 8/29/2023 1:22 AM, Pekka Paalanen wrote: >>> On Mon, 28 Aug 2023 17:05:13 -0700 >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>> >>>> Loosen the requirements for atomic and legacy commit so that, in cases >>>> where pixel_source != FB, the commit can still go through. >>>> >>>> This includes adding framebuffer NULL checks in other areas to >>>> account for >>>> FB being NULL when non-FB pixel sources are enabled. >>>> >>>> To disable a plane, the pixel_source must be NONE or the FB must be >>>> NULL >>>> if pixel_source == FB. >>>> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- >>>> drivers/gpu/drm/drm_atomic_helper.c | 36 >>>> ++++++++++++++++++++---------------- >>>> include/drm/drm_atomic_helper.h | 4 ++-- >>>> include/drm/drm_plane.h | 29 >>>> +++++++++++++++++++++++++++++ >>>> 4 files changed, 62 insertions(+), 27 deletions(-) >>> >>> ... >>> >>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>>> index a58f84b6bd5e..4c5b7bcdb25c 100644 >>>> --- a/include/drm/drm_plane.h >>>> +++ b/include/drm/drm_plane.h >>>> @@ -992,6 +992,35 @@ static inline struct drm_plane >>>> *drm_plane_find(struct drm_device *dev, >>>> #define drm_for_each_plane(plane, dev) \ >>>> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) >>>> +/** >>>> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on >>>> plane >>>> + * @state: plane state >>>> + * >>>> + * Returns: >>>> + * Whether the plane has been assigned a solid_fill_blob >>>> + */ >>>> +static inline bool drm_plane_solid_fill_enabled(struct >>>> drm_plane_state *state) >>>> +{ >>>> + if (!state) >>>> + return false; >>>> + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL >>>> && state->solid_fill_blob; >>>> +} >>>> + >>>> +static inline bool drm_plane_has_visible_data(const struct >>>> drm_plane_state *state) >>>> +{ >>>> + switch (state->pixel_source) { >>>> + case DRM_PLANE_PIXEL_SOURCE_NONE: >>>> + return false; >>>> + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: >>>> + return state->solid_fill_blob != NULL; >>> >>> This reminds me, new UAPI docs did not say what the requirements are for >>> choosing solid fill pixel source. Is the atomic commit rejected if >>> pixel source is solid fill, but solid_fill property has no blob? >> >> Hi Pekka, >> >> Yes, if pixel_source is solid_fill and the solid_fill property blob >> isn't set, the atomic commit should throw an error. >> >> Will document this in the UAPI. > > I don't see a corresponding error check in atomic_check() functions. > Could you please check that there is one, as you are updating the uAPI. Hi Dmitry, Sorry for the late response. drm_plane_has_visible_data() is being called from drm_atomic_plane_check() which is called from drm_atomic_commit() (via drm_atomic_check_only()). It's also called within the atomic_check() callstack in drm_atomic_helper_check_plane_state(), though that check will set plane.visible to false and return 0. Would it be better to have a more explicit `if (source == solid_fill && !plane->solid_fill_blob) then return -EINVAL` check in atomic_check()? Thanks, Jessica Zhang > >> >> Thanks, >> >> Jessica Zhang >> >>> >>> This should be doc'd. >>> >>> >>> Thanks, >>> pq >>> >>>> + case DRM_PLANE_PIXEL_SOURCE_FB: >>>> + default: >>>> + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); >>>> + } >>>> + >>>> + return state->fb != NULL; >>>> +} >>>> + >>>> bool drm_any_plane_has_format(struct drm_device *dev, >>>> u32 format, u64 modifier); >>>> >>> > > -- > With best wishes > Dmitry >
Hi Jessica, On Tue, 17 Oct 2023 at 03:41, Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > On 9/24/2023 3:23 AM, Dmitry Baryshkov wrote: > > On 22/09/2023 20:49, Jessica Zhang wrote: > >> > >> > >> On 8/29/2023 1:22 AM, Pekka Paalanen wrote: > >>> On Mon, 28 Aug 2023 17:05:13 -0700 > >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >>> > >>>> Loosen the requirements for atomic and legacy commit so that, in cases > >>>> where pixel_source != FB, the commit can still go through. > >>>> > >>>> This includes adding framebuffer NULL checks in other areas to > >>>> account for > >>>> FB being NULL when non-FB pixel sources are enabled. > >>>> > >>>> To disable a plane, the pixel_source must be NONE or the FB must be > >>>> NULL > >>>> if pixel_source == FB. > >>>> > >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > >>>> --- > >>>> drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- > >>>> drivers/gpu/drm/drm_atomic_helper.c | 36 > >>>> ++++++++++++++++++++---------------- > >>>> include/drm/drm_atomic_helper.h | 4 ++-- > >>>> include/drm/drm_plane.h | 29 > >>>> +++++++++++++++++++++++++++++ > >>>> 4 files changed, 62 insertions(+), 27 deletions(-) > >>> > >>> ... > >>> > >>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > >>>> index a58f84b6bd5e..4c5b7bcdb25c 100644 > >>>> --- a/include/drm/drm_plane.h > >>>> +++ b/include/drm/drm_plane.h > >>>> @@ -992,6 +992,35 @@ static inline struct drm_plane > >>>> *drm_plane_find(struct drm_device *dev, > >>>> #define drm_for_each_plane(plane, dev) \ > >>>> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) > >>>> +/** > >>>> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on > >>>> plane > >>>> + * @state: plane state > >>>> + * > >>>> + * Returns: > >>>> + * Whether the plane has been assigned a solid_fill_blob > >>>> + */ > >>>> +static inline bool drm_plane_solid_fill_enabled(struct > >>>> drm_plane_state *state) > >>>> +{ > >>>> + if (!state) > >>>> + return false; > >>>> + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL > >>>> && state->solid_fill_blob; > >>>> +} > >>>> + > >>>> +static inline bool drm_plane_has_visible_data(const struct > >>>> drm_plane_state *state) > >>>> +{ > >>>> + switch (state->pixel_source) { > >>>> + case DRM_PLANE_PIXEL_SOURCE_NONE: > >>>> + return false; > >>>> + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: > >>>> + return state->solid_fill_blob != NULL; > >>> > >>> This reminds me, new UAPI docs did not say what the requirements are for > >>> choosing solid fill pixel source. Is the atomic commit rejected if > >>> pixel source is solid fill, but solid_fill property has no blob? > >> > >> Hi Pekka, > >> > >> Yes, if pixel_source is solid_fill and the solid_fill property blob > >> isn't set, the atomic commit should throw an error. > >> > >> Will document this in the UAPI. > > > > I don't see a corresponding error check in atomic_check() functions. > > Could you please check that there is one, as you are updating the uAPI. > > Hi Dmitry, > > Sorry for the late response. No worries. > > drm_plane_has_visible_data() is being called from > drm_atomic_plane_check() which is called from drm_atomic_commit() (via > drm_atomic_check_only()). > > It's also called within the atomic_check() callstack in > drm_atomic_helper_check_plane_state(), though that check will set > plane.visible to false and return 0. > > Would it be better to have a more explicit `if (source == solid_fill && > !plane->solid_fill_blob) then return -EINVAL` check in atomic_check()? No. Your current code is good already. It was me who missed the visible data check. If you are going to send the next version for some reason, it might be good to add a small comment to drm_atomic_helper_check_plane_state(). Something like 'check that the selected pixel source (fb, solid fill, etc.) is valid'. > > Thanks, > > Jessica Zhang > > > > >> > >> Thanks, > >> > >> Jessica Zhang > >> > >>> > >>> This should be doc'd. > >>> > >>> > >>> Thanks, > >>> pq > >>> > >>>> + case DRM_PLANE_PIXEL_SOURCE_FB: > >>>> + default: > >>>> + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); > >>>> + } > >>>> + > >>>> + return state->fb != NULL; > >>>> +} > >>>> + > >>>> bool drm_any_plane_has_format(struct drm_device *dev, > >>>> u32 format, u64 modifier); > >>>> > >>> > > > > -- > > With best wishes > > Dmitry > >
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index cc0e93d19e15..cdc6cfedd433 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -668,14 +668,14 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, const struct drm_framebuffer *fb = new_plane_state->fb; int ret; - /* either *both* CRTC and FB must be set, or neither */ - if (crtc && !fb) { - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n", + /* either *both* CRTC and pixel source must be set, or neither */ + if (crtc && !drm_plane_has_visible_data(new_plane_state)) { + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no visible data\n", plane->base.id, plane->name); return -EINVAL; - } else if (fb && !crtc) { - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] FB set but no CRTC\n", - plane->base.id, plane->name); + } else if (drm_plane_has_visible_data(new_plane_state) && !crtc) { + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Source %d has visible data but no CRTC\n", + plane->base.id, plane->name, new_plane_state->pixel_source); return -EINVAL; } @@ -706,9 +706,11 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, } - ret = drm_atomic_plane_check_fb(new_plane_state); - if (ret) - return ret; + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + ret = drm_atomic_plane_check_fb(new_plane_state); + if (ret) + return ret; + } if (plane_switching_crtc(old_plane_state, new_plane_state)) { drm_dbg_atomic(plane->dev, diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 41b8066f61ff..a176064ee27e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, *src = drm_plane_state_src(plane_state); *dst = drm_plane_state_dest(plane_state); - if (!fb) { + if (!drm_plane_has_visible_data(plane_state)) { plane_state->visible = false; return 0; } @@ -881,25 +881,29 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, return -EINVAL; } - drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); + if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); - /* Check scaling */ - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); - if (hscale < 0 || vscale < 0) { - drm_dbg_kms(plane_state->plane->dev, - "Invalid scaling of plane\n"); - drm_rect_debug_print("src: ", &plane_state->src, true); - drm_rect_debug_print("dst: ", &plane_state->dst, false); - return -ERANGE; - } + /* Check scaling */ + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); - if (crtc_state->enable) - drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); + if (hscale < 0 || vscale < 0) { + drm_dbg_kms(plane_state->plane->dev, + "Invalid scaling of plane\n"); + drm_rect_debug_print("src: ", &plane_state->src, true); + drm_rect_debug_print("dst: ", &plane_state->dst, false); + return -ERANGE; + } - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); + if (crtc_state->enable) + drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); + } else if (drm_plane_solid_fill_enabled(plane_state)) { + plane_state->visible = true; + } if (!plane_state->visible) /* diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 536a0b0091c3..6d97f38ac1f6 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -256,8 +256,8 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state, * Anything else should be considered a bug in the atomic core, so we * gently warn about it. */ - WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) || - (new_plane_state->crtc != NULL && new_plane_state->fb == NULL)); + WARN_ON((new_plane_state->crtc == NULL && drm_plane_has_visible_data(new_plane_state)) || + (new_plane_state->crtc != NULL && !drm_plane_has_visible_data(new_plane_state))); return old_plane_state->crtc && !new_plane_state->crtc; } diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index a58f84b6bd5e..4c5b7bcdb25c 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -992,6 +992,35 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev, #define drm_for_each_plane(plane, dev) \ list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) +/** + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane + * @state: plane state + * + * Returns: + * Whether the plane has been assigned a solid_fill_blob + */ +static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state *state) +{ + if (!state) + return false; + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL && state->solid_fill_blob; +} + +static inline bool drm_plane_has_visible_data(const struct drm_plane_state *state) +{ + switch (state->pixel_source) { + case DRM_PLANE_PIXEL_SOURCE_NONE: + return false; + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL: + return state->solid_fill_blob != NULL; + case DRM_PLANE_PIXEL_SOURCE_FB: + default: + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB); + } + + return state->fb != NULL; +} + bool drm_any_plane_has_format(struct drm_device *dev, u32 format, u64 modifier);
Loosen the requirements for atomic and legacy commit so that, in cases where pixel_source != FB, the commit can still go through. This includes adding framebuffer NULL checks in other areas to account for FB being NULL when non-FB pixel sources are enabled. To disable a plane, the pixel_source must be NONE or the FB must be NULL if pixel_source == FB. Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/drm_atomic.c | 20 +++++++++++--------- drivers/gpu/drm/drm_atomic_helper.c | 36 ++++++++++++++++++++---------------- include/drm/drm_atomic_helper.h | 4 ++-- include/drm/drm_plane.h | 29 +++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 27 deletions(-)