[PATCH 0/2] Follow-up patches for Cedrus v9

[PATCH 0/2] Follow-up patches for Cedrus v9

[Paul Kocialkowski]

This brings the requested modifications on top of version 9 of the
Cedrus VPU driver, that implements stateless video decoding using the
Request API.

Paul Kocialkowski (2):
  media: cedrus: Fix error reporting in request validation
  media: cedrus: Add TODO file with tasks to complete before unstaging

 drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
 drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100644 drivers/staging/media/sunxi/cedrus/TODO

-- 
2.18.0

[PATCH 0/2] Follow-up patches for Cedrus v9

[Paul Kocialkowski]

This brings the requested modifications on top of version 9 of the
Cedrus VPU driver, that implements stateless video decoding using the
Request API.

Paul Kocialkowski (2):
  media: cedrus: Fix error reporting in request validation
  media: cedrus: Add TODO file with tasks to complete before unstaging

 drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
 drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100644 drivers/staging/media/sunxi/cedrus/TODO

-- 
2.18.0

[PATCH 1/2] media: cedrus: Fix error reporting in request validation

[Paul Kocialkowski]

This fixes error reporting by using the appropriate logging helpers and
return codes, while introducing new messages when there are not enough
or too many buffers associated with the request.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 09ab1b732c31..0a9363c7db06 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -105,10 +105,19 @@ static int cedrus_request_validate(struct media_request *req)
 	struct v4l2_ctrl_handler *parent_hdl, *hdl;
 	struct cedrus_ctx *ctx = NULL;
 	struct v4l2_ctrl *ctrl_test;
+	unsigned int count;
 	unsigned int i;
 
