diff mbox series

[12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

Message ID 20190301125627.7285-13-maarten.lankhorst@linux.intel.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series None | expand

Commit Message

Maarten Lankhorst March 1, 2019, 12:56 p.m. UTC
Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding destroy_state(), call
it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart March 1, 2019, 1:13 p.m. UTC | #1
Hi Maarten,

Thank you for the patch.

On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(), call
> it directly for freeing the old state.

I don't think the second sentence applies to this patch.

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 4cdea14d552f..7766551e67fc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
>  
>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>  {
> -	struct rcar_du_crtc_state *state;
> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>  
> -	if (crtc->state) {
> +	if (crtc->state)
>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> -		crtc->state = NULL;
> -	}
>  
> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);

state may be NULL here if the above kzalloc() failed. Let's keep the
original order of the function, and simply call
__drm_atomic_helper_crtc_reset() after the NULL check below.

>  	if (state == NULL)
>  		return;
>  
>  	state->crc.source = VSP1_DU_CRC_NONE;
>  	state->crc.index = 0;
> -
> -	crtc->state = &state->state;
> -	crtc->state->crtc = crtc;
>  }
>  
>  static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
Maarten Lankhorst March 1, 2019, 2:08 p.m. UTC | #2
Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> Hi Maarten,
>
> Thank you for the patch.
>
> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
>> writing its own version. Instead of open coding destroy_state(), call
>> it directly for freeing the old state.
> I don't think the second sentence applies to this patch.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index 4cdea14d552f..7766551e67fc 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
>>  
>>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>>  {
>> -	struct rcar_du_crtc_state *state;
>> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>>  
>> -	if (crtc->state) {
>> +	if (crtc->state)
>>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
>> -		crtc->state = NULL;
>> -	}
>>  
>> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
>> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> state may be NULL here if the above kzalloc() failed. Let's keep the
> original order of the function, and simply call
> __drm_atomic_helper_crtc_reset() after the NULL check below.

There were 10 different ways crtc was implemented, I felt it was good to settle on one.

We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.

Looking more closely, it's the same way that errors in rcar_du_plane_reset() are handled. :)

Cheers,

~Maarten
Laurent Pinchart March 1, 2019, 2:36 p.m. UTC | #3
Hi Marteen,

On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> > On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> >> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> >> writing its own version. Instead of open coding destroy_state(), call
> >> it directly for freeing the old state.
> > I don't think the second sentence applies to this patch.
> >
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> ---
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
> >>  1 file changed, 3 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> index 4cdea14d552f..7766551e67fc 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
> >>  
> >>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> >>  {
> >> -	struct rcar_du_crtc_state *state;
> >> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>  
> >> -	if (crtc->state) {
> >> +	if (crtc->state)
> >>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> >> -		crtc->state = NULL;
> >> -	}
> >>  
> >> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> >> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> > 
> > state may be NULL here if the above kzalloc() failed. Let's keep the
> > original order of the function, and simply call
> > __drm_atomic_helper_crtc_reset() after the NULL check below.
> 
> There were 10 different ways crtc was implemented, I felt it was good to settle on one.
> 
> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.

That's not the point. As state can be NULL, you could end up
dereferencing a NULL pointer. The fact that the base state is the first
field in the rcar_du_crtc_state structure is just luck, and shouldn't be
relied on.

> Looking more closely, it's the same way that errors in
> rcar_du_plane_reset() are handled. :)

It's not, the return value of kzalloc() is checked explicitly in
rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
Please copy the code flow of rcar_du_plane_reset() to implement
rcar_du_crtc_reset().
Maarten Lankhorst March 1, 2019, 2:47 p.m. UTC | #4
Op 01-03-2019 om 15:36 schreef Laurent Pinchart:
> Hi Marteen,
>
> On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
>> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
>>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
>>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
>>>> writing its own version. Instead of open coding destroy_state(), call
>>>> it directly for freeing the old state.
>>> I don't think the second sentence applies to this patch.
>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>> ---
>>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
>>>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> index 4cdea14d552f..7766551e67fc 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
>>>>  
>>>>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>>>>  {
>>>> -	struct rcar_du_crtc_state *state;
>>>> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>>>>  
>>>> -	if (crtc->state) {
>>>> +	if (crtc->state)
>>>>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
>>>> -		crtc->state = NULL;
>>>> -	}
>>>>  
>>>> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
>>>> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
>>> state may be NULL here if the above kzalloc() failed. Let's keep the
>>> original order of the function, and simply call
>>> __drm_atomic_helper_crtc_reset() after the NULL check below.
>> There were 10 different ways crtc was implemented, I felt it was good to settle on one.
>>
>> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.
> That's not the point. As state can be NULL, you could end up
> dereferencing a NULL pointer. The fact that the base state is the first
> field in the rcar_du_crtc_state structure is just luck, and shouldn't be
> relied on.

Would it be ok if I changed it to state ? &state->state : NULL and let the compiler deal with it?

Will probably fix up all other patches as well before committing.

>> Looking more closely, it's the same way that errors in
>> rcar_du_plane_reset() are handled. :)
> It's not, the return value of kzalloc() is checked explicitly in
> rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
> Please copy the code flow of rcar_du_plane_reset() to implement
> rcar_du_crtc_reset().
>
Laurent Pinchart March 1, 2019, 3:06 p.m. UTC | #5
Hi Marteen,

On Fri, Mar 01, 2019 at 03:47:02PM +0100, Maarten Lankhorst wrote:
> Op 01-03-2019 om 15:36 schreef Laurent Pinchart:
> > On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
> >> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> >>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> >>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> >>>> writing its own version. Instead of open coding destroy_state(), call
> >>>> it directly for freeing the old state.
> >>> I don't think the second sentence applies to this patch.
> >>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>> Cc: linux-renesas-soc@vger.kernel.org
> >>>> ---
> >>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
> >>>>  1 file changed, 3 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> index 4cdea14d552f..7766551e67fc 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
> >>>>  
> >>>>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> >>>>  {
> >>>> -	struct rcar_du_crtc_state *state;
> >>>> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>>>  
> >>>> -	if (crtc->state) {
> >>>> +	if (crtc->state)
> >>>>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> >>>> -		crtc->state = NULL;
> >>>> -	}
> >>>>  
> >>>> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>>> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> >>> state may be NULL here if the above kzalloc() failed. Let's keep the
> >>> original order of the function, and simply call
> >>> __drm_atomic_helper_crtc_reset() after the NULL check below.
> >> There were 10 different ways crtc was implemented, I felt it was good to settle on one.
> >>
> >> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.
> > That's not the point. As state can be NULL, you could end up
> > dereferencing a NULL pointer. The fact that the base state is the first
> > field in the rcar_du_crtc_state structure is just luck, and shouldn't be
> > relied on.
> 
> Would it be ok if I changed it to state ? &state->state : NULL and let
> the compiler deal with it?

What's wrong with a proper implementation ?

static void rcar_du_crtc_reset(struct drm_crtc *crtc)
{
	struct rcar_du_crtc_state *state;

	if (crtc->state) {
		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
		crtc->state = NULL;
	}

	state = kzalloc(sizeof(*state), GFP_KERNEL);
	if (state == NULL)
		return;

	__drm_atomic_helper_crtc_reset(crtc, &state->state);

	state->crc.source = VSP1_DU_CRC_NONE;
	state->crc.index = 0;
}

> Will probably fix up all other patches as well before committing.

You won't commit this one before I ack it, right ? :-)

> >> Looking more closely, it's the same way that errors in
> >> rcar_du_plane_reset() are handled. :)
> > It's not, the return value of kzalloc() is checked explicitly in
> > rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
> > Please copy the code flow of rcar_du_plane_reset() to implement
> > rcar_du_crtc_reset().
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 4cdea14d552f..7766551e67fc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -891,22 +891,17 @@  static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
 
 static void rcar_du_crtc_reset(struct drm_crtc *crtc)
 {
-	struct rcar_du_crtc_state *state;
+	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
 
-	if (crtc->state) {
+	if (crtc->state)
 		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
-		crtc->state = NULL;
-	}
 
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	__drm_atomic_helper_crtc_reset(crtc, &state->state);
 	if (state == NULL)
 		return;
 
 	state->crc.source = VSP1_DU_CRC_NONE;
 	state->crc.index = 0;
-
-	crtc->state = &state->state;
-	crtc->state->crtc = crtc;
 }
 
 static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)