Re: [PATCH v4 77/78] drm/vc4: drv: Support BCM2711
Hi Dave,
On Tue, Jul 28, 2020 at 04:30:16PM +0100, Dave Stevenson wrote:
> > @@ -681,10 +684,14 @@ int vc4_kms_load(struct drm_device *dev)
> > struct vc4_load_tracker_state *load_state;
> > int ret;
> >
> > - /* Start with the load tracker enabled. Can be disabled through the
> > -* debugfs load_tracker file.
> > -*/
> > - vc4->load_tracker_enabled = true;
> > + if (!of_device_is_compatible(dev->dev->of_node,
> > "brcm,bcm2711-vc5")) {
>
> Is it better to look up the compatible string, or pass something via
> the .data element of the of_device_id table? Probably down to personal
> preference?
It's pretty much equivalent, so I'm not sure one is arguably better than
the other. However, checking for the compatible can be pretty cumbersome
when you have to do it repeatedly (like we do in the HDMI controller),
and when you don't it a lot, having a structure associated to the
compatible is also fairly cumbersome.
> > + vc4->load_tracker_available = true;
> > +
> > + /* Start with the load tracker enabled. Can be
> > +* disabled through the debugfs load_tracker file.
> > +*/
> > + vc4->load_tracker_enabled = true;
> > + }
> >
> > sema_init(&vc4->async_modeset, 1);
> >
> > @@ -698,8 +705,14 @@ int vc4_kms_load(struct drm_device *dev)
> > return ret;
> > }
> >
> > - dev->mode_config.max_width = 2048;
> > - dev->mode_config.max_height = 2048;
> > + if (of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5"))
> > {
>
> We're making the same of_device_is_compatible call twice within
> vc4_kms_load. Set a flag based on it and check that instead?
Good idea, thanks!
Maxime
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 77/78] drm/vc4: drv: Support BCM2711
Hi Maxime
On Wed, 8 Jul 2020 at 18:44, Maxime Ripard wrote:
>
> The BCM2711 has a reworked display pipeline, and the load tracker needs
> some adjustement to operate properly. Let's add a compatible for BCM2711
s/adjustement/adjustment
> and disable the load tracker until properly supported.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_drv.c | 1 +-
> drivers/gpu/drm/vc4/vc4_drv.h | 3 ++-
> drivers/gpu/drm/vc4/vc4_kms.c | 42 +++---
> drivers/gpu/drm/vc4/vc4_plane.c | 5 -
> 4 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 9567d1019212..f1a5fd5dab6f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -372,6 +372,7 @@ static int vc4_platform_drm_remove(struct platform_device
> *pdev)
> }
>
> static const struct of_device_id vc4_of_match[] = {
> + { .compatible = "brcm,bcm2711-vc5", },
> { .compatible = "brcm,bcm2835-vc4", },
> { .compatible = "brcm,cygnus-vc4", },
> {},
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 501a48a714d3..8c8d96b6289f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -200,6 +200,9 @@ struct vc4_dev {
>
> int power_refcount;
>
> + /* Set to true when the load tracker is supported. */
> + bool load_tracker_available;
> +
> /* Set to true when the load tracker is active. */
> bool load_tracker_enabled;
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 7c8a87339959..ae479f988666 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -532,6 +532,9 @@ static int vc4_load_tracker_atomic_check(struct
> drm_atomic_state *state)
> struct drm_plane *plane;
> int i;
>
> + if (!vc4->load_tracker_available)
> + return 0;
> +
> priv_state = drm_atomic_get_private_obj_state(state,
> &vc4->load_tracker);
> if (IS_ERR(priv_state))
> @@ -681,10 +684,14 @@ int vc4_kms_load(struct drm_device *dev)
> struct vc4_load_tracker_state *load_state;
> int ret;
>
> - /* Start with the load tracker enabled. Can be disabled through the
> -* debugfs load_tracker file.
> -*/
> - vc4->load_tracker_enabled = true;
> + if (!of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5")) {
Is it better to look up the compatible string, or pass something via
the .data element of the of_device_id table? Probably down to personal
preference?
> + vc4->load_tracker_available = true;
> +
> + /* Start with the load tracker enabled. Can be
> +* disabled through the debugfs load_tracker file.
> +*/
> + vc4->load_tracker_enabled = true;
> + }
>
> sema_init(&vc4->async_modeset, 1);
>
> @@ -698,8 +705,14 @@ int vc4_kms_load(struct drm_device *dev)
> return ret;
> }
>
> - dev->mode_config.max_width = 2048;
> - dev->mode_config.max_height = 2048;
> + if (of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5")) {
We're making the same of_device_is_compatible call twice within
vc4_kms_load. Set a flag based on it and check that instead?
Dave
> + dev->mode_config.max_width = 7680;
> + dev->mode_config.max_height = 7680;
> + } else {
> + dev->mode_config.max_width = 2048;
> + dev->mode_config.max_height = 2048;
> + }
> +
> dev->mode_config.funcs = &vc4_mode_funcs;
> dev->mode_config.preferred_depth = 24;
> dev->mode_config.async_page_flip = true;
> @@ -714,14 +727,17 @@ int vc4_kms_load(struct drm_device *dev)
> drm_atomic_private_obj_init(dev, &vc4->ctm_manager, &ctm_state->base,
> &vc4_ctm_state_funcs);
>
> - load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
> - if (!load_state) {
> - drm_atomic_private_obj_fini(&vc4->ctm_manager);
> - return -ENOMEM;
> - }
> + if (vc4->load_tracker_available) {
> + load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
> + if (!load_state) {
> + drm_atomic_private_obj_fini(&vc4->ctm_manager);
> + return -ENOMEM;
> + }
>
> - drm_atomic_private_obj_init(dev, &vc4->load_tracker,
> &load_state->base,
> - &vc4_load_tracker_state_funcs);
> + drm_atomic_private_obj_init(dev, &vc4->load_tracker,
> + &load_state->base,
> + &vc4_load_tracker_state_funcs);
> + }
>
> drm_mode_config_reset(dev);
[PATCH v4 77/78] drm/vc4: drv: Support BCM2711
The BCM2711 has a reworked display pipeline, and the load tracker needs
some adjustement to operate properly. Let's add a compatible for BCM2711
and disable the load tracker until properly supported.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_drv.c | 1 +-
drivers/gpu/drm/vc4/vc4_drv.h | 3 ++-
drivers/gpu/drm/vc4/vc4_kms.c | 42 +++---
drivers/gpu/drm/vc4/vc4_plane.c | 5 -
4 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 9567d1019212..f1a5fd5dab6f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -372,6 +372,7 @@ static int vc4_platform_drm_remove(struct platform_device
*pdev)
}
static const struct of_device_id vc4_of_match[] = {
+ { .compatible = "brcm,bcm2711-vc5", },
{ .compatible = "brcm,bcm2835-vc4", },
{ .compatible = "brcm,cygnus-vc4", },
{},
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 501a48a714d3..8c8d96b6289f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -200,6 +200,9 @@ struct vc4_dev {
int power_refcount;
+ /* Set to true when the load tracker is supported. */
+ bool load_tracker_available;
+
/* Set to true when the load tracker is active. */
bool load_tracker_enabled;
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 7c8a87339959..ae479f988666 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -532,6 +532,9 @@ static int vc4_load_tracker_atomic_check(struct
drm_atomic_state *state)
struct drm_plane *plane;
int i;
+ if (!vc4->load_tracker_available)
+ return 0;
+
priv_state = drm_atomic_get_private_obj_state(state,
&vc4->load_tracker);
if (IS_ERR(priv_state))
@@ -681,10 +684,14 @@ int vc4_kms_load(struct drm_device *dev)
struct vc4_load_tracker_state *load_state;
int ret;
- /* Start with the load tracker enabled. Can be disabled through the
-* debugfs load_tracker file.
-*/
- vc4->load_tracker_enabled = true;
+ if (!of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5")) {
+ vc4->load_tracker_available = true;
+
+ /* Start with the load tracker enabled. Can be
+* disabled through the debugfs load_tracker file.
+*/
+ vc4->load_tracker_enabled = true;
+ }
sema_init(&vc4->async_modeset, 1);
@@ -698,8 +705,14 @@ int vc4_kms_load(struct drm_device *dev)
return ret;
}
- dev->mode_config.max_width = 2048;
- dev->mode_config.max_height = 2048;
+ if (of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5")) {
+ dev->mode_config.max_width = 7680;
+ dev->mode_config.max_height = 7680;
+ } else {
+ dev->mode_config.max_width = 2048;
+ dev->mode_config.max_height = 2048;
+ }
+
dev->mode_config.funcs = &vc4_mode_funcs;
dev->mode_config.preferred_depth = 24;
dev->mode_config.async_page_flip = true;
@@ -714,14 +727,17 @@ int vc4_kms_load(struct drm_device *dev)
drm_atomic_private_obj_init(dev, &vc4->ctm_manager, &ctm_state->base,
&vc4_ctm_state_funcs);
- load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
- if (!load_state) {
- drm_atomic_private_obj_fini(&vc4->ctm_manager);
- return -ENOMEM;
- }
+ if (vc4->load_tracker_available) {
+ load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
+ if (!load_state) {
+ drm_atomic_private_obj_fini(&vc4->ctm_manager);
+ return -ENOMEM;
+ }
- drm_atomic_private_obj_init(dev, &vc4->load_tracker, &load_state->base,
- &vc4_load_tracker_state_funcs);
+ drm_atomic_private_obj_init(dev, &vc4->load_tracker,
+ &load_state->base,
+ &vc4_load_tracker_state_funcs);
+ }
drm_mode_config_reset(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 1e38e603f83b..24d7e6db6fdd 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -516,6 +516,11 @@ static void vc4_plane_calc_load(struct drm_plane_state
*state)
struct vc4_plane_state *vc4_state;
struct drm_crtc_state *crtc_state;
unsigned int vscale_factor;
+ struct vc4_dev *vc4;
+
+ vc4 = to_vc4_dev(state->plane->dev);
+ if (!vc4->load_tracker_available)
+ return;
vc4_state = to_vc4_plane_sta
