[PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Maxime Ripard]

The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
embeds drm_crtc_state as its first member, and therefore can be safely
casted.

However, this is pretty fragile especially since there's no check for this
in place, and we're going to need to access vc4_crtc_state member at reset
so this looks like a good occasion to make it more robust.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

---

Changes from v1:
  - new patch
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index a393f93390a2..7ef20adedee5 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
 
 void vc4_crtc_reset(struct drm_crtc *crtc)
 {
+	struct vc4_crtc_state *vc4_crtc_state;
+
 	if (crtc->state)
 		vc4_crtc_destroy_state(crtc, crtc->state);
-	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
-	if (crtc->state)
-		__drm_atomic_helper_crtc_reset(crtc, crtc->state);
+
+	vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
+	if (!vc4_crtc_state) {
+		crtc->state = NULL;
+		return;
+	}
+
+	__drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
 }
 
 static const struct drm_crtc_funcs vc4_crtc_funcs = {
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Maxime Ripard]

The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
embeds drm_crtc_state as its first member, and therefore can be safely
casted.

However, this is pretty fragile especially since there's no check for this
in place, and we're going to need to access vc4_crtc_state member at reset
so this looks like a good occasion to make it more robust.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

---

Changes from v1:
  - new patch
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index a393f93390a2..7ef20adedee5 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
 
 void vc4_crtc_reset(struct drm_crtc *crtc)
 {
+	struct vc4_crtc_state *vc4_crtc_state;
+
 	if (crtc->state)
 		vc4_crtc_destroy_state(crtc, crtc->state);
-	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
-	if (crtc->state)
-		__drm_atomic_helper_crtc_reset(crtc, crtc->state);
+
+	vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
+	if (!vc4_crtc_state) {
+		crtc->state = NULL;
+		return;
+	}
+
+	__drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
 }
 
 static const struct drm_crtc_funcs vc4_crtc_funcs = {
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO

[Maxime Ripard]

The HVS FIFOs are currently assigned each time we have an atomic_check
for all the enabled CRTCs.

However, if we are running multiple outputs in parallel and we happen to
disable the first (by index) CRTC, we end up changing the assigned FIFO
of the second CRTC without disabling and reenabling the pixelvalve which
ends up in a stall and eventually a VBLANK timeout.

In order to fix this, we can create a special value for our assigned
channel to mark it as disabled, and if our CRTC already had an assigned
channel in its previous state, we keep on using it.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

---

Changes from v1:
  - Split away the crtc state reset refactoring
  - Fixed the checkpatch warnings
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  1 +
 drivers/gpu/drm/vc4/vc4_drv.h  |  2 ++
 drivers/gpu/drm/vc4/vc4_kms.c  | 22 ++++++++++++++++------
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 7ef20adedee5..482219fb4db2 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc)
 		return;
 	}
 
+	vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 	__drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
 }
 
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 8c8d96b6289f..90b911fd2a7f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -532,6 +532,8 @@ struct vc4_crtc_state {
 	} margins;
 };
 
+#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1)
+
 static inline struct vc4_crtc_state *
 to_vc4_crtc_state(struct drm_crtc_state *crtc_state)
 {
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 01fa60844695..149825ff5df8 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -616,7 +616,7 @@ static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i, ret;
 
@@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	 * modified.
 	 */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct drm_crtc_state *crtc_state;
+
 		if (!crtc->state->enable)
 			continue;
 
@@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 			return PTR_ERR(crtc_state);
 	}
 
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		struct vc4_crtc_state *vc4_crtc_state =
-			to_vc4_crtc_state(crtc_state);
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct vc4_crtc_state *new_vc4_crtc_state =
+			to_vc4_crtc_state(new_crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
 
-		if (!crtc_state->enable)
+		if (old_crtc_state->enable && !new_crtc_state->enable)
+			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
+
+		if (!new_crtc_state->enable)
 			continue;
 
+		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) {
+			unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel);
+			continue;
+		}
+
 		/*
 		 * The problem we have to solve here is that we have
 		 * up to 7 encoders, connected to up to 6 CRTCs.
@@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 		if (matching_channels) {
 			unsigned int channel = ffs(matching_channels) - 1;
 
-			vc4_crtc_state->assigned_channel = channel;
+			new_vc4_crtc_state->assigned_channel = channel;
 			unassigned_channels &= ~BIT(channel);
 		} else {
 			return -EINVAL;
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO

[Maxime Ripard]

The HVS FIFOs are currently assigned each time we have an atomic_check
for all the enabled CRTCs.

However, if we are running multiple outputs in parallel and we happen to
disable the first (by index) CRTC, we end up changing the assigned FIFO
of the second CRTC without disabling and reenabling the pixelvalve which
ends up in a stall and eventually a VBLANK timeout.

In order to fix this, we can create a special value for our assigned
channel to mark it as disabled, and if our CRTC already had an assigned
channel in its previous state, we keep on using it.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

---

Changes from v1:
  - Split away the crtc state reset refactoring
  - Fixed the checkpatch warnings
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  1 +
 drivers/gpu/drm/vc4/vc4_drv.h  |  2 ++
 drivers/gpu/drm/vc4/vc4_kms.c  | 22 ++++++++++++++++------
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 7ef20adedee5..482219fb4db2 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc)
 		return;
 	}
 
+	vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 	__drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
 }
 
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 8c8d96b6289f..90b911fd2a7f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -532,6 +532,8 @@ struct vc4_crtc_state {
 	} margins;
 };
 
+#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1)
+
 static inline struct vc4_crtc_state *
 to_vc4_crtc_state(struct drm_crtc_state *crtc_state)
 {
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 01fa60844695..149825ff5df8 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -616,7 +616,7 @@ static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i, ret;
 
@@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	 * modified.
 	 */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct drm_crtc_state *crtc_state;
+
 		if (!crtc->state->enable)
 			continue;
 