-	if (vb2_request_buffer_cnt(req) != 1)
+	count = vb2_request_buffer_cnt(req);
+	if (!count) {
+		v4l2_info(&ctx->dev->v4l2_dev,
+			  "No buffer was provided with the request\n");
 		return -ENOENT;
+	} else if (count > 1) {
+		v4l2_info(&ctx->dev->v4l2_dev,
+			  "More than one buffer was provided with the request\n");
+		return -EINVAL;
+	}
 
 	list_for_each_entry(obj, &req->objects, list) {
 		struct vb2_buffer *vb;
@@ -128,7 +137,7 @@ static int cedrus_request_validate(struct media_request *req)
 
 	hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
 	if (!hdl) {
-		v4l2_err(&ctx->dev->v4l2_dev, "Missing codec control(s)\n");
+		v4l2_info(&ctx->dev->v4l2_dev, "Missing codec control(s)\n");
 		return -ENOENT;
 	}
 
@@ -140,7 +149,7 @@ static int cedrus_request_validate(struct media_request *req)
 		ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
 			cedrus_controls[i].id);
 		if (!ctrl_test) {
-			v4l2_err(&ctx->dev->v4l2_dev,
+			v4l2_info(&ctx->dev->v4l2_dev,
 				 "Missing required codec control\n");
 			return -ENOENT;
 		}
-- 
2.18.0

[PATCH 1/2] media: cedrus: Fix error reporting in request validation

[Paul Kocialkowski]

This fixes error reporting by using the appropriate logging helpers and
return codes, while introducing new messages when there are not enough
or too many buffers associated with the request.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 09ab1b732c31..0a9363c7db06 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -105,10 +105,19 @@ static int cedrus_request_validate(struct media_request *req)
 	struct v4l2_ctrl_handler *parent_hdl, *hdl;
 	struct cedrus_ctx *ctx = NULL;
 	struct v4l2_ctrl *ctrl_test;
+	unsigned int count;
 	unsigned int i;
 
-	if (vb2_request_buffer_cnt(req) != 1)
+	count = vb2_request_buffer_cnt(req);
+	if (!count) {
+		v4l2_info(&ctx->dev->v4l2_dev,
+			  "No buffer was provided with the request\n");
 		return -ENOENT;
+	} else if (count > 1) {
+		v4l2_info(&ctx->dev->v4l2_dev,
+			  "More than one buffer was provided with the request\n");
+		return -EINVAL;
+	}
 
 	list_for_each_entry(obj, &req->objects, list) {
 		struct vb2_buffer *vb;
@@ -128,7 +137,7 @@ static int cedrus_request_validate(struct media_request *req)
 
 	hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
 	if (!hdl) {
-		v4l2_err(&ctx->dev->v4l2_dev, "Missing codec control(s)\n");
+		v4l2_info(&ctx->dev->v4l2_dev, "Missing codec control(s)\n");
 		return -ENOENT;
 	}
 
@@ -140,7 +149,7 @@ static int cedrus_request_validate(struct media_request *req)
 		ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
 			cedrus_controls[i].id);
 		if (!ctrl_test) {
-			v4l2_err(&ctx->dev->v4l2_dev,
+			v4l2_info(&ctx->dev->v4l2_dev,
 				 "Missing required codec control\n");
 			return -ENOENT;
 		}
-- 
2.18.0

[PATCH 2/2] media: cedrus: Add TODO file with tasks to complete before unstaging

[Paul Kocialkowski]

When the elements listed are complete, the Cedrus driver will be ready
to move out of the staging area of the kernel.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/staging/media/sunxi/cedrus/TODO | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 drivers/staging/media/sunxi/cedrus/TODO

diff --git a/drivers/staging/media/sunxi/cedrus/TODO b/drivers/staging/media/sunxi/cedrus/TODO
new file mode 100644
index 000000000000..ec277ece47af
--- /dev/null
+++ b/drivers/staging/media/sunxi/cedrus/TODO
@@ -0,0 +1,7 @@
+Before this stateless decoder driver can leave the staging area:
+* The Request API needs to be stabilized;
+* The codec-specific controls need to be thoroughly reviewed to ensure they
+  cover all intended uses cases;
+* Userspace support for the Request API needs to be reviewed;
+* Another stateless decoder driver should be submitted;
+* At least one stateless encoder driver should be submitted.
-- 
2.18.0

[PATCH 2/2] media: cedrus: Add TODO file with tasks to complete before unstaging

[Paul Kocialkowski]

When the elements listed are complete, the Cedrus driver will be ready
to move out of the staging area of the kernel.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/staging/media/sunxi/cedrus/TODO | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 drivers/staging/media/sunxi/cedrus/TODO

diff --git a/drivers/staging/media/sunxi/cedrus/TODO b/drivers/staging/media/sunxi/cedrus/TODO
new file mode 100644
index 000000000000..ec277ece47af
--- /dev/null
+++ b/drivers/staging/media/sunxi/cedrus/TODO
@@ -0,0 +1,7 @@
+Before this stateless decoder driver can leave the staging area:
+* The Request API needs to be stabilized;
+* The codec-specific controls need to be thoroughly reviewed to ensure they
+  cover all intended uses cases;
+* Userspace support for the Request API needs to be reviewed;
+* Another stateless decoder driver should be submitted;
+* At least one stateless encoder driver should be submitted.
-- 
2.18.0

Re: [PATCH 0/2] Follow-up patches for Cedrus v9

[Hans Verkuil]

On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> This brings the requested modifications on top of version 9 of the
> Cedrus VPU driver, that implements stateless video decoding using the
> Request API.
> 
> Paul Kocialkowski (2):
>   media: cedrus: Fix error reporting in request validation
>   media: cedrus: Add TODO file with tasks to complete before unstaging
> 
>  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> 

So close...

When compiling under e.g. intel I get errors since it doesn't know about
the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.

Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.

The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
makes me wonder if this information shouldn't come from the device tree.

You are the only driver that uses this define directly, which makes me
suspicious.

Regards,

	Hans

[PATCH 0/2] Follow-up patches for Cedrus v9

[Hans Verkuil]

On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> This brings the requested modifications on top of version 9 of the
> Cedrus VPU driver, that implements stateless video decoding using the
> Request API.
> 
> Paul Kocialkowski (2):
>   media: cedrus: Fix error reporting in request validation
>   media: cedrus: Add TODO file with tasks to complete before unstaging
> 
>  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> 

So close...

When compiling under e.g. intel I get errors since it doesn't know about
the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.

Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.

The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
makes me wonder if this information shouldn't come from the device tree.

You are the only driver that uses this define directly, which makes me
suspicious.

Regards,

	Hans

Re: [PATCH 0/2] Follow-up patches for Cedrus v9

[Chen-Yu Tsai]

On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> > This brings the requested modifications on top of version 9 of the
> > Cedrus VPU driver, that implements stateless video decoding using the
> > Request API.
> >
> > Paul Kocialkowski (2):
> >   media: cedrus: Fix error reporting in request validation
> >   media: cedrus: Add TODO file with tasks to complete before unstaging
> >
> >  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
> >  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> >
>
> So close...
>
> When compiling under e.g. intel I get errors since it doesn't know about
> the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
>
> Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
> if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
>
> The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
> way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
> makes me wonder if this information shouldn't come from the device tree.
>
> You are the only driver that uses this define directly, which makes me
> suspicious.

On Allwinner platforms, some devices do DMA directly on the memory BUS
with the DRAM controller. In such cases, the DRAM has no offset. In all
other cases where the DMA goes through the common system bus and the DRAM
offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
former case is not described in the device tree (this is being worked on
by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
only the display and media subsystems (VPU, camera, TS) are wired this
way.

In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
PHYS_OFFSET, which is pretty much the same thing.

Regards
ChenYu

[PATCH 0/2] Follow-up patches for Cedrus v9

[Chen-Yu Tsai]

On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> > This brings the requested modifications on top of version 9 of the
> > Cedrus VPU driver, that implements stateless video decoding using the
> > Request API.
> >
> > Paul Kocialkowski (2):
> >   media: cedrus: Fix error reporting in request validation
> >   media: cedrus: Add TODO file with tasks to complete before unstaging
> >
> >  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
> >  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> >
>
> So close...
>
> When compiling under e.g. intel I get errors since it doesn't know about
> the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
>
> Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
> if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
>
> The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
> way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
> makes me wonder if this information shouldn't come from the device tree.
>
> You are the only driver that uses this define directly, which makes me
> suspicious.

On Allwinner platforms, some devices do DMA directly on the memory BUS
with the DRAM controller. In such cases, the DRAM has no offset. In all
other cases where the DMA goes through the common system bus and the DRAM
offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
former case is not described in the device tree (this is being worked on
by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
only the display and media subsystems (VPU, camera, TS) are wired this
way.

In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
PHYS_OFFSET, which is pretty much the same thing.

Regards
ChenYu

Re: [PATCH 0/2] Follow-up patches for Cedrus v9

[Hans Verkuil]

On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
> On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
>>> This brings the requested modifications on top of version 9 of the
>>> Cedrus VPU driver, that implements stateless video decoding using the
>>> Request API.
>>>
>>> Paul Kocialkowski (2):
>>>   media: cedrus: Fix error reporting in request validation
>>>   media: cedrus: Add TODO file with tasks to complete before unstaging
>>>
>>>  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
>>>  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
>>>  2 files changed, 19 insertions(+), 3 deletions(-)
>>>  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
>>>
>>
>> So close...
>>
>> When compiling under e.g. intel I get errors since it doesn't know about
>> the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
>>
>> Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
>> if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
>>
>> The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
>> way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
>> makes me wonder if this information shouldn't come from the device tree.
>>
>> You are the only driver that uses this define directly, which makes me
>> suspicious.
> 
> On Allwinner platforms, some devices do DMA directly on the memory BUS
> with the DRAM controller. In such cases, the DRAM has no offset. In all
> other cases where the DMA goes through the common system bus and the DRAM
> offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
> former case is not described in the device tree (this is being worked on
> by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
> only the display and media subsystems (VPU, camera, TS) are wired this
> way.
> 
> In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
> PHYS_OFFSET, which is pretty much the same thing.
>

OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
with a comment that this will eventually come from the device tree.

Regards,

	Hans

[PATCH 0/2] Follow-up patches for Cedrus v9

[Hans Verkuil]

On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
> On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
>>> This brings the requested modifications on top of version 9 of the
>>> Cedrus VPU driver, that implements stateless video decoding using the
>>> Request API.
>>>
>>> Paul Kocialkowski (2):
>>>   media: cedrus: Fix error reporting in request validation
>>>   media: cedrus: Add TODO file with tasks to complete before unstaging
>>>
>>>  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
>>>  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
>>>  2 files changed, 19 insertions(+), 3 deletions(-)
>>>  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
>>>
>>
>> So close...
>>
>> When compiling under e.g. intel I get errors since it doesn't know about
>> the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
>>
>> Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
>> if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
>>
>> The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
>> way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
>> makes me wonder if this information shouldn't come from the device tree.
>>
>> You are the only driver that uses this define directly, which makes me
>> suspicious.
> 
> On Allwinner platforms, some devices do DMA directly on the memory BUS
> with the DRAM controller. In such cases, the DRAM has no offset. In all
> other cases where the DMA goes through the common system bus and the DRAM
> offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
> former case is not described in the device tree (this is being worked on
> by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
> only the display and media subsystems (VPU, camera, TS) are wired this
> way.
> 
> In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
> PHYS_OFFSET, which is pretty much the same thing.
>

OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
with a comment that this will eventually come from the device tree.

Regards,

	Hans

Re: [PATCH 0/2] Follow-up patches for Cedrus v9

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]

Hi,

Le samedi 08 septembre 2018 à 13:24 +0200, Hans Verkuil a écrit :
> On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
> > On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > 
> > > On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> > > > This brings the requested modifications on top of version 9 of the
> > > > Cedrus VPU driver, that implements stateless video decoding using the
> > > > Request API.
> > > > 
> > > > Paul Kocialkowski (2):
> > > >   media: cedrus: Fix error reporting in request validation
> > > >   media: cedrus: Add TODO file with tasks to complete before unstaging
> > > > 
> > > >  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
> > > >  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
> > > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > >  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> > > > 
> > > 
> > > So close...
> > > 
> > > When compiling under e.g. intel I get errors since it doesn't know about
> > > the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
> > > 
> > > Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
> > > if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
> > > 
> > > The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
> > > way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
> > > makes me wonder if this information shouldn't come from the device tree.
> > > 
> > > You are the only driver that uses this define directly, which makes me
> > > suspicious.
> > 
> > On Allwinner platforms, some devices do DMA directly on the memory BUS
> > with the DRAM controller. In such cases, the DRAM has no offset. In all
> > other cases where the DMA goes through the common system bus and the DRAM
> > offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
> > former case is not described in the device tree (this is being worked on
> > by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
> > only the display and media subsystems (VPU, camera, TS) are wired this
> > way.
> > 
> > In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
> > PHYS_OFFSET, which is pretty much the same thing.
> > 
> 
> OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
> with a comment that this will eventually come from the device tree.

That seems fine, although I'm less certain about what to do for the
SRAM situation. Other drivers that use SUNXI_SRAM have a Kconfig select
on it (that Cedrus lacks). Provided that the SRAM driver builds fine
for non-sunxi configs, bringing-in that select would remove the need
for dummy functions and also ensure that the actual implementation is
always used on sunxi. Otherwise, there'd be a risk of having the dummy
functions used (if the SRAM driver is not explicitly selected in the
config), causing a hang when accessing the VPU.

What do you think?

Cheers,

Paul

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 0/2] Follow-up patches for Cedrus v9

[Paul Kocialkowski]

Hi,

Le samedi 08 septembre 2018 ? 13:24 +0200, Hans Verkuil a ?crit :
> On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
> > On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > 
> > > On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> > > > This brings the requested modifications on top of version 9 of the
> > > > Cedrus VPU driver, that implements stateless video decoding using the
> > > > Request API.
> > > > 
> > > > Paul Kocialkowski (2):
> > > >   media: cedrus: Fix error reporting in request validation
> > > >   media: cedrus: Add TODO file with tasks to complete before unstaging
> > > > 
> > > >  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
> > > >  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
> > > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > >  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> > > > 
> > > 
> > > So close...
> > > 
> > > When compiling under e.g. intel I get errors since it doesn't know about
> > > the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
> > > 
> > > Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
> > > if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
> > > 
> > > The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
> > > way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
> > > makes me wonder if this information shouldn't come from the device tree.
> > > 
> > > You are the only driver that uses this define directly, which makes me
> > > suspicious.
> > 
> > On Allwinner platforms, some devices do DMA directly on the memory BUS
> > with the DRAM controller. In such cases, the DRAM has no offset. In all
> > other cases where the DMA goes through the common system bus and the DRAM
> > offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
> > former case is not described in the device tree (this is being worked on
> > by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
> > only the display and media subsystems (VPU, camera, TS) are wired this
> > way.
> > 
> > In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
> > PHYS_OFFSET, which is pretty much the same thing.
> > 
> 
> OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
> with a comment that this will eventually come from the device tree.

That seems fine, although I'm less certain about what to do for the
SRAM situation. Other drivers that use SUNXI_SRAM have a Kconfig select
on it (that Cedrus lacks). Provided that the SRAM driver builds fine
for non-sunxi configs, bringing-in that select would remove the need
for dummy functions and also ensure that the actual implementation is
always used on sunxi. Otherwise, there'd be a risk of having the dummy
functions used (if the SRAM driver is not explicitly selected in the
config), causing a hang when accessing the VPU.

What do you think?

Cheers,

Paul

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180908/1e5e2ff5/attachment.sig>

Re: [PATCH 0/2] Follow-up patches for Cedrus v9

[Hans Verkuil]

On 09/08/2018 09:42 PM, Paul Kocialkowski wrote:
> Hi,
> 
> Le samedi 08 septembre 2018 à 13:24 +0200, Hans Verkuil a écrit :
>> On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
>>> On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
>>>>> This brings the requested modifications on top of version 9 of the
>>>>> Cedrus VPU driver, that implements stateless video decoding using the
>>>>> Request API.
>>>>>
>>>>> Paul Kocialkowski (2):
>>>>>   media: cedrus: Fix error reporting in request validation
>>>>>   media: cedrus: Add TODO file with tasks to complete before unstaging
>>>>>
>>>>>  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
>>>>>  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
>>>>>  2 files changed, 19 insertions(+), 3 deletions(-)
>>>>>  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
>>>>>
>>>>
>>>> So close...
>>>>
>>>> When compiling under e.g. intel I get errors since it doesn't know about
>>>> the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
>>>>
>>>> Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
>>>> if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
>>>>
>>>> The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
>>>> way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
>>>> makes me wonder if this information shouldn't come from the device tree.
>>>>
>>>> You are the only driver that uses this define directly, which makes me
>>>> suspicious.
>>>
>>> On Allwinner platforms, some devices do DMA directly on the memory BUS
>>> with the DRAM controller. In such cases, the DRAM has no offset. In all
>>> other cases where the DMA goes through the common system bus and the DRAM
>>> offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
>>> former case is not described in the device tree (this is being worked on
>>> by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
>>> only the display and media subsystems (VPU, camera, TS) are wired this
>>> way.
>>>
>>> In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
>>> PHYS_OFFSET, which is pretty much the same thing.
>>>
>>
>> OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
>> with a comment that this will eventually come from the device tree.
> 
> That seems fine, although I'm less certain about what to do for the
> SRAM situation. Other drivers that use SUNXI_SRAM have a Kconfig select
> on it (that Cedrus lacks). Provided that the SRAM driver builds fine
> for non-sunxi configs, bringing-in that select would remove the need
> for dummy functions and also ensure that the actual implementation is
> always used on sunxi. Otherwise, there'd be a risk of having the dummy
> functions used (if the SRAM driver is not explicitly selected in the
> config), causing a hang when accessing the VPU.

You should certainly select this kernel config.

But the real problem seems to to be drivers/soc/Makefile where it says:

obj-$(CONFIG_ARCH_SUNXI)       += sunxi/

I think this should be:

obj-y       += sunxi/

Now all compiles fine on i686 with the cedrus driver selecting SUNXI_SRAM.

Regards,

	Hans

[PATCH 0/2] Follow-up patches for Cedrus v9

[Hans Verkuil]

On 09/08/2018 09:42 PM, Paul Kocialkowski wrote:
> Hi,
> 
> Le samedi 08 septembre 2018 ? 13:24 +0200, Hans Verkuil a ?crit :
>> On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
>>> On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
>>>>> This brings the requested modifications on top of version 9 of the
>>>>> Cedrus VPU driver, that implements stateless video decoding using the
>>>>> Request API.
>>>>>
>>>>> Paul Kocialkowski (2):
>>>>>   media: cedrus: Fix error reporting in request validation
>>>>>   media: cedrus: Add TODO file with tasks to complete before unstaging
>>>>>
>>>>>  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
>>>>>  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
>>>>>  2 files changed, 19 insertions(+), 3 deletions(-)
>>>>>  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
>>>>>
>>>>
>>>> So close...
>>>>
>>>> When compiling under e.g. intel I get errors since it doesn't know about
>>>> the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
>>>>
>>>> Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
>>>> if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
>>>>
>>>> The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
>>>> way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
>>>> makes me wonder if this information shouldn't come from the device tree.
>>>>
>>>> You are the only driver that uses this define directly, which makes me
>>>> suspicious.
>>>
>>> On Allwinner platforms, some devices do DMA directly on the memory BUS
>>> with the DRAM controller. In such cases, the DRAM has no offset. In all
>>> other cases where the DMA goes through the common system bus and the DRAM
>>> offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
>>> former case is not described in the device tree (this is being worked on
>>> by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
>>> only the display and media subsystems (VPU, camera, TS) are wired this
>>> way.
>>>
>>> In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
>>> PHYS_OFFSET, which is pretty much the same thing.
>>>
>>
>> OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
>> with a comment that this will eventually come from the device tree.
> 
> That seems fine, although I'm less certain about what to do for the
> SRAM situation. Other drivers that use SUNXI_SRAM have a Kconfig select
> on it (that Cedrus lacks). Provided that the SRAM driver builds fine
> for non-sunxi configs, bringing-in that select would remove the need
> for dummy functions and also ensure that the actual implementation is
> always used on sunxi. Otherwise, there'd be a risk of having the dummy
> functions used (if the SRAM driver is not explicitly selected in the
> config), causing a hang when accessing the VPU.

You should certainly select this kernel config.

But the real problem seems to to be drivers/soc/Makefile where it says:

obj-$(CONFIG_ARCH_SUNXI)       += sunxi/

I think this should be:

obj-y       += sunxi/

Now all compiles fine on i686 with the cedrus driver selecting SUNXI_SRAM.

Regards,

	Hans

Re: [PATCH 0/2] Follow-up patches for Cedrus v9

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 4301 bytes --]

Hi,

Le dimanche 09 septembre 2018 à 11:04 +0200, Hans Verkuil a écrit :
> On 09/08/2018 09:42 PM, Paul Kocialkowski wrote:
> > Hi,
> > 
> > Le samedi 08 septembre 2018 à 13:24 +0200, Hans Verkuil a écrit :
> > > On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
> > > > On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > 
> > > > > On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> > > > > > This brings the requested modifications on top of version 9 of the
> > > > > > Cedrus VPU driver, that implements stateless video decoding using the
> > > > > > Request API.
> > > > > > 
> > > > > > Paul Kocialkowski (2):
> > > > > >   media: cedrus: Fix error reporting in request validation
> > > > > >   media: cedrus: Add TODO file with tasks to complete before unstaging
> > > > > > 
> > > > > >  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
> > > > > >  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
> > > > > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > > > >  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> > > > > > 
> > > > > 
> > > > > So close...
> > > > > 
> > > > > When compiling under e.g. intel I get errors since it doesn't know about
> > > > > the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
> > > > > 
> > > > > Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
> > > > > if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
> > > > > 
> > > > > The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
> > > > > way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
> > > > > makes me wonder if this information shouldn't come from the device tree.
> > > > > 
> > > > > You are the only driver that uses this define directly, which makes me
> > > > > suspicious.
> > > > 
> > > > On Allwinner platforms, some devices do DMA directly on the memory BUS
> > > > with the DRAM controller. In such cases, the DRAM has no offset. In all
> > > > other cases where the DMA goes through the common system bus and the DRAM
> > > > offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
> > > > former case is not described in the device tree (this is being worked on
> > > > by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
> > > > only the display and media subsystems (VPU, camera, TS) are wired this
> > > > way.
> > > > 
> > > > In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
> > > > PHYS_OFFSET, which is pretty much the same thing.
> > > > 
> > > 
> > > OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
> > > with a comment that this will eventually come from the device tree.
> > 
> > That seems fine, although I'm less certain about what to do for the
> > SRAM situation. Other drivers that use SUNXI_SRAM have a Kconfig select
> > on it (that Cedrus lacks). Provided that the SRAM driver builds fine
> > for non-sunxi configs, bringing-in that select would remove the need
> > for dummy functions and also ensure that the actual implementation is
> > always used on sunxi. Otherwise, there'd be a risk of having the dummy
> > functions used (if the SRAM driver is not explicitly selected in the
> > config), causing a hang when accessing the VPU.
> 
> You should certainly select this kernel config.
> 
> But the real problem seems to to be drivers/soc/Makefile where it says:
> 
> obj-$(CONFIG_ARCH_SUNXI)       += sunxi/
> 
> I think this should be:
> 
> obj-y       += sunxi/
> 
> Now all compiles fine on i686 with the cedrus driver selecting SUNXI_SRAM.

I have just crafted a patch to the attention of Maxime to allow
building the sunxi SRAM driver without selecting the sunxi
architecture, the way you suggested.

With that, I just sent out v2 of the follow-up series for the Cedrus
driver, which should cover the build issues encountered so far.

Hopefully everything should be in order this time!

Cheers,

Paul

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 0/2] Follow-up patches for Cedrus v9

[Paul Kocialkowski]

Hi,

Le dimanche 09 septembre 2018 ? 11:04 +0200, Hans Verkuil a ?crit :
> On 09/08/2018 09:42 PM, Paul Kocialkowski wrote:
> > Hi,
> > 
> > Le samedi 08 septembre 2018 ? 13:24 +0200, Hans Verkuil a ?crit :
> > > On 09/08/2018 12:22 PM, Chen-Yu Tsai wrote:
> > > > On Sat, Sep 8, 2018 at 6:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > 
> > > > > On 09/07/2018 06:33 PM, Paul Kocialkowski wrote:
> > > > > > This brings the requested modifications on top of version 9 of the
> > > > > > Cedrus VPU driver, that implements stateless video decoding using the
> > > > > > Request API.
> > > > > > 
> > > > > > Paul Kocialkowski (2):
> > > > > >   media: cedrus: Fix error reporting in request validation
> > > > > >   media: cedrus: Add TODO file with tasks to complete before unstaging
> > > > > > 
> > > > > >  drivers/staging/media/sunxi/cedrus/TODO     |  7 +++++++
> > > > > >  drivers/staging/media/sunxi/cedrus/cedrus.c | 15 ++++++++++++---
> > > > > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > > > >  create mode 100644 drivers/staging/media/sunxi/cedrus/TODO
> > > > > > 
> > > > > 
> > > > > So close...
> > > > > 
> > > > > When compiling under e.g. intel I get errors since it doesn't know about
> > > > > the sunxi_sram_claim/release function and the PHYS_PFN_OFFSET define.
> > > > > 
> > > > > Is it possible to add stub functions to linux/soc/sunxi/sunxi_sram.h
> > > > > if CONFIG_SUNXI_SRAM is not defined? That would be the best fix for that.
> > > > > 
> > > > > The use of PHYS_PFN_OFFSET is weird: are you sure this is the right
> > > > > way? I see that drivers/of/device.c also sets dev->dma_pfn_offset, which
> > > > > makes me wonder if this information shouldn't come from the device tree.
> > > > > 
> > > > > You are the only driver that uses this define directly, which makes me
> > > > > suspicious.
> > > > 
> > > > On Allwinner platforms, some devices do DMA directly on the memory BUS
> > > > with the DRAM controller. In such cases, the DRAM has no offset. In all
> > > > other cases where the DMA goes through the common system bus and the DRAM
> > > > offset is either 0x40000000 or 0x20000000, depending on the SoC. Since the
> > > > former case is not described in the device tree (this is being worked on
> > > > by Maxime BTW), the dma_pfn_offset is not the value it should be. AFAIK
> > > > only the display and media subsystems (VPU, camera, TS) are wired this
> > > > way.
> > > > 
> > > > In drivers/gpu/drm/sun4i/sun4i_backend.c (the display driver) we use
> > > > PHYS_OFFSET, which is pretty much the same thing.
> > > > 
> > > 
> > > OK, in that case just put #ifdef PHYS_PFN_OFFSET around that line together
> > > with a comment that this will eventually come from the device tree.
> > 
> > That seems fine, although I'm less certain about what to do for the
> > SRAM situation. Other drivers that use SUNXI_SRAM have a Kconfig select
> > on it (that Cedrus lacks). Provided that the SRAM driver builds fine
> > for non-sunxi configs, bringing-in that select would remove the need
> > for dummy functions and also ensure that the actual implementation is
> > always used on sunxi. Otherwise, there'd be a risk of having the dummy
> > functions used (if the SRAM driver is not explicitly selected in the
> > config), causing a hang when accessing the VPU.
> 
> You should certainly select this kernel config.
> 
> But the real problem seems to to be drivers/soc/Makefile where it says:
> 
> obj-$(CONFIG_ARCH_SUNXI)       += sunxi/
> 
> I think this should be:
> 
> obj-y       += sunxi/
> 
> Now all compiles fine on i686 with the cedrus driver selecting SUNXI_SRAM.

I have just crafted a patch to the attention of Maxime to allow
building the sunxi SRAM driver without selecting the sunxi
architecture, the way you suggested.

With that, I just sent out v2 of the follow-up series for the Cedrus
driver, which should cover the build issues encountered so far.

Hopefully everything should be in order this time!

Cheers,

Paul

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180909/bd283f3e/attachment-0001.sig>