| Message ID | 20220114101753.24996-3-etom@igel.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support modifiers for drivers whose planes only support linear layout | expand |
On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote: > The LINEAR modifier is advertised as default if a driver doesn't specify > modifiers. ... > + const uint64_t default_modifiers[] = { > + DRM_FORMAT_MOD_LINEAR, > + DRM_FORMAT_MOD_INVALID + Comma? > + }; Why not enum?
On Friday, January 14th, 2022 at 15:16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> Why not enum?
There is no enum for DRM format modifiers.
On Fri, Jan 14, 2022 at 03:07:21PM +0000, Simon Ser wrote: > On Friday, January 14th, 2022 at 15:16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Why not enum? > > There is no enum for DRM format modifiers. I'm not sure how this prevents to use enum in the code instead of const u64. Any specific reason for that?
On Friday, January 14th, 2022 at 16:17, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Jan 14, 2022 at 03:07:21PM +0000, Simon Ser wrote: > > On Friday, January 14th, 2022 at 15:16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > Why not enum? > > > > There is no enum for DRM format modifiers. > > I'm not sure how this prevents to use enum in the code instead of const u64. > Any specific reason for that? I'm not sure how one would use an enum as the array item type, when there is no defined enum type. Moreover, all the rest of DRM uses uint64 for modifiers.
On Fri, Jan 14, 2022 at 03:42:54PM +0000, Simon Ser wrote: > On Friday, January 14th, 2022 at 16:17, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Jan 14, 2022 at 03:07:21PM +0000, Simon Ser wrote: > > > On Friday, January 14th, 2022 at 15:16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > Why not enum? > > > > > > There is no enum for DRM format modifiers. > > > > I'm not sure how this prevents to use enum in the code instead of const u64. > > Any specific reason for that? > > I'm not sure how one would use an enum as the array item type, when there is no > defined enum type. > > Moreover, all the rest of DRM uses uint64 for modifiers. Ah, I see now. This is an array that filled by predefined values. Thanks for explanation.
Thank you for your reviews. On 2022/01/14 23:16, Andy Shevchenko wrote: > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote: >> The LINEAR modifier is advertised as default if a driver doesn't specify >> modifiers. > > ... > >> + const uint64_t default_modifiers[] = { >> + DRM_FORMAT_MOD_LINEAR, >> + DRM_FORMAT_MOD_INVALID > > + Comma? There is no mention in the coding style about adding/removing a comma to the last element of an array. Is there a policy in drm driver? I think the advantage of adding a comma to the last element of an array is that diff is only one line when an element is added to the end. However since INVALID is always the last element in the modifiers array, I think it can be either in this case. If there is a policy, I will match it. Thanks, Tomohito Esaki
On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote: > On 2022/01/14 23:16, Andy Shevchenko wrote: > > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote: > > > The LINEAR modifier is advertised as default if a driver doesn't specify > > > modifiers. > > > > ... > > > > > + const uint64_t default_modifiers[] = { > > > + DRM_FORMAT_MOD_LINEAR, > > > + DRM_FORMAT_MOD_INVALID > > > > + Comma? > > There is no mention in the coding style about adding/removing a comma to the > last element of an array. Is there a policy in drm driver? > > I think the advantage of adding a comma to the last element of an array is > that diff is only one line when an element is added to the end. > However since INVALID is always the last element in the modifiers array, I > think it can be either in this case. > If there is a policy, I will match it. Indeed, but there is a common sense. The idea behind (multi-line) definitions that when next time somebody will add an element in the array, there are will be: a) no additional churn (like in case of this patch, if the item will be added at the bottom; b) an element that may not be added behind the terminator, which will look weird. That said, the question is if the element is terminator one or not, if not, comma is better than no comma and vise versa.
On 2022/01/18 18:53, Andy Shevchenko wrote: > On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote: >> On 2022/01/14 23:16, Andy Shevchenko wrote: >>> On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote: >>>> The LINEAR modifier is advertised as default if a driver doesn't specify >>>> modifiers. >>> >>> ... >>> >>>> + const uint64_t default_modifiers[] = { >>>> + DRM_FORMAT_MOD_LINEAR, >>>> + DRM_FORMAT_MOD_INVALID >>> >>> + Comma? >> >> There is no mention in the coding style about adding/removing a comma to the >> last element of an array. Is there a policy in drm driver? >> >> I think the advantage of adding a comma to the last element of an array is >> that diff is only one line when an element is added to the end. >> However since INVALID is always the last element in the modifiers array, I >> think it can be either in this case. >> If there is a policy, I will match it. > > Indeed, but there is a common sense. The idea behind (multi-line) definitions > that when next time somebody will add an element in the array, there are will > be: > > a) no additional churn (like in case of this patch, if the item will be added > at the bottom; > > b) an element that may not be added behind the terminator, which will look > weird. > > That said, the question is if the element is terminator one or not, if not, > comma is better than no comma and vise versa. > Ah I see. In this case, DRM_FORMAT_MOD_INVALID is terminator, so it should not have a comma. Thanks Tomohito Esaki
On Wed, Jan 19, 2022 at 11:35:22AM +0900, Esaki Tomohito wrote: > On 2022/01/18 18:53, Andy Shevchenko wrote: > > On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote: > > > On 2022/01/14 23:16, Andy Shevchenko wrote: > > > > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote: > > > > > The LINEAR modifier is advertised as default if a driver doesn't specify > > > > > modifiers. > > > > > > > > ... > > > > > > > > > + const uint64_t default_modifiers[] = { > > > > > + DRM_FORMAT_MOD_LINEAR, > > > > > + DRM_FORMAT_MOD_INVALID > > > > > > > > + Comma? > > > > > > There is no mention in the coding style about adding/removing a comma to the > > > last element of an array. Is there a policy in drm driver? > > > > > > I think the advantage of adding a comma to the last element of an array is > > > that diff is only one line when an element is added to the end. > > > However since INVALID is always the last element in the modifiers array, I > > > think it can be either in this case. > > > If there is a policy, I will match it. > > > > Indeed, but there is a common sense. The idea behind (multi-line) definitions > > that when next time somebody will add an element in the array, there are will > > be: > > > > a) no additional churn (like in case of this patch, if the item will be added > > at the bottom; > > > > b) an element that may not be added behind the terminator, which will look > > weird. > > > > That said, the question is if the element is terminator one or not, if not, > > comma is better than no comma and vise versa. > > Ah I see. In this case, DRM_FORMAT_MOD_INVALID is terminator, so it > should not have a comma. Thanks for pointing this out. In this case we are good and any new item, AFAIU, must be added before _INVALID one.
Hello Esaki-san, Thank you for the patch. On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote: > The LINEAR modifier is advertised as default if a driver doesn't specify > modifiers. > > Signed-off-by: Tomohito Esaki <etom@igel.co.jp> > --- > drivers/gpu/drm/drm_plane.c | 15 ++++++++++++--- > include/drm/drm_plane.h | 3 +++ > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index deeec60a3315..5aa7e241971e 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -237,6 +237,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[] = { This can be static. > + DRM_FORMAT_MOD_LINEAR, > + DRM_FORMAT_MOD_INVALID No need for a terminator here. The terminator is needed in the format_modifiers array passed to this function, to count the number of elements, but the default_modifiers is used with a hard-coded count. > + }; > unsigned int format_modifier_count = 0; > int ret; > > @@ -277,6 +281,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; You can use format_modifier_count = ARRAY_SIZE(default_modifiers); > + } > } > > /* autoset the cap and check for consistency across all planes */ > @@ -341,7 +350,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; > @@ -368,8 +377,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. I'd write this a bit differently: * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set * @format_modifiers to NULL. The plane will advertise the linear modifier. As fb_modifiers_not_supported is only for a few legacy drivers, I wouldn't even mention it here, it would only obfuscate things. > * > * Returns: > * Zero on success, error code on failure. > 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. Same here. With these small issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * > * Returns: > * Pointer to new plane, or ERR_PTR on failure. > */
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index deeec60a3315..5aa7e241971e 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -237,6 +237,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; @@ -277,6 +281,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 */ @@ -341,7 +350,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; @@ -368,8 +377,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_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. */
The LINEAR modifier is advertised as default if a driver doesn't specify modifiers. Signed-off-by: Tomohito Esaki <etom@igel.co.jp> --- drivers/gpu/drm/drm_plane.c | 15 ++++++++++++--- include/drm/drm_plane.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)