Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 11 Jul 2023 15:07:19 -0700 | From | Jessica Zhang <> | Subject | Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property |
| |
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 <dmitry.baryshkov@linaro.org> 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 <quic_jesszhan@quicinc.com> >>>>> --- >>>>> 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 non-atomic paths, shouldn't that be enough to default pixel_source to FB for old userspace?
Thanks,
Jessica Zhang
> > >> >>>> >>>>> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane >>>>> *plane, >>>>> } else if (property == plane->solid_fill_property) { >>>>> *val =state->solid_fill_blob ? >>>>> state->solid_fill_blob->base.id : 0; >>>>> + } else if (property == plane->pixel_source_property) { >>>>> + *val = state->pixel_source; >>>>> } else if (property == plane->alpha_property) { >>>>> *val =state->alpha; >>>>> } else if (property == plane->blend_mode_property) { >>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c >>>>> index 38c3c5d6453a..8c100a957ee2 100644 >>>>> --- a/drivers/gpu/drm/drm_blend.c >>>>> +++ b/drivers/gpu/drm/drm_blend.c >>>>> @@ -189,6 +189,18 @@ >>>>> * solid_fill is set up with >>>>> drm_plane_create_solid_fill_property(). It >>>>> * contains pixel data that drivers can use to fill a plane. >>>>> * >>>>> + * pixel_source: >>>>> + * pixel_source is set up with >>>>> drm_plane_create_pixel_source_property(). >>>>> + * It is used to toggle the source of pixel data for the plane. >>> >>> Other sources than the selected one are ignored? >> >> Yep, the plane will only display the data from the set pixel_source. >> >> So if pixel_source == FB and solid_fill_blob is non-NULL, >> solid_fill_blob will be ignored and the plane will display the FB that >> is set. > > correct. > >> >> Will add a note about this in the comment docs. >> >>> >>>>> + * >>>>> + * Possible values: >>> >>> Wouldn't hurt to explicitly mention here that this is an enum. >> >> Acked. >> >>> >>>>> + * >>>>> + * "FB": >>>>> + * Framebuffer source >>>>> + * >>>>> + * "COLOR": >>>>> + * solid_fill source >>> >>> I think these two should be more explicit. Framebuffer source is the >>> usual source from the property "FB_ID". Solid fill source comes from >>> the property "solid_fill". >> >> Acked. >> >>> >>> Why "COLOR" and not, say, "SOLID_FILL"? >> >> Ah, that would make more sense :) >> >> I'll change this to "SOLID_FILL". >> >>> >>>>> + * >>>>> * Note that all the property extensions described here apply >>>>> either to the >>>>> * plane or the CRTC (e.g. for the background color, which >>>>> currently is not >>>>> * exposed and assumed to be black). >>>>> @@ -648,3 +660,72 @@ int >>>>> drm_plane_create_solid_fill_property(struct drm_plane *plane) >>>>> return 0; >>>>> } >>>>> EXPORT_SYMBOL(drm_plane_create_solid_fill_property); >>>>> + >>>>> +/** >>>>> + * drm_plane_create_pixel_source_property - create a new pixel >>>>> source property >>>>> + * @plane: drm plane >>>>> + * @supported_sources: bitmask of supported pixel_sources for the >>>>> driver (NOT >>>>> + * including DRM_PLANE_PIXEL_SOURCE_FB, as it >>>>> will be supported >>>>> + * by default). >>>> >>>> I'd say this is too strong. I'd suggest either renaming this to >>>> extra_sources (mentioning that FB is enabled for all the planes) or >>>> allowing any source bitmask (mentioning that FB should be enabled by >>>> the >>>> caller, unless there is a good reason not to do so). >>> >>> Right. I don't see any problem with having planes of type OVERLAY that >>> support only solid_fill and no FB. Planes of type PRIMARY and CURSOR I >>> would expect to always support at least FB. >>> >>> Atomic userspace is prepared to have an OVERLAY plane fail for any >>> arbitrary reason. Legacy userspace probably should not ever see a plane >>> that does not support FB. >> >> Got it... If we allow the possibility of FB sources not being >> supported, then should the default pixel_source per plane be decided >> by the driver too? >> >> I'd forced FB support so that I could set pixel_source to FB in >> __drm_atomic_helper_plane_state_reset(). If we allow more flexibility >> in the default pixel_source value, I guess we can also store a >> default_pixel_source value in the plane_state. > > I'd say, the FB is a sane default. It the driver has other needs, it can > override the value in drm_plane_funcs::reset(). > >> > > [skipped the rest] > > -- > With best wishes > Dmitry >
| ![\](/images/icornerr.gif) |