Re: [PATCH] drm: rcar-du: Make sure DRM planes are created by VSP1

[Laurent Pinchart]

Hi Jacopo,

On Friday 03 Mar 2017 13:37:56 jacopo mondi wrote:
> On 03/03/2017 12:26, Laurent Pinchart wrote:
> > On Friday 03 Mar 2017 09:09:38 Jacopo Mondi wrote:
> >> On Gen3 platforms compositing planes are allocated  by VSP on behalf of
> >> DRM/KMS. If VSP support is not compiled in, vsp1 initialization stub
> >> routine is called, leading to invalid memory accesses later on when un-
> >> initialized planes are accessed.
> >> 
> >> Fail explicitly earlier if planes are not properly created.
> >> 
> >> Signed-off-by: Jacopo Mondi 
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index b5d3f16..7f56c09 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> @@ -616,6 +616,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
> >>ret = rcar_du_vsp_init(vsp);
> >>if (ret < 0)
> >>return ret;
> >> +
> >> +  if (!vsp->planes)
> >> +  return -EINVAL;
> > 
> > Given that this code is only called when the DU hardware requires the VSP
> > to operate, how about modifying the rcar_du_vsp_init() stub function to
> > return - ENXIO instead of 0 ? That way you won't need to add an
> > additional check here.
>
> Ok, I'll make sure it is only called there also.
> 
> > Ideally we should require VSP through Kconfig as well. If you feel like
> > submitting a patch (and testing it with various combinations of module and
> > built-in) it would be appreciated.
> 
> That would be ideal.
> Am I wrong or the condition that selects DRM_RCAR_VSP is (DRM_RCAR_DU &&
> Gen3-platform)? Implying that on Gen2 DU can live without VSP enabled?

That's correct. On Gen2 VSP isn't required to operate the display.

> >>}
> >>
> >>}

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Make sure DRM planes are created by VSP1

[jacopo mondi]

HI Laurent,

On 03/03/2017 12:26, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Friday 03 Mar 2017 09:09:38 Jacopo Mondi wrote:

On Gen3 platforms compositing planes are allocated  by VSP on behalf of
DRM/KMS.
If VSP support is not compiled in, vsp1 initialization stub routine is
called, leading to invalid memory accesses later on when un-initialized
planes are accessed.

Fail explicitly earlier if planes are not properly created.

Signed-off-by: Jacopo Mondi 
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index b5d3f16..7f56c09 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -616,6 +616,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
ret = rcar_du_vsp_init(vsp);
if (ret < 0)
return ret;
+
+   if (!vsp->planes)
+   return -EINVAL;


Given that this code is only called when the DU hardware requires the VSP to
operate, how about modifying the rcar_du_vsp_init() stub function to return -
ENXIO instead of 0 ? That way you won't need to add an additional check here.



Ok, I'll make sure it is only called there also.


Ideally we should require VSP through Kconfig as well. If you feel like
submitting a patch (and testing it with various combinations of module and
built-in) it would be appreciated.


That would be ideal.
Am I wrong or the condition that selects DRM_RCAR_VSP is (DRM_RCAR_DU && 
Gen3-platform)? Implying that on Gen2 DU can live without VSP enabled?


Thanks
   j





}
}

Re: [PATCH] drm: rcar-du: Make sure DRM planes are created by VSP1

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday 03 Mar 2017 09:09:38 Jacopo Mondi wrote:
> On Gen3 platforms compositing planes are allocated  by VSP on behalf of
> DRM/KMS.
> If VSP support is not compiled in, vsp1 initialization stub routine is
> called, leading to invalid memory accesses later on when un-initialized
> planes are accessed.
> 
> Fail explicitly earlier if planes are not properly created.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index b5d3f16..7f56c09 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -616,6 +616,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>   ret = rcar_du_vsp_init(vsp);
>   if (ret < 0)
>   return ret;
> +
> + if (!vsp->planes)
> + return -EINVAL;

Given that this code is only called when the DU hardware requires the VSP to 
operate, how about modifying the rcar_du_vsp_init() stub function to return -
ENXIO instead of 0 ? That way you won't need to add an additional check here.

Ideally we should require VSP through Kconfig as well. If you feel like 
submitting a patch (and testing it with various combinations of module and 
built-in) it would be appreciated.

>   }
>   }

-- 
Regards,

Laurent Pinchart

[PATCH] drm: rcar-du: Make sure DRM planes are created by VSP1

[Jacopo Mondi]

On Gen3 platforms compositing planes are allocated  by VSP on behalf of
DRM/KMS.
If VSP support is not compiled in, vsp1 initialization stub routine is
called, leading to invalid memory accesses later on when un-initialized
planes are accessed.

Fail explicitly earlier if planes are not properly created.

Signed-off-by: Jacopo Mondi 
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c 
b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index b5d3f16..7f56c09 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -616,6 +616,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
ret = rcar_du_vsp_init(vsp);
if (ret < 0)
return ret;
+
+   if (!vsp->planes)
+   return -EINVAL;
}
}
 
-- 
2.7.4