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

[Laurent Pinchart]

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 
>  Cc: Laurent Pinchart 
>  Cc: Kieran Bingham 
>  Cc: linux-renesas-...@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 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 : 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->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().

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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 
 Cc: Laurent Pinchart 
 Cc: Kieran Bingham 
 Cc: linux-renesas-...@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 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 : 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().
>

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

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

[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 
> >> Cc: Laurent Pinchart 
> >> Cc: Kieran Bingham 
> >> Cc: linux-renesas-...@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 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().

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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 
>> Cc: Laurent Pinchart 
>> Cc: Kieran Bingham 
>> Cc: linux-renesas-...@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 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

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

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

[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 
> Cc: Laurent Pinchart 
> Cc: Kieran Bingham 
> Cc: linux-renesas-...@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 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;
> - crtc->state->crtc = crtc;
>  }
>  
>  static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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 
Cc: Laurent Pinchart 
Cc: Kieran Bingham 
Cc: linux-renesas-...@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);
if (state == NULL)
return;
 
state->crc.source = VSP1_DU_CRC_NONE;
state->crc.index = 0;
-
-   crtc->state = >state;
-   crtc->state->crtc = crtc;
 }
 
 static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
-- 
2.20.1

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