On Tue, 11 Jul 2023 15:42:28 -0700
Abhinav Kumar wrote:
> On 7/11/2023 3:19 PM, Dmitry Baryshkov wrote:
> > On 12/07/2023 01:07, Jessica Zhang wrote:
> >>
> >>
> >> On 7/10/2023 1:11 PM, Dmitry Baryshkov wrote:
> >>> On 10/07/2023 22:51, Jessica Zhang wrote:
>
>
> On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
> > On Fri, 30 Jun 2023 03:42:28 +0300
> > Dmitry Baryshkov wrote:
> >
> >> On 30/06/2023 03:25, Jessica Zhang wrote:
> >>> Add support for pixel_source property to drm_plane and related
> >>> documentation.
> >>>
> >>> This enum property will allow user to specify a pixel source for the
> >>> plane. Possible pixel sources will be defined in the
> >>> drm_plane_pixel_source enum.
> >>>
> >>> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> >>> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
> >>
> >> I think, this should come before the solid fill property addition.
> >> First
> >> you tell that there is a possibility to define other pixel
> >> sources, then
> >> additional sources are defined.
> >
> > Hi,
> >
> > that would be logical indeed.
>
> Hi Dmitry and Pekka,
>
> Sorry for the delay in response, was out of office last week.
>
> Acked.
>
> >
> >>>
> >>> Signed-off-by: Jessica Zhang
> >>> ---
> >>> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> >>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> >>> drivers/gpu/drm/drm_blend.c | 81
> >>> +++
> >>> include/drm/drm_blend.h | 2 +
> >>> include/drm/drm_plane.h | 21
> >>> 5 files changed, 109 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> >>> b/drivers/gpu/drm/drm_atomic_state_helper.c
> >>> index fe14be2bd2b2..86fb876efbe6 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >>> @@ -252,6 +252,7 @@ void
> >>> __drm_atomic_helper_plane_state_reset(struct drm_plane_state
> >>> *plane_state,
> >>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> >>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >>> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >>> if (plane_state->solid_fill_blob) {
> >>> drm_property_blob_put(plane_state->solid_fill_blob);
> >>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >>> b/drivers/gpu/drm/drm_atomic_uapi.c
> >>> index a28b4ee79444..6e59c21af66b 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >>> @@ -596,6 +596,8 @@ static int
> >>> drm_atomic_plane_set_property(struct drm_plane *plane,
> >>> drm_property_blob_put(solid_fill);
> >>> return ret;
> >>> + } else if (property == plane->pixel_source_property) {
> >>> + state->pixel_source = val;
> >>> } else if (property == plane->alpha_property) {
> >>> state->alpha = val;
> >>> } else if (property == plane->blend_mode_property) {
> >>
> >> I think, it was pointed out in the discussion that
> >> drm_mode_setplane()
> >> (a pre-atomic IOCTL to turn the plane on and off) should also reset
> >> pixel_source to FB.
>
> I don't remember drm_mode_setplane() being mentioned in the
> pixel_source discussion... can you share where it was mentioned?
> >>>
> >>> https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/
> >>>
> >>> Let me quote it here:
> >>> "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."
> >>>
> >>>
>
> I'd prefer to avoid having driver change the pixel_source directly
> as it could cause some unexpected side effects. In general, I would
> like userspace to assign the value of pixel_source without driver
> doing anything "under the hood".
> >>>
> >>> s/driver/drm core/
> >>>
> >>> We have to remain compatible with old userspace, especially with the
> >>> non-atomic one. If the userspace calls
> >>> ioctl(DRM_IOCTL_MODE_SETPLANE), we have to display the specified FB,
> >>> no matter what was the value of PIXEL_SOURCE before this ioctl.
> >>
> >>
> >> Got it, thanks the clarification -- I see your point.
> >>
> >> I'm already setting plane_state->pixel_source to FB in
> >> __drm_atomic_helper_plane_reset() and it seems to me that all drivers
> >> are calling that within their respective plane_funcs->reset().
> >>
> >> Since (as far as I know) reset() is being called for both the atomic
> >> and