@@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 			return PTR_ERR(crtc_state);
 	}
 
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		struct vc4_crtc_state *vc4_crtc_state =
-			to_vc4_crtc_state(crtc_state);
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct vc4_crtc_state *new_vc4_crtc_state =
+			to_vc4_crtc_state(new_crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
 
-		if (!crtc_state->enable)
+		if (old_crtc_state->enable && !new_crtc_state->enable)
+			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
+
+		if (!new_crtc_state->enable)
 			continue;
 
+		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) {
+			unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel);
+			continue;
+		}
+
 		/*
 		 * The problem we have to solve here is that we have
 		 * up to 7 encoders, connected to up to 6 CRTCs.
@@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 		if (matching_channels) {
 			unsigned int channel = ffs(matching_channels) - 1;
 
-			vc4_crtc_state->assigned_channel = channel;
+			new_vc4_crtc_state->assigned_channel = channel;
 			unassigned_channels &= ~BIT(channel);
 		} else {
 			return -EINVAL;
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Dave Stevenson]

Hi Maxime

On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> embeds drm_crtc_state as its first member, and therefore can be safely
> casted.

s/casted/cast

> However, this is pretty fragile especially since there's no check for this
> in place, and we're going to need to access vc4_crtc_state member at reset
> so this looks like a good occasion to make it more robust.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Based on the issue I perceived with the previous patch, I'm happy. I
haven't thought about how the framework handles losing the state, but
that's not the driver's problem.

There is still an implicit assumption that drm_crtc_state is the first
member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
make it even more robust that could be a container_of instead. I
haven't checked for any other places that make the assumption though.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>
> Changes from v1:
>   - new patch
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index a393f93390a2..7ef20adedee5 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>
>  void vc4_crtc_reset(struct drm_crtc *crtc)
>  {
> +       struct vc4_crtc_state *vc4_crtc_state;
> +
>         if (crtc->state)
>                 vc4_crtc_destroy_state(crtc, crtc->state);
> -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> -       if (crtc->state)
> -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> +
> +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> +       if (!vc4_crtc_state) {
> +               crtc->state = NULL;
> +               return;
> +       }
> +
> +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
>  }
>
>  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> --
> 2.26.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Dave Stevenson]

Hi Maxime

On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> embeds drm_crtc_state as its first member, and therefore can be safely
> casted.

s/casted/cast

> However, this is pretty fragile especially since there's no check for this
> in place, and we're going to need to access vc4_crtc_state member at reset
> so this looks like a good occasion to make it more robust.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Based on the issue I perceived with the previous patch, I'm happy. I
haven't thought about how the framework handles losing the state, but
that's not the driver's problem.

There is still an implicit assumption that drm_crtc_state is the first
member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
make it even more robust that could be a container_of instead. I
haven't checked for any other places that make the assumption though.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>
> Changes from v1:
>   - new patch
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index a393f93390a2..7ef20adedee5 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>
>  void vc4_crtc_reset(struct drm_crtc *crtc)
>  {
> +       struct vc4_crtc_state *vc4_crtc_state;
> +
>         if (crtc->state)
>                 vc4_crtc_destroy_state(crtc, crtc->state);
> -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> -       if (crtc->state)
> -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> +
> +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> +       if (!vc4_crtc_state) {
> +               crtc->state = NULL;
> +               return;
> +       }
> +
> +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
>  }
>
>  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> --
> 2.26.2
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Daniel Vetter]

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.

Also your next atomic_check will probably look at this, most definitely in
the memcpy to take over the old state. So yeah KASAN should complain if
you get this wrong :-)

Probably want a Fixes: line for this too.
-Daniel

> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > ---
> >
> > Changes from v1:
> >   - new patch
> > ---
> >  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > index a393f93390a2..7ef20adedee5 100644
> > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
> >
> >  void vc4_crtc_reset(struct drm_crtc *crtc)
> >  {
> > +       struct vc4_crtc_state *vc4_crtc_state;
> > +
> >         if (crtc->state)
> >                 vc4_crtc_destroy_state(crtc, crtc->state);
> > -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> > -       if (crtc->state)
> > -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> > +
> > +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> > +       if (!vc4_crtc_state) {
> > +               crtc->state = NULL;
> > +               return;
> > +       }
> > +
> > +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
> >  }
> >
> >  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> > --
> > 2.26.2
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Daniel Vetter]

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.

Also your next atomic_check will probably look at this, most definitely in
the memcpy to take over the old state. So yeah KASAN should complain if
you get this wrong :-)

Probably want a Fixes: line for this too.
-Daniel

> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > ---
> >
> > Changes from v1:
> >   - new patch
> > ---
> >  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > index a393f93390a2..7ef20adedee5 100644
> > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
> >
> >  void vc4_crtc_reset(struct drm_crtc *crtc)
> >  {
> > +       struct vc4_crtc_state *vc4_crtc_state;
> > +
> >         if (crtc->state)
> >                 vc4_crtc_destroy_state(crtc, crtc->state);
> > -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> > -       if (crtc->state)
> > -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> > +
> > +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> > +       if (!vc4_crtc_state) {
> > +               crtc->state = NULL;
> > +               return;
> > +       }
> > +
> > +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
> >  }
> >
> >  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> > --
> > 2.26.2
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Maxime Ripard]

Hi Dave,

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.

Good catch, I'll send another patch to fix it

> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Does it apply to the second patch as well?

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Maxime Ripard]

Hi Dave,

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.

Good catch, I'll send another patch to fix it

> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Does it apply to the second patch as well?

Thanks!
Maxime
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Dave Stevenson]

On Fri, 25 Sep 2020 at 12:38, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Dave,
>
> On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> > Hi Maxime
> >
> > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > > embeds drm_crtc_state as its first member, and therefore can be safely
> > > casted.
> >
> > s/casted/cast
> >
> > > However, this is pretty fragile especially since there's no check for this
> > > in place, and we're going to need to access vc4_crtc_state member at reset
> > > so this looks like a good occasion to make it more robust.
> > >
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Based on the issue I perceived with the previous patch, I'm happy. I
> > haven't thought about how the framework handles losing the state, but
> > that's not the driver's problem.
> >
> > There is still an implicit assumption that drm_crtc_state is the first
> > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> > make it even more robust that could be a container_of instead. I
> > haven't checked for any other places that make the assumption though.
>
> Good catch, I'll send another patch to fix it
>
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Does it apply to the second patch as well?

No. I got another interrupt before I'd refreshed my memory over what
the second patch was doing and responding to it. I'll do that now (I
don't think it changed much from v1)

  Dave.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Dave Stevenson]

On Fri, 25 Sep 2020 at 12:38, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Dave,
>
> On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> > Hi Maxime
> >
> > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > > embeds drm_crtc_state as its first member, and therefore can be safely
> > > casted.
> >
> > s/casted/cast
> >
> > > However, this is pretty fragile especially since there's no check for this
> > > in place, and we're going to need to access vc4_crtc_state member at reset
> > > so this looks like a good occasion to make it more robust.
> > >
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Based on the issue I perceived with the previous patch, I'm happy. I
> > haven't thought about how the framework handles losing the state, but
> > that's not the driver's problem.
> >
> > There is still an implicit assumption that drm_crtc_state is the first
> > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> > make it even more robust that could be a container_of instead. I
> > haven't checked for any other places that make the assumption though.
>
> Good catch, I'll send another patch to fix it
>
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Does it apply to the second patch as well?

No. I got another interrupt before I'd refreshed my memory over what
the second patch was doing and responding to it. I'll do that now (I
don't think it changed much from v1)

  Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO

[Dave Stevenson]

Hi Maxime

Sorry for the delay.

On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The HVS FIFOs are currently assigned each time we have an atomic_check
> for all the enabled CRTCs.
>
> However, if we are running multiple outputs in parallel and we happen to
> disable the first (by index) CRTC, we end up changing the assigned FIFO
> of the second CRTC without disabling and reenabling the pixelvalve which
> ends up in a stall and eventually a VBLANK timeout.
>
> In order to fix this, we can create a special value for our assigned
> channel to mark it as disabled, and if our CRTC already had an assigned
> channel in its previous state, we keep on using it.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I retested too and had triple output (HDMI-0, HDMI-1, and DPI) working
happily! Switching around resolutions on the HDMI outputs was working
fine. DPI was an Adafruit Kippah and 800x480 LCD, so no options on
resolution.
We may finally have this muxing nailed.

  Dave

> ---
>
> Changes from v1:
>   - Split away the crtc state reset refactoring
>   - Fixed the checkpatch warnings
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  1 +
>  drivers/gpu/drm/vc4/vc4_drv.h  |  2 ++
>  drivers/gpu/drm/vc4/vc4_kms.c  | 22 ++++++++++++++++------
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 7ef20adedee5..482219fb4db2 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc)
>                 return;
>         }
>
> +       vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>         __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
>  }
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 8c8d96b6289f..90b911fd2a7f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -532,6 +532,8 @@ struct vc4_crtc_state {
>         } margins;
>  };
>
> +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1)
> +
>  static inline struct vc4_crtc_state *
>  to_vc4_crtc_state(struct drm_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 01fa60844695..149825ff5df8 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -616,7 +616,7 @@ static int
>  vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>         unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
> -       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>         struct drm_crtc *crtc;
>         int i, ret;
>
> @@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>          * modified.
>          */
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +               struct drm_crtc_state *crtc_state;
> +
>                 if (!crtc->state->enable)
>                         continue;
>
> @@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>                         return PTR_ERR(crtc_state);
>         }
>
> -       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -               struct vc4_crtc_state *vc4_crtc_state =
> -                       to_vc4_crtc_state(crtc_state);
> +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +               struct vc4_crtc_state *new_vc4_crtc_state =
> +                       to_vc4_crtc_state(new_crtc_state);
>                 struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>                 unsigned int matching_channels;
>
> -               if (!crtc_state->enable)
> +               if (old_crtc_state->enable && !new_crtc_state->enable)
> +                       new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +
> +               if (!new_crtc_state->enable)
>                         continue;
>
> +               if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) {
> +                       unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel);
> +                       continue;
> +               }
> +
>                 /*
>                  * The problem we have to solve here is that we have
>                  * up to 7 encoders, connected to up to 6 CRTCs.
> @@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>                 if (matching_channels) {
>                         unsigned int channel = ffs(matching_channels) - 1;
>
> -                       vc4_crtc_state->assigned_channel = channel;
> +                       new_vc4_crtc_state->assigned_channel = channel;
>                         unassigned_channels &= ~BIT(channel);
>                 } else {
>                         return -EINVAL;
> --
> 2.26.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO

[Dave Stevenson]

Hi Maxime

Sorry for the delay.

On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The HVS FIFOs are currently assigned each time we have an atomic_check
> for all the enabled CRTCs.
>
> However, if we are running multiple outputs in parallel and we happen to
> disable the first (by index) CRTC, we end up changing the assigned FIFO
> of the second CRTC without disabling and reenabling the pixelvalve which
> ends up in a stall and eventually a VBLANK timeout.
>
> In order to fix this, we can create a special value for our assigned
> channel to mark it as disabled, and if our CRTC already had an assigned
> channel in its previous state, we keep on using it.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I retested too and had triple output (HDMI-0, HDMI-1, and DPI) working
happily! Switching around resolutions on the HDMI outputs was working
fine. DPI was an Adafruit Kippah and 800x480 LCD, so no options on
resolution.
We may finally have this muxing nailed.

  Dave

> ---
>
> Changes from v1:
>   - Split away the crtc state reset refactoring
>   - Fixed the checkpatch warnings
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  1 +
>  drivers/gpu/drm/vc4/vc4_drv.h  |  2 ++
>  drivers/gpu/drm/vc4/vc4_kms.c  | 22 ++++++++++++++++------
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 7ef20adedee5..482219fb4db2 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc)
>                 return;
>         }
>
> +       vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>         __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
>  }
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 8c8d96b6289f..90b911fd2a7f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -532,6 +532,8 @@ struct vc4_crtc_state {
>         } margins;
>  };
>
> +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1)
> +
>  static inline struct vc4_crtc_state *
>  to_vc4_crtc_state(struct drm_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 01fa60844695..149825ff5df8 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -616,7 +616,7 @@ static int
>  vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>         unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
> -       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>         struct drm_crtc *crtc;
>         int i, ret;
>
> @@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>          * modified.
>          */
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +               struct drm_crtc_state *crtc_state;
> +
>                 if (!crtc->state->enable)
>                         continue;
>
> @@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>                         return PTR_ERR(crtc_state);
>         }
>
> -       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -               struct vc4_crtc_state *vc4_crtc_state =
> -                       to_vc4_crtc_state(crtc_state);
> +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +               struct vc4_crtc_state *new_vc4_crtc_state =
> +                       to_vc4_crtc_state(new_crtc_state);
>                 struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>                 unsigned int matching_channels;
>
> -               if (!crtc_state->enable)
> +               if (old_crtc_state->enable && !new_crtc_state->enable)
> +                       new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +
> +               if (!new_crtc_state->enable)
>                         continue;
>
> +               if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) {
> +                       unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel);
> +                       continue;
> +               }
> +
>                 /*
>                  * The problem we have to solve here is that we have
>                  * up to 7 encoders, connected to up to 6 CRTCs.
> @@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>                 if (matching_channels) {
>                         unsigned int channel = ffs(matching_channels) - 1;
>
> -                       vc4_crtc_state->assigned_channel = channel;
> +                       new_vc4_crtc_state->assigned_channel = channel;
>                         unassigned_channels &= ~BIT(channel);
>                 } else {
>                         return -EINVAL;
> --
> 2.26.2
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --]

Hi,

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Applied 1 and 2, with the typo fixed (and the fixes tag suggested by Daniel)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --]

Hi,

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Applied 1 and 2, with the typo fixed (and the fixes tag suggested by Daniel)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel