| Message ID | 20190509054518.10781-1-etom@igel.co.jp (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: add modifiers support | expand |
Hi Tomohito-san, On Thu, May 09, 2019 at 02:45:18PM +0900, Tomohito Esaki wrote: > Add support for the linear modifier. Since the rcar-du device supports > only linear modifier, this driver doesn't support other modifiers. What's the purpose of this, as it adds no new functionality to the driver ? Why is this change needed ? > Signed-off-by: Tomohito Esaki <etom@igel.co.jp> > --- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 +++++++++++ > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 ++++++++++++++- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 15 ++++++++++++++- > 3 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > index 3b7d50a8fb9b..9c5e15a5ab1c 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -214,6 +214,16 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, > return ERR_PTR(-EINVAL); > } > > + /* > + * Support only LINEAR modifier. > + */ > + if ((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) && > + mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR) { > + dev_dbg(dev->dev, "unsupported fb modifier 0x%llx\n", > + mode_cmd->modifier[0]); > + return ERR_PTR(-EINVAL); > + } > + > if (rcdu->info->gen < 3) { > /* > * On Gen2 the DU limits the pitch to 4095 pixels and requires > @@ -529,6 +539,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) > dev->mode_config.min_width = 0; > dev->mode_config.min_height = 0; > dev->mode_config.normalize_zpos = true; > + dev->mode_config.allow_fb_modifiers = true; > dev->mode_config.funcs = &rcar_du_mode_config_funcs; > dev->mode_config.helper_private = &rcar_du_mode_config_helper; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > index c6430027169f..32135ad387fa 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > @@ -726,6 +726,13 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, > return 0; > } > > +static bool rcar_du_plane_format_mod_supported(struct drm_plane *plane, > + uint32_t format, > + uint64_t modifier) > +{ > + return modifier == DRM_FORMAT_MOD_LINEAR; > +} > + > static const struct drm_plane_funcs rcar_du_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > @@ -735,6 +742,7 @@ static const struct drm_plane_funcs rcar_du_plane_funcs = { > .atomic_destroy_state = rcar_du_plane_atomic_destroy_state, > .atomic_set_property = rcar_du_plane_atomic_set_property, > .atomic_get_property = rcar_du_plane_atomic_get_property, > + .format_mod_supported = rcar_du_plane_format_mod_supported, > }; > > static const uint32_t formats[] = { > @@ -750,6 +758,11 @@ static const uint32_t formats[] = { > DRM_FORMAT_NV16, > }; > > +static const uint64_t modifiers[] = { > + DRM_FORMAT_MOD_LINEAR, > + DRM_FORMAT_MOD_INVALID, > +}; > + > int rcar_du_planes_init(struct rcar_du_group *rgrp) > { > struct rcar_du_device *rcdu = rgrp->dev; > @@ -776,7 +789,7 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) > ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs, > &rcar_du_plane_funcs, formats, > ARRAY_SIZE(formats), > - NULL, type, NULL); > + modifiers, type, NULL); > if (ret < 0) > return ret; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > index 0878accbd134..9d1382b02717 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > @@ -139,6 +139,11 @@ static const u32 formats_kms[] = { > DRM_FORMAT_YVU444, > }; > > +static const uint64_t modifiers_kms[] = { > + DRM_FORMAT_MOD_LINEAR, > + DRM_FORMAT_MOD_INVALID, > +}; > + > static const u32 formats_v4l2[] = { > V4L2_PIX_FMT_RGB332, > V4L2_PIX_FMT_ARGB444, > @@ -344,6 +349,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) > state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; > } > > +static bool rcar_du_vsp_plane_format_mod_supported(struct drm_plane *plane, > + uint32_t format, > + uint64_t modifier) > +{ > + return modifier == DRM_FORMAT_MOD_LINEAR; > +} > + > static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > @@ -351,6 +363,7 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > .destroy = drm_plane_cleanup, > .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, > .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, > + .format_mod_supported = rcar_du_vsp_plane_format_mod_supported, > }; > > int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > @@ -397,7 +410,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > &rcar_du_vsp_plane_funcs, > formats_kms, > ARRAY_SIZE(formats_kms), > - NULL, type, NULL); > + modifiers_kms, type, NULL); > if (ret < 0) > return ret; > > -- > 2.17.1 >
Hi Laurent-san > What's the purpose of this, as it adds no new functionality to the > driver ? Why is this change needed ? Weston compositor (v5.0.0 or later) uses the DRM API to get the supported modifiers and determines if the sprite plane can be used by comparing the modifiers with the client specified modifier. In currently driver, since the weston doesn't know supported modifiers, that cannot determine if the received dmabuf can be passed through to sprite plane. Since there are R-Car GPU which support linear modifier, the sprite plane cannot be used in a compositor similar to the weston if client specify linear modifier. Thanks. On 2019/05/09 16:14, Laurent Pinchart wrote: > Hi Tomohito-san, > > On Thu, May 09, 2019 at 02:45:18PM +0900, Tomohito Esaki wrote: >> Add support for the linear modifier. Since the rcar-du device supports >> only linear modifier, this driver doesn't support other modifiers. > > What's the purpose of this, as it adds no new functionality to the > driver ? Why is this change needed ? > >> Signed-off-by: Tomohito Esaki <etom@igel.co.jp> >> --- >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 +++++++++++ >> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 ++++++++++++++- >> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 15 ++++++++++++++- >> 3 files changed, 39 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c >> index 3b7d50a8fb9b..9c5e15a5ab1c 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c >> @@ -214,6 +214,16 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, >> return ERR_PTR(-EINVAL); >> } >> >> + /* >> + * Support only LINEAR modifier. >> + */ >> + if ((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) && >> + mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR) { >> + dev_dbg(dev->dev, "unsupported fb modifier 0x%llx\n", >> + mode_cmd->modifier[0]); >> + return ERR_PTR(-EINVAL); >> + } >> + >> if (rcdu->info->gen < 3) { >> /* >> * On Gen2 the DU limits the pitch to 4095 pixels and requires >> @@ -529,6 +539,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) >> dev->mode_config.min_width = 0; >> dev->mode_config.min_height = 0; >> dev->mode_config.normalize_zpos = true; >> + dev->mode_config.allow_fb_modifiers = true; >> dev->mode_config.funcs = &rcar_du_mode_config_funcs; >> dev->mode_config.helper_private = &rcar_du_mode_config_helper; >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c >> index c6430027169f..32135ad387fa 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c >> @@ -726,6 +726,13 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, >> return 0; >> } >> >> +static bool rcar_du_plane_format_mod_supported(struct drm_plane *plane, >> + uint32_t format, >> + uint64_t modifier) >> +{ >> + return modifier == DRM_FORMAT_MOD_LINEAR; >> +} >> + >> static const struct drm_plane_funcs rcar_du_plane_funcs = { >> .update_plane = drm_atomic_helper_update_plane, >> .disable_plane = drm_atomic_helper_disable_plane, >> @@ -735,6 +742,7 @@ static const struct drm_plane_funcs rcar_du_plane_funcs = { >> .atomic_destroy_state = rcar_du_plane_atomic_destroy_state, >> .atomic_set_property = rcar_du_plane_atomic_set_property, >> .atomic_get_property = rcar_du_plane_atomic_get_property, >> + .format_mod_supported = rcar_du_plane_format_mod_supported, >> }; >> >> static const uint32_t formats[] = { >> @@ -750,6 +758,11 @@ static const uint32_t formats[] = { >> DRM_FORMAT_NV16, >> }; >> >> +static const uint64_t modifiers[] = { >> + DRM_FORMAT_MOD_LINEAR, >> + DRM_FORMAT_MOD_INVALID, >> +}; >> + >> int rcar_du_planes_init(struct rcar_du_group *rgrp) >> { >> struct rcar_du_device *rcdu = rgrp->dev; >> @@ -776,7 +789,7 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) >> ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs, >> &rcar_du_plane_funcs, formats, >> ARRAY_SIZE(formats), >> - NULL, type, NULL); >> + modifiers, type, NULL); >> if (ret < 0) >> return ret; >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c >> index 0878accbd134..9d1382b02717 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c >> @@ -139,6 +139,11 @@ static const u32 formats_kms[] = { >> DRM_FORMAT_YVU444, >> }; >> >> +static const uint64_t modifiers_kms[] = { >> + DRM_FORMAT_MOD_LINEAR, >> + DRM_FORMAT_MOD_INVALID, >> +}; >> + >> static const u32 formats_v4l2[] = { >> V4L2_PIX_FMT_RGB332, >> V4L2_PIX_FMT_ARGB444, >> @@ -344,6 +349,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) >> state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; >> } >> >> +static bool rcar_du_vsp_plane_format_mod_supported(struct drm_plane *plane, >> + uint32_t format, >> + uint64_t modifier) >> +{ >> + return modifier == DRM_FORMAT_MOD_LINEAR; >> +} >> + >> static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { >> .update_plane = drm_atomic_helper_update_plane, >> .disable_plane = drm_atomic_helper_disable_plane, >> @@ -351,6 +363,7 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { >> .destroy = drm_plane_cleanup, >> .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, >> .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, >> + .format_mod_supported = rcar_du_vsp_plane_format_mod_supported, >> }; >> >> int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, >> @@ -397,7 +410,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, >> &rcar_du_vsp_plane_funcs, >> formats_kms, >> ARRAY_SIZE(formats_kms), >> - NULL, type, NULL); >> + modifiers_kms, type, NULL); >> if (ret < 0) >> return ret; >> >> -- >> 2.17.1 >> >
Hello Esaki-san, On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: > Hi Laurent-san > > > What's the purpose of this, as it adds no new functionality to the > > driver ? Why is this change needed ? > > Weston compositor (v5.0.0 or later) uses the DRM API to get the > supported modifiers and determines if the sprite plane can be used by > comparing the modifiers with the client specified modifier. > In currently driver, since the weston doesn't know supported modifiers, > that cannot determine if the received dmabuf can be passed through to > sprite plane. > Since there are R-Car GPU which support linear modifier, the sprite > plane cannot be used in a compositor similar to the weston if client > specify linear modifier. I don't think the right solution is to expose the linear modifier from all drivers that don't otherwise support modifiers. We should instead fix it either in Weston, and treat drivers that don't support the modifiers API as supporting the linear modifier only, or in the DRM/KMS core by reporting the linear modifier for drivers that don't explicitly support modifiers. > On 2019/05/09 16:14, Laurent Pinchart wrote: > > On Thu, May 09, 2019 at 02:45:18PM +0900, Tomohito Esaki wrote: > >> Add support for the linear modifier. Since the rcar-du device supports > >> only linear modifier, this driver doesn't support other modifiers. > > > > What's the purpose of this, as it adds no new functionality to the > > driver ? Why is this change needed ? > > > >> Signed-off-by: Tomohito Esaki <etom@igel.co.jp> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 +++++++++++ > >> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 ++++++++++++++- > >> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 15 ++++++++++++++- > >> 3 files changed, 39 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >> index 3b7d50a8fb9b..9c5e15a5ab1c 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >> @@ -214,6 +214,16 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, > >> return ERR_PTR(-EINVAL); > >> } > >> > >> + /* > >> + * Support only LINEAR modifier. > >> + */ > >> + if ((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) && > >> + mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR) { > >> + dev_dbg(dev->dev, "unsupported fb modifier 0x%llx\n", > >> + mode_cmd->modifier[0]); > >> + return ERR_PTR(-EINVAL); > >> + } > >> + > >> if (rcdu->info->gen < 3) { > >> /* > >> * On Gen2 the DU limits the pitch to 4095 pixels and requires > >> @@ -529,6 +539,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) > >> dev->mode_config.min_width = 0; > >> dev->mode_config.min_height = 0; > >> dev->mode_config.normalize_zpos = true; > >> + dev->mode_config.allow_fb_modifiers = true; > >> dev->mode_config.funcs = &rcar_du_mode_config_funcs; > >> dev->mode_config.helper_private = &rcar_du_mode_config_helper; > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > >> index c6430027169f..32135ad387fa 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > >> @@ -726,6 +726,13 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, > >> return 0; > >> } > >> > >> +static bool rcar_du_plane_format_mod_supported(struct drm_plane *plane, > >> + uint32_t format, > >> + uint64_t modifier) > >> +{ > >> + return modifier == DRM_FORMAT_MOD_LINEAR; > >> +} > >> + > >> static const struct drm_plane_funcs rcar_du_plane_funcs = { > >> .update_plane = drm_atomic_helper_update_plane, > >> .disable_plane = drm_atomic_helper_disable_plane, > >> @@ -735,6 +742,7 @@ static const struct drm_plane_funcs rcar_du_plane_funcs = { > >> .atomic_destroy_state = rcar_du_plane_atomic_destroy_state, > >> .atomic_set_property = rcar_du_plane_atomic_set_property, > >> .atomic_get_property = rcar_du_plane_atomic_get_property, > >> + .format_mod_supported = rcar_du_plane_format_mod_supported, > >> }; > >> > >> static const uint32_t formats[] = { > >> @@ -750,6 +758,11 @@ static const uint32_t formats[] = { > >> DRM_FORMAT_NV16, > >> }; > >> > >> +static const uint64_t modifiers[] = { > >> + DRM_FORMAT_MOD_LINEAR, > >> + DRM_FORMAT_MOD_INVALID, > >> +}; > >> + > >> int rcar_du_planes_init(struct rcar_du_group *rgrp) > >> { > >> struct rcar_du_device *rcdu = rgrp->dev; > >> @@ -776,7 +789,7 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) > >> ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs, > >> &rcar_du_plane_funcs, formats, > >> ARRAY_SIZE(formats), > >> - NULL, type, NULL); > >> + modifiers, type, NULL); > >> if (ret < 0) > >> return ret; > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > >> index 0878accbd134..9d1382b02717 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > >> @@ -139,6 +139,11 @@ static const u32 formats_kms[] = { > >> DRM_FORMAT_YVU444, > >> }; > >> > >> +static const uint64_t modifiers_kms[] = { > >> + DRM_FORMAT_MOD_LINEAR, > >> + DRM_FORMAT_MOD_INVALID, > >> +}; > >> + > >> static const u32 formats_v4l2[] = { > >> V4L2_PIX_FMT_RGB332, > >> V4L2_PIX_FMT_ARGB444, > >> @@ -344,6 +349,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) > >> state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; > >> } > >> > >> +static bool rcar_du_vsp_plane_format_mod_supported(struct drm_plane *plane, > >> + uint32_t format, > >> + uint64_t modifier) > >> +{ > >> + return modifier == DRM_FORMAT_MOD_LINEAR; > >> +} > >> + > >> static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > >> .update_plane = drm_atomic_helper_update_plane, > >> .disable_plane = drm_atomic_helper_disable_plane, > >> @@ -351,6 +363,7 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > >> .destroy = drm_plane_cleanup, > >> .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, > >> .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, > >> + .format_mod_supported = rcar_du_vsp_plane_format_mod_supported, > >> }; > >> > >> int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > >> @@ -397,7 +410,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > >> &rcar_du_vsp_plane_funcs, > >> formats_kms, > >> ARRAY_SIZE(formats_kms), > >> - NULL, type, NULL); > >> + modifiers_kms, type, NULL); > >> if (ret < 0) > >> return ret; > >>
Hello Laurent-san, On 2019/05/12 3:10, Laurent Pinchart wrote: > Hello Esaki-san, > > On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: >> Hi Laurent-san >> >>> What's the purpose of this, as it adds no new functionality to the >>> driver ? Why is this change needed ? >> >> Weston compositor (v5.0.0 or later) uses the DRM API to get the >> supported modifiers and determines if the sprite plane can be used by >> comparing the modifiers with the client specified modifier. >> In currently driver, since the weston doesn't know supported modifiers, >> that cannot determine if the received dmabuf can be passed through to >> sprite plane. >> Since there are R-Car GPU which support linear modifier, the sprite >> plane cannot be used in a compositor similar to the weston if client >> specify linear modifier. > > I don't think the right solution is to expose the linear modifier from > all drivers that don't otherwise support modifiers. We should instead > fix it either in Weston, and treat drivers that don't support the > modifiers API as supporting the linear modifier only, or in the DRM/KMS > core by reporting the linear modifier for drivers that don't explicitly > support modifiers. I would like to ask for an opinion to wayland community. I will add you to Cc in that mail. Thanks! >> On 2019/05/09 16:14, Laurent Pinchart wrote: >>> On Thu, May 09, 2019 at 02:45:18PM +0900, Tomohito Esaki wrote: >>>> Add support for the linear modifier. Since the rcar-du device supports >>>> only linear modifier, this driver doesn't support other modifiers. >>> >>> What's the purpose of this, as it adds no new functionality to the >>> driver ? Why is this change needed ? >>> >>>> Signed-off-by: Tomohito Esaki <etom@igel.co.jp> >>>> --- >>>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 +++++++++++ >>>> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 ++++++++++++++- >>>> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 15 ++++++++++++++- >>>> 3 files changed, 39 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c >>>> index 3b7d50a8fb9b..9c5e15a5ab1c 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c >>>> @@ -214,6 +214,16 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, >>>> return ERR_PTR(-EINVAL); >>>> } >>>> >>>> + /* >>>> + * Support only LINEAR modifier. >>>> + */ >>>> + if ((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) && >>>> + mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR) { >>>> + dev_dbg(dev->dev, "unsupported fb modifier 0x%llx\n", >>>> + mode_cmd->modifier[0]); >>>> + return ERR_PTR(-EINVAL); >>>> + } >>>> + >>>> if (rcdu->info->gen < 3) { >>>> /* >>>> * On Gen2 the DU limits the pitch to 4095 pixels and requires >>>> @@ -529,6 +539,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) >>>> dev->mode_config.min_width = 0; >>>> dev->mode_config.min_height = 0; >>>> dev->mode_config.normalize_zpos = true; >>>> + dev->mode_config.allow_fb_modifiers = true; >>>> dev->mode_config.funcs = &rcar_du_mode_config_funcs; >>>> dev->mode_config.helper_private = &rcar_du_mode_config_helper; >>>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c >>>> index c6430027169f..32135ad387fa 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c >>>> @@ -726,6 +726,13 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, >>>> return 0; >>>> } >>>> >>>> +static bool rcar_du_plane_format_mod_supported(struct drm_plane *plane, >>>> + uint32_t format, >>>> + uint64_t modifier) >>>> +{ >>>> + return modifier == DRM_FORMAT_MOD_LINEAR; >>>> +} >>>> + >>>> static const struct drm_plane_funcs rcar_du_plane_funcs = { >>>> .update_plane = drm_atomic_helper_update_plane, >>>> .disable_plane = drm_atomic_helper_disable_plane, >>>> @@ -735,6 +742,7 @@ static const struct drm_plane_funcs rcar_du_plane_funcs = { >>>> .atomic_destroy_state = rcar_du_plane_atomic_destroy_state, >>>> .atomic_set_property = rcar_du_plane_atomic_set_property, >>>> .atomic_get_property = rcar_du_plane_atomic_get_property, >>>> + .format_mod_supported = rcar_du_plane_format_mod_supported, >>>> }; >>>> >>>> static const uint32_t formats[] = { >>>> @@ -750,6 +758,11 @@ static const uint32_t formats[] = { >>>> DRM_FORMAT_NV16, >>>> }; >>>> >>>> +static const uint64_t modifiers[] = { >>>> + DRM_FORMAT_MOD_LINEAR, >>>> + DRM_FORMAT_MOD_INVALID, >>>> +}; >>>> + >>>> int rcar_du_planes_init(struct rcar_du_group *rgrp) >>>> { >>>> struct rcar_du_device *rcdu = rgrp->dev; >>>> @@ -776,7 +789,7 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) >>>> ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs, >>>> &rcar_du_plane_funcs, formats, >>>> ARRAY_SIZE(formats), >>>> - NULL, type, NULL); >>>> + modifiers, type, NULL); >>>> if (ret < 0) >>>> return ret; >>>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c >>>> index 0878accbd134..9d1382b02717 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c >>>> @@ -139,6 +139,11 @@ static const u32 formats_kms[] = { >>>> DRM_FORMAT_YVU444, >>>> }; >>>> >>>> +static const uint64_t modifiers_kms[] = { >>>> + DRM_FORMAT_MOD_LINEAR, >>>> + DRM_FORMAT_MOD_INVALID, >>>> +}; >>>> + >>>> static const u32 formats_v4l2[] = { >>>> V4L2_PIX_FMT_RGB332, >>>> V4L2_PIX_FMT_ARGB444, >>>> @@ -344,6 +349,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) >>>> state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; >>>> } >>>> >>>> +static bool rcar_du_vsp_plane_format_mod_supported(struct drm_plane *plane, >>>> + uint32_t format, >>>> + uint64_t modifier) >>>> +{ >>>> + return modifier == DRM_FORMAT_MOD_LINEAR; >>>> +} >>>> + >>>> static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { >>>> .update_plane = drm_atomic_helper_update_plane, >>>> .disable_plane = drm_atomic_helper_disable_plane, >>>> @@ -351,6 +363,7 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { >>>> .destroy = drm_plane_cleanup, >>>> .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, >>>> .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, >>>> + .format_mod_supported = rcar_du_vsp_plane_format_mod_supported, >>>> }; >>>> >>>> int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, >>>> @@ -397,7 +410,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, >>>> &rcar_du_vsp_plane_funcs, >>>> formats_kms, >>>> ARRAY_SIZE(formats_kms), >>>> - NULL, type, NULL); >>>> + modifiers_kms, type, NULL); >>>> if (ret < 0) >>>> return ret; >>>> >
Hello Esaki-san, (CC'ing dri-devel and Daniel Stone) On Sat, May 11, 2019 at 09:10:27PM +0300, Laurent Pinchart wrote: > On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: > > Hi Laurent-san > > > > > What's the purpose of this, as it adds no new functionality to the > > > driver ? Why is this change needed ? > > > > Weston compositor (v5.0.0 or later) uses the DRM API to get the > > supported modifiers and determines if the sprite plane can be used by > > comparing the modifiers with the client specified modifier. > > In currently driver, since the weston doesn't know supported modifiers, > > that cannot determine if the received dmabuf can be passed through to > > sprite plane. > > Since there are R-Car GPU which support linear modifier, the sprite > > plane cannot be used in a compositor similar to the weston if client > > specify linear modifier. > > I don't think the right solution is to expose the linear modifier from > all drivers that don't otherwise support modifiers. We should instead > fix it either in Weston, and treat drivers that don't support the > modifiers API as supporting the linear modifier only, or in the DRM/KMS > core by reporting the linear modifier for drivers that don't explicitly > support modifiers. I've been pointed to https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3350#note_1161827, and we had a discussion on the #dri-devel IRC channel today on this topic. It turns out I was wrong, not specifying modifiers in userspace is different than specifying a linear modifier. This is true for some legacy drivers only (e.g. radeon) that pre-date the modifiers API, and which select a tiling format internally based on some heuristics. I still don't like this patch though, as it would need to be replicated in most drivers. It would be better if we could handle this in the DRM core. Daniel kindly offered to summarize the IRC discussion in a reply to this e-mail. > > On 2019/05/09 16:14, Laurent Pinchart wrote: > > > On Thu, May 09, 2019 at 02:45:18PM +0900, Tomohito Esaki wrote: > > >> Add support for the linear modifier. Since the rcar-du device supports > > >> only linear modifier, this driver doesn't support other modifiers. > > > > > > What's the purpose of this, as it adds no new functionality to the > > > driver ? Why is this change needed ? > > > > > >> Signed-off-by: Tomohito Esaki <etom@igel.co.jp> > > >> --- > > >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 +++++++++++ > > >> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 ++++++++++++++- > > >> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 15 ++++++++++++++- > > >> 3 files changed, 39 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > > >> index 3b7d50a8fb9b..9c5e15a5ab1c 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > > >> @@ -214,6 +214,16 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, > > >> return ERR_PTR(-EINVAL); > > >> } > > >> > > >> + /* > > >> + * Support only LINEAR modifier. > > >> + */ > > >> + if ((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) && > > >> + mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR) { > > >> + dev_dbg(dev->dev, "unsupported fb modifier 0x%llx\n", > > >> + mode_cmd->modifier[0]); > > >> + return ERR_PTR(-EINVAL); > > >> + } > > >> + > > >> if (rcdu->info->gen < 3) { > > >> /* > > >> * On Gen2 the DU limits the pitch to 4095 pixels and requires > > >> @@ -529,6 +539,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) > > >> dev->mode_config.min_width = 0; > > >> dev->mode_config.min_height = 0; > > >> dev->mode_config.normalize_zpos = true; > > >> + dev->mode_config.allow_fb_modifiers = true; > > >> dev->mode_config.funcs = &rcar_du_mode_config_funcs; > > >> dev->mode_config.helper_private = &rcar_du_mode_config_helper; > > >> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > > >> index c6430027169f..32135ad387fa 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > > >> @@ -726,6 +726,13 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, > > >> return 0; > > >> } > > >> > > >> +static bool rcar_du_plane_format_mod_supported(struct drm_plane *plane, > > >> + uint32_t format, > > >> + uint64_t modifier) > > >> +{ > > >> + return modifier == DRM_FORMAT_MOD_LINEAR; > > >> +} > > >> + > > >> static const struct drm_plane_funcs rcar_du_plane_funcs = { > > >> .update_plane = drm_atomic_helper_update_plane, > > >> .disable_plane = drm_atomic_helper_disable_plane, > > >> @@ -735,6 +742,7 @@ static const struct drm_plane_funcs rcar_du_plane_funcs = { > > >> .atomic_destroy_state = rcar_du_plane_atomic_destroy_state, > > >> .atomic_set_property = rcar_du_plane_atomic_set_property, > > >> .atomic_get_property = rcar_du_plane_atomic_get_property, > > >> + .format_mod_supported = rcar_du_plane_format_mod_supported, > > >> }; > > >> > > >> static const uint32_t formats[] = { > > >> @@ -750,6 +758,11 @@ static const uint32_t formats[] = { > > >> DRM_FORMAT_NV16, > > >> }; > > >> > > >> +static const uint64_t modifiers[] = { > > >> + DRM_FORMAT_MOD_LINEAR, > > >> + DRM_FORMAT_MOD_INVALID, > > >> +}; > > >> + > > >> int rcar_du_planes_init(struct rcar_du_group *rgrp) > > >> { > > >> struct rcar_du_device *rcdu = rgrp->dev; > > >> @@ -776,7 +789,7 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) > > >> ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs, > > >> &rcar_du_plane_funcs, formats, > > >> ARRAY_SIZE(formats), > > >> - NULL, type, NULL); > > >> + modifiers, type, NULL); > > >> if (ret < 0) > > >> return ret; > > >> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > >> index 0878accbd134..9d1382b02717 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > >> @@ -139,6 +139,11 @@ static const u32 formats_kms[] = { > > >> DRM_FORMAT_YVU444, > > >> }; > > >> > > >> +static const uint64_t modifiers_kms[] = { > > >> + DRM_FORMAT_MOD_LINEAR, > > >> + DRM_FORMAT_MOD_INVALID, > > >> +}; > > >> + > > >> static const u32 formats_v4l2[] = { > > >> V4L2_PIX_FMT_RGB332, > > >> V4L2_PIX_FMT_ARGB444, > > >> @@ -344,6 +349,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) > > >> state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; > > >> } > > >> > > >> +static bool rcar_du_vsp_plane_format_mod_supported(struct drm_plane *plane, > > >> + uint32_t format, > > >> + uint64_t modifier) > > >> +{ > > >> + return modifier == DRM_FORMAT_MOD_LINEAR; > > >> +} > > >> + > > >> static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > > >> .update_plane = drm_atomic_helper_update_plane, > > >> .disable_plane = drm_atomic_helper_disable_plane, > > >> @@ -351,6 +363,7 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > > >> .destroy = drm_plane_cleanup, > > >> .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, > > >> .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, > > >> + .format_mod_supported = rcar_du_vsp_plane_format_mod_supported, > > >> }; > > >> > > >> int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > > >> @@ -397,7 +410,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > > >> &rcar_du_vsp_plane_funcs, > > >> formats_kms, > > >> ARRAY_SIZE(formats_kms), > > >> - NULL, type, NULL); > > >> + modifiers_kms, type, NULL); > > >> if (ret < 0) > > >> return ret; > > >>
Hi all, Thanks for this Laurent. Esaki-san, could you please CC dri-devel@ on discussions like this? On Thu, 18 Nov 2021 at 12:32, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Sat, May 11, 2019 at 09:10:27PM +0300, Laurent Pinchart wrote: > > On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: > > > Weston compositor (v5.0.0 or later) uses the DRM API to get the > > > supported modifiers and determines if the sprite plane can be used by > > > comparing the modifiers with the client specified modifier. > > > In currently driver, since the weston doesn't know supported modifiers, > > > that cannot determine if the received dmabuf can be passed through to > > > sprite plane. > > > Since there are R-Car GPU which support linear modifier, the sprite > > > plane cannot be used in a compositor similar to the weston if client > > > specify linear modifier. > > > > I don't think the right solution is to expose the linear modifier from > > all drivers that don't otherwise support modifiers. We should instead > > fix it either in Weston, and treat drivers that don't support the > > modifiers API as supporting the linear modifier only, or in the DRM/KMS > > core by reporting the linear modifier for drivers that don't explicitly > > support modifiers. > > I've been pointed to > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3350#note_1161827, > and we had a discussion on the #dri-devel IRC channel today on this > topic. It turns out I was wrong, not specifying modifiers in userspace > is different than specifying a linear modifier. This is true for some > legacy drivers only (e.g. radeon) that pre-date the modifiers API, and > which select a tiling format internally based on some heuristics. > > I still don't like this patch though, as it would need to be replicated > in most drivers. It would be better if we could handle this in the DRM > core. Daniel kindly offered to summarize the IRC discussion in a reply > to this e-mail. Just quickly, I believe the check for the linear modifier in fb_create is unnecessary, as this should already be checked in the core through format_mod_supported(). There is indeed a difference between LINEAR and INVALID. Linear is an explicit declaration of the layout; INVALID (i.e. no modifier) means 'I don't know what this is, so you should guess'. Guessing is obviously not reliable, so Weston only passes buffers with no modifier to KMS in two cases. The first case is when we allocate a dumb buffer and the driver does not support modifiers; this is safe since it's the same driver. The second case is when either the GPU driver or KMS driver do not support modifiers and we allocate a buffer via GBM with USE_SCANOUT; in this case, it is GBM's responsibility to select the 'right' layout. We will never create a DRM framebuffer with no modifiers when the original buffer comes from a client. If the client does not support modifiers but the KMS driver does, then we do not know that the client has allocated a suitable layout, so this is not safe. If the client does explicitly declare a modifier but the KMS driver does not support modifiers, then we also do not know that this is safe to use. So unless both sides (client/GPU and KMS) support modifiers, we do not do direct scanout from client buffers. This patch would enable this usecase by declaring support for the linear modifier from KMS; when used with a PVR driver which explicitly declares the linear modifier, we know it is safe to pass that client buffer to KMS. Laurent's concern is that the DRM core should handle this rather than open-coding in every driver, which I agree with. Some drivers (e.g. radeon, maybe legacy NV?) do not support modifiers, and _also_ do magic inference of the actual layout of the underlying buffer. However, these drivers are legacy and we do not accept any new addition of inferring layout without modifiers. I think the best way forward here is: - add a new mode_config.cannot_support_modifiers flag, and enable this in radeon (plus any other drivers in the same boat) - change drm_universal_plane_init() to advertise the LINEAR modifier when NULL is passed as the modifier list (including installing a default .format_mod_supported hook) - remove the mode_config.allow_fb_modifiers hook and always advertise modifier support, unless mode_config.cannot_support_modifiers is set FWIW, the effective modifier API and also valid usage is documented here, which should be finished and merged shortly: https://lore.kernel.org/dri-devel/20210905122742.86029-1-daniels@collabora.com/ Cheers, Daniel
Hi Daniel, On Thu, Nov 18, 2021 at 01:02:11PM +0000, Daniel Stone wrote: > Hi all, > Thanks for this Laurent. Esaki-san, could you please CC dri-devel@ on > discussions like this? > > On Thu, 18 Nov 2021 at 12:32, Laurent Pinchart wrote: > > On Sat, May 11, 2019 at 09:10:27PM +0300, Laurent Pinchart wrote: > > > On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: > > > > Weston compositor (v5.0.0 or later) uses the DRM API to get the > > > > supported modifiers and determines if the sprite plane can be used by > > > > comparing the modifiers with the client specified modifier. > > > > In currently driver, since the weston doesn't know supported modifiers, > > > > that cannot determine if the received dmabuf can be passed through to > > > > sprite plane. > > > > Since there are R-Car GPU which support linear modifier, the sprite > > > > plane cannot be used in a compositor similar to the weston if client > > > > specify linear modifier. > > > > > > I don't think the right solution is to expose the linear modifier from > > > all drivers that don't otherwise support modifiers. We should instead > > > fix it either in Weston, and treat drivers that don't support the > > > modifiers API as supporting the linear modifier only, or in the DRM/KMS > > > core by reporting the linear modifier for drivers that don't explicitly > > > support modifiers. > > > > I've been pointed to > > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3350#note_1161827, > > and we had a discussion on the #dri-devel IRC channel today on this > > topic. It turns out I was wrong, not specifying modifiers in userspace > > is different than specifying a linear modifier. This is true for some > > legacy drivers only (e.g. radeon) that pre-date the modifiers API, and > > which select a tiling format internally based on some heuristics. > > > > I still don't like this patch though, as it would need to be replicated > > in most drivers. It would be better if we could handle this in the DRM > > core. Daniel kindly offered to summarize the IRC discussion in a reply > > to this e-mail. > > Just quickly, I believe the check for the linear modifier in fb_create > is unnecessary, as this should already be checked in the core through > format_mod_supported(). > > There is indeed a difference between LINEAR and INVALID. Linear is an > explicit declaration of the layout; INVALID (i.e. no modifier) means > 'I don't know what this is, so you should guess'. Guessing is > obviously not reliable, so Weston only passes buffers with no modifier > to KMS in two cases. The first case is when we allocate a dumb buffer > and the driver does not support modifiers; this is safe since it's the > same driver. The second case is when either the GPU driver or KMS > driver do not support modifiers and we allocate a buffer via GBM with > USE_SCANOUT; in this case, it is GBM's responsibility to select the > 'right' layout. > > We will never create a DRM framebuffer with no modifiers when the > original buffer comes from a client. If the client does not support > modifiers but the KMS driver does, then we do not know that the client > has allocated a suitable layout, so this is not safe. If the client > does explicitly declare a modifier but the KMS driver does not support > modifiers, then we also do not know that this is safe to use. So > unless both sides (client/GPU and KMS) support modifiers, we do not do > direct scanout from client buffers. > > This patch would enable this usecase by declaring support for the > linear modifier from KMS; when used with a PVR driver which explicitly > declares the linear modifier, we know it is safe to pass that client > buffer to KMS. > > Laurent's concern is that the DRM core should handle this rather than > open-coding in every driver, which I agree with. Some drivers (e.g. > radeon, maybe legacy NV?) do not support modifiers, and _also_ do > magic inference of the actual layout of the underlying buffer. > However, these drivers are legacy and we do not accept any new > addition of inferring layout without modifiers. > > I think the best way forward here is: > - add a new mode_config.cannot_support_modifiers flag, and enable > this in radeon (plus any other drivers in the same boat) Is there an easy way to identify the drivers that need this ? > - change drm_universal_plane_init() to advertise the LINEAR modifier > when NULL is passed as the modifier list (including installing a > default .format_mod_supported hook) > - remove the mode_config.allow_fb_modifiers hook and always > advertise modifier support, unless > mode_config.cannot_support_modifiers is set Looks good to me. > FWIW, the effective modifier API and also valid usage is documented > here, which should be finished and merged shortly: > https://lore.kernel.org/dri-devel/20210905122742.86029-1-daniels@collabora.com/
Hello Daniel Thanks for the summary of the discussion. On 2021/11/18 22:02, Daniel Stone wrote: > Hi all, > Thanks for this Laurent. Esaki-san, could you please CC dri-devel@ on > discussions like this? I'm sorry. I will check using get_maintainer.pl next time. > On Thu, 18 Nov 2021 at 12:32, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> On Sat, May 11, 2019 at 09:10:27PM +0300, Laurent Pinchart wrote: >>> On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: >>>> Weston compositor (v5.0.0 or later) uses the DRM API to get the >>>> supported modifiers and determines if the sprite plane can be used by >>>> comparing the modifiers with the client specified modifier. >>>> In currently driver, since the weston doesn't know supported modifiers, >>>> that cannot determine if the received dmabuf can be passed through to >>>> sprite plane. >>>> Since there are R-Car GPU which support linear modifier, the sprite >>>> plane cannot be used in a compositor similar to the weston if client >>>> specify linear modifier. >>> >>> I don't think the right solution is to expose the linear modifier from >>> all drivers that don't otherwise support modifiers. We should instead >>> fix it either in Weston, and treat drivers that don't support the >>> modifiers API as supporting the linear modifier only, or in the DRM/KMS >>> core by reporting the linear modifier for drivers that don't explicitly >>> support modifiers. >> >> I've been pointed to >> https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3350#note_1161827, >> and we had a discussion on the #dri-devel IRC channel today on this >> topic. It turns out I was wrong, not specifying modifiers in userspace >> is different than specifying a linear modifier. This is true for some >> legacy drivers only (e.g. radeon) that pre-date the modifiers API, and >> which select a tiling format internally based on some heuristics. >> >> I still don't like this patch though, as it would need to be replicated >> in most drivers. It would be better if we could handle this in the DRM >> core. Daniel kindly offered to summarize the IRC discussion in a reply >> to this e-mail. > > Just quickly, I believe the check for the linear modifier in fb_create > is unnecessary, as this should already be checked in the core through > format_mod_supported(). > > There is indeed a difference between LINEAR and INVALID. Linear is an > explicit declaration of the layout; INVALID (i.e. no modifier) means > 'I don't know what this is, so you should guess'. Guessing is > obviously not reliable, so Weston only passes buffers with no modifier > to KMS in two cases. The first case is when we allocate a dumb buffer > and the driver does not support modifiers; this is safe since it's the > same driver. The second case is when either the GPU driver or KMS > driver do not support modifiers and we allocate a buffer via GBM with > USE_SCANOUT; in this case, it is GBM's responsibility to select the > 'right' layout. > > We will never create a DRM framebuffer with no modifiers when the > original buffer comes from a client. If the client does not support > modifiers but the KMS driver does, then we do not know that the client > has allocated a suitable layout, so this is not safe. If the client > does explicitly declare a modifier but the KMS driver does not support > modifiers, then we also do not know that this is safe to use. So > unless both sides (client/GPU and KMS) support modifiers, we do not do > direct scanout from client buffers. > > This patch would enable this usecase by declaring support for the > linear modifier from KMS; when used with a PVR driver which explicitly > declares the linear modifier, we know it is safe to pass that client > buffer to KMS. > > Laurent's concern is that the DRM core should handle this rather than > open-coding in every driver, which I agree with. Some drivers (e.g. > radeon, maybe legacy NV?) do not support modifiers, and _also_ do > magic inference of the actual layout of the underlying buffer. > However, these drivers are legacy and we do not accept any new > addition of inferring layout without modifiers. > > I think the best way forward here is: > - add a new mode_config.cannot_support_modifiers flag, and enable > this in radeon (plus any other drivers in the same boat) > - change drm_universal_plane_init() to advertise the LINEAR modifier > when NULL is passed as the modifier list (including installing a > default .format_mod_supported hook) > - remove the mode_config.allow_fb_modifiers hook and always > advertise modifier support, unless > mode_config.cannot_support_modifiers is set > > FWIW, the effective modifier API and also valid usage is documented > here, which should be finished and merged shortly: > https://lore.kernel.org/dri-devel/20210905122742.86029-1-daniels@collabora.com/ > > Cheers, > Daniel > -- Best Regards Tomohito Esaki
Hi Daniel, On 2021/11/18 23:05, Laurent Pinchart wrote: > Hi Daniel, > > On Thu, Nov 18, 2021 at 01:02:11PM +0000, Daniel Stone wrote: >> Hi all, >> Thanks for this Laurent. Esaki-san, could you please CC dri-devel@ on >> discussions like this? >> >> On Thu, 18 Nov 2021 at 12:32, Laurent Pinchart wrote: >>> On Sat, May 11, 2019 at 09:10:27PM +0300, Laurent Pinchart wrote: >>>> On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: >>>>> Weston compositor (v5.0.0 or later) uses the DRM API to get the >>>>> supported modifiers and determines if the sprite plane can be used by >>>>> comparing the modifiers with the client specified modifier. >>>>> In currently driver, since the weston doesn't know supported modifiers, >>>>> that cannot determine if the received dmabuf can be passed through to >>>>> sprite plane. >>>>> Since there are R-Car GPU which support linear modifier, the sprite >>>>> plane cannot be used in a compositor similar to the weston if client >>>>> specify linear modifier. >>>> >>>> I don't think the right solution is to expose the linear modifier from >>>> all drivers that don't otherwise support modifiers. We should instead >>>> fix it either in Weston, and treat drivers that don't support the >>>> modifiers API as supporting the linear modifier only, or in the DRM/KMS >>>> core by reporting the linear modifier for drivers that don't explicitly >>>> support modifiers. >>> >>> I've been pointed to >>> https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3350#note_1161827, >>> and we had a discussion on the #dri-devel IRC channel today on this >>> topic. It turns out I was wrong, not specifying modifiers in userspace >>> is different than specifying a linear modifier. This is true for some >>> legacy drivers only (e.g. radeon) that pre-date the modifiers API, and >>> which select a tiling format internally based on some heuristics. >>> >>> I still don't like this patch though, as it would need to be replicated >>> in most drivers. It would be better if we could handle this in the DRM >>> core. Daniel kindly offered to summarize the IRC discussion in a reply >>> to this e-mail. >> >> Just quickly, I believe the check for the linear modifier in fb_create >> is unnecessary, as this should already be checked in the core through >> format_mod_supported(). >> >> There is indeed a difference between LINEAR and INVALID. Linear is an >> explicit declaration of the layout; INVALID (i.e. no modifier) means >> 'I don't know what this is, so you should guess'. Guessing is >> obviously not reliable, so Weston only passes buffers with no modifier >> to KMS in two cases. The first case is when we allocate a dumb buffer >> and the driver does not support modifiers; this is safe since it's the >> same driver. The second case is when either the GPU driver or KMS >> driver do not support modifiers and we allocate a buffer via GBM with >> USE_SCANOUT; in this case, it is GBM's responsibility to select the >> 'right' layout. >> >> We will never create a DRM framebuffer with no modifiers when the >> original buffer comes from a client. If the client does not support >> modifiers but the KMS driver does, then we do not know that the client >> has allocated a suitable layout, so this is not safe. If the client >> does explicitly declare a modifier but the KMS driver does not support >> modifiers, then we also do not know that this is safe to use. So >> unless both sides (client/GPU and KMS) support modifiers, we do not do >> direct scanout from client buffers. >> >> This patch would enable this usecase by declaring support for the >> linear modifier from KMS; when used with a PVR driver which explicitly >> declares the linear modifier, we know it is safe to pass that client >> buffer to KMS. >> >> Laurent's concern is that the DRM core should handle this rather than >> open-coding in every driver, which I agree with. Some drivers (e.g. >> radeon, maybe legacy NV?) do not support modifiers, and _also_ do >> magic inference of the actual layout of the underlying buffer. >> However, these drivers are legacy and we do not accept any new >> addition of inferring layout without modifiers. >> >> I think the best way forward here is: >> - add a new mode_config.cannot_support_modifiers flag, and enable >> this in radeon (plus any other drivers in the same boat) > > Is there an easy way to identify the drivers that need this ? Should I find a driver that has not use drm_plane_funcs? >> - change drm_universal_plane_init() to advertise the LINEAR modifier >> when NULL is passed as the modifier list (including installing a >> default .format_mod_supported hook) >> - remove the mode_config.allow_fb_modifiers hook and always >> advertise modifier support, unless >> mode_config.cannot_support_modifiers is set > > Looks good to me. I agree with this way, I'll try to create a patches. >> FWIW, the effective modifier API and also valid usage is documented >> here, which should be finished and merged shortly: >> https://lore.kernel.org/dri-devel/20210905122742.86029-1-daniels@collabora.com/ >
Hi Esaki-san, On Tue, 30 Nov 2021 at 08:44, Esaki Tomohito <etom@igel.co.jp> wrote: > On 2021/11/18 23:05, Laurent Pinchart wrote: > > On Thu, Nov 18, 2021 at 01:02:11PM +0000, Daniel Stone wrote: > >> Laurent's concern is that the DRM core should handle this rather than > >> open-coding in every driver, which I agree with. Some drivers (e.g. > >> radeon, maybe legacy NV?) do not support modifiers, and _also_ do > >> magic inference of the actual layout of the underlying buffer. > >> However, these drivers are legacy and we do not accept any new > >> addition of inferring layout without modifiers. > >> > >> I think the best way forward here is: > >> - add a new mode_config.cannot_support_modifiers flag, and enable > >> this in radeon (plus any other drivers in the same boat) > > > > Is there an easy way to identify the drivers that need this ? > > Should I find a driver that has not use drm_plane_funcs? I don't think there's a good way to systematically audit it. The only two I know are radeon (i.e. pre-amdgpu) and nouveau (pre-nv50), both of which pass no modifiers to drm_universal_plane_init(), but do have magic back channels to communicate tiling information. If anyone knows of any others, well, I guess we'll find out. :) > >> - change drm_universal_plane_init() to advertise the LINEAR modifier > >> when NULL is passed as the modifier list (including installing a > >> default .format_mod_supported hook) ... except when cannot_support_modifiers is set. > >> - remove the mode_config.allow_fb_modifiers hook and always > >> advertise modifier support, unless > >> mode_config.cannot_support_modifiers is set > > > > Looks good to me. > > I agree with this way, I'll try to create a patches. Thanks a lot! :) Cheers, Daniel
Hi Daniel-san, On 2021/11/30 22:20, Daniel Stone wrote: > On Tue, 30 Nov 2021 at 08:44, Esaki Tomohito <etom@igel.co.jp> wrote: >> On 2021/11/18 23:05, Laurent Pinchart wrote: >>> On Thu, Nov 18, 2021 at 01:02:11PM +0000, Daniel Stone wrote: >>>> Laurent's concern is that the DRM core should handle this rather than >>>> open-coding in every driver, which I agree with. Some drivers (e.g. >>>> radeon, maybe legacy NV?) do not support modifiers, and _also_ do >>>> magic inference of the actual layout of the underlying buffer. >>>> However, these drivers are legacy and we do not accept any new >>>> addition of inferring layout without modifiers. >>>> >>>> I think the best way forward here is: >>>> - add a new mode_config.cannot_support_modifiers flag, and enable >>>> this in radeon (plus any other drivers in the same boat) >>> >>> Is there an easy way to identify the drivers that need this ? >> >> Should I find a driver that has not use drm_plane_funcs? > > I don't think there's a good way to systematically audit it. The only > two I know are radeon (i.e. pre-amdgpu) and nouveau (pre-nv50), both > of which pass no modifiers to drm_universal_plane_init(), but do have > magic back channels to communicate tiling information. If anyone knows > of any others, well, I guess we'll find out. :) Thanks for the information. I checked the driver code with the tiling keyword, and it seems that these are the only two drivers that require cannot_support_modifiers. I will start creating patches for these two drivers first.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 3b7d50a8fb9b..9c5e15a5ab1c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -214,6 +214,16 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, return ERR_PTR(-EINVAL); } + /* + * Support only LINEAR modifier. + */ + if ((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) && + mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR) { + dev_dbg(dev->dev, "unsupported fb modifier 0x%llx\n", + mode_cmd->modifier[0]); + return ERR_PTR(-EINVAL); + } + if (rcdu->info->gen < 3) { /* * On Gen2 the DU limits the pitch to 4095 pixels and requires @@ -529,6 +539,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) dev->mode_config.min_width = 0; dev->mode_config.min_height = 0; dev->mode_config.normalize_zpos = true; + dev->mode_config.allow_fb_modifiers = true; dev->mode_config.funcs = &rcar_du_mode_config_funcs; dev->mode_config.helper_private = &rcar_du_mode_config_helper; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index c6430027169f..32135ad387fa 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -726,6 +726,13 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, return 0; } +static bool rcar_du_plane_format_mod_supported(struct drm_plane *plane, + uint32_t format, + uint64_t modifier) +{ + return modifier == DRM_FORMAT_MOD_LINEAR; +} + static const struct drm_plane_funcs rcar_du_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -735,6 +742,7 @@ static const struct drm_plane_funcs rcar_du_plane_funcs = { .atomic_destroy_state = rcar_du_plane_atomic_destroy_state, .atomic_set_property = rcar_du_plane_atomic_set_property, .atomic_get_property = rcar_du_plane_atomic_get_property, + .format_mod_supported = rcar_du_plane_format_mod_supported, }; static const uint32_t formats[] = { @@ -750,6 +758,11 @@ static const uint32_t formats[] = { DRM_FORMAT_NV16, }; +static const uint64_t modifiers[] = { + DRM_FORMAT_MOD_LINEAR, + DRM_FORMAT_MOD_INVALID, +}; + int rcar_du_planes_init(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev; @@ -776,7 +789,7 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs, &rcar_du_plane_funcs, formats, ARRAY_SIZE(formats), - NULL, type, NULL); + modifiers, type, NULL); if (ret < 0) return ret; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 0878accbd134..9d1382b02717 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -139,6 +139,11 @@ static const u32 formats_kms[] = { DRM_FORMAT_YVU444, }; +static const uint64_t modifiers_kms[] = { + DRM_FORMAT_MOD_LINEAR, + DRM_FORMAT_MOD_INVALID, +}; + static const u32 formats_v4l2[] = { V4L2_PIX_FMT_RGB332, V4L2_PIX_FMT_ARGB444, @@ -344,6 +349,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; } +static bool rcar_du_vsp_plane_format_mod_supported(struct drm_plane *plane, + uint32_t format, + uint64_t modifier) +{ + return modifier == DRM_FORMAT_MOD_LINEAR; +} + static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -351,6 +363,7 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .destroy = drm_plane_cleanup, .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, + .format_mod_supported = rcar_du_vsp_plane_format_mod_supported, }; int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, @@ -397,7 +410,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, &rcar_du_vsp_plane_funcs, formats_kms, ARRAY_SIZE(formats_kms), - NULL, type, NULL); + modifiers_kms, type, NULL); if (ret < 0) return ret;
Add support for the linear modifier. Since the rcar-du device supports only linear modifier, this driver doesn't support other modifiers. Signed-off-by: Tomohito Esaki <etom@igel.co.jp> --- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 +++++++++++ drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 ++++++++++++++- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 15 ++++++++++++++- 3 files changed, 39 insertions(+), 2 deletions(-)