Messages in this thread | | | Date | Mon, 17 Jan 2022 11:45:27 +0900 | Subject | Re: [RFC PATH 1/3] drm: add support modifiers for drivers whose planes only support linear layout | From | Esaki Tomohito <> |
| |
Thank you for your reviews.
On 2022/01/15 1:50, Daniel Vetter wrote: > On Wed, Dec 22, 2021 at 02:27:25PM +0900, Tomohito Esaki wrote: >> The LINEAR modifier is advertised as default if a driver doesn't specify >> modifiers. However, there are legacy drivers such as radeon that do not >> support modifiers but infer the actual layout of the underlying buffer. >> Therefore, a new flag not_support_fb_modifires is introduced for these >> legacy drivers. Allow_fb_modifiers will be replaced with this new flag. >> >> Signed-off-by: Tomohito Esaki <etom@igel.co.jp> >> --- >> drivers/gpu/drm/drm_plane.c | 34 ++++++++++++++++++++++++++-------- >> include/drm/drm_mode_config.h | 10 ++++++++++ >> include/drm/drm_plane.h | 3 +++ >> 3 files changed, 39 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c >> index 82afb854141b..75308ee240c0 100644 >> --- a/drivers/gpu/drm/drm_plane.c >> +++ b/drivers/gpu/drm/drm_plane.c >> @@ -161,6 +161,16 @@ modifiers_ptr(struct drm_format_modifier_blob *blob) >> return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset); >> } >> >> +static bool check_format_modifier(struct drm_plane *plane, uint32_t format, >> + uint64_t modifier) >> +{ >> + if (plane->funcs->format_mod_supported) >> + return plane->funcs->format_mod_supported(plane, format, >> + modifier); >> + >> + return modifier == DRM_FORMAT_MOD_LINEAR; >> +} >> + >> static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane) >> { >> const struct drm_mode_config *config = &dev->mode_config; >> @@ -203,16 +213,15 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane >> memcpy(formats_ptr(blob_data), plane->format_types, formats_size); >> >> /* If we can't determine support, just bail */ >> - if (!plane->funcs->format_mod_supported) >> + if (config->fb_modifiers_not_supported) >> goto done; >> >> mod = modifiers_ptr(blob_data); >> for (i = 0; i < plane->modifier_count; i++) { >> for (j = 0; j < plane->format_count; j++) { >> - if (plane->funcs->format_mod_supported(plane, >> - plane->format_types[j], >> - plane->modifiers[i])) { >> - >> + if (check_format_modifier(plane, >> + plane->format_types[j], >> + plane->modifiers[i])) { >> mod->formats |= 1ULL << j; >> } >> } >> @@ -242,6 +251,10 @@ static int __drm_universal_plane_init(struct drm_device *dev, >> const char *name, va_list ap) >> { >> struct drm_mode_config *config = &dev->mode_config; >> + const uint64_t default_modifiers[] = { >> + DRM_FORMAT_MOD_LINEAR, >> + DRM_FORMAT_MOD_INVALID >> + }; >> unsigned int format_modifier_count = 0; >> int ret; >> >> @@ -282,6 +295,11 @@ static int __drm_universal_plane_init(struct drm_device *dev, >> >> while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) >> format_modifier_count++; >> + } else { >> + if (!dev->mode_config.fb_modifiers_not_supported) { >> + format_modifiers = default_modifiers; >> + format_modifier_count = 1; >> + } >> } >> >> /* autoset the cap and check for consistency across all planes */ >> @@ -346,7 +364,7 @@ static int __drm_universal_plane_init(struct drm_device *dev, >> drm_object_attach_property(&plane->base, config->prop_src_h, 0); >> } >> >> - if (config->allow_fb_modifiers) >> + if (format_modifier_count) >> create_in_format_blob(dev, plane); >> >> return 0; >> @@ -373,8 +391,8 @@ static int __drm_universal_plane_init(struct drm_device *dev, >> * drm_universal_plane_init() to let the DRM managed resource infrastructure >> * take care of cleanup and deallocation. >> * >> - * Drivers supporting modifiers must set @format_modifiers on all their planes, >> - * even those that only support DRM_FORMAT_MOD_LINEAR. >> + * For drivers supporting modifiers, all planes will advertise >> + * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set. >> * >> * Returns: >> * Zero on success, error code on failure. >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >> index 48b7de80daf5..c56f298c55bd 100644 >> --- a/include/drm/drm_mode_config.h >> +++ b/include/drm/drm_mode_config.h >> @@ -920,6 +920,16 @@ struct drm_mode_config { >> */ >> bool allow_fb_modifiers; >> >> + /** >> + * @fb_modifiers_not_supported: >> + * >> + * This flag is for legacy drivers such as radeon that do not support > > Maybe don't put specific driver names into kerneldoc (in commit message to > motivate your changes it's fine). It's unlikely radeon ever changes on > this, but also no one will update this in the docs if we ever do that. > > Perhaps also add that new driver should never set this, just to hammer it > home that modifiers really should work everywhere.
I agree with you. I'll modify this docs.
Thanks, Tomohito Esaki
> > Otherwise I think this series is the right thing to do. > -Daniel > >> + * modifiers but infer the actual layout of the underlying buffer. >> + * Generally, each drivers must support modifiers, this flag should not >> + * be set. >> + */ >> + bool fb_modifiers_not_supported; >> + >> /** >> * @normalize_zpos: >> * >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index 0c1102dc4d88..cad641b1f797 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -803,6 +803,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, >> * >> * The @drm_plane_funcs.destroy hook must be NULL. >> * >> + * For drivers supporting modifiers, all planes will advertise >> + * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set. >> + * >> * Returns: >> * Pointer to new plane, or ERR_PTR on failure. >> */ >> -- >> 2.17.1 >> >
| |