[PATCH dev-5.0 v2 0/5] Enable video engine

[PATCH dev-5.0 v2 0/5] Enable video engine

[Eddie James]

This series adds the necessary clocks for the video engine to the Aspeed clock
driver. It also adds the video engine node to the AST2500 dts and enables that
node in the witherspoon and romulus dts. It also removes the use of the reset
line in the video engine driver.

Changes since v1:
 - new reserved memory node for video engine
 - remove reset line from video engine driver
 - no more addition of reset line to clock driver

Eddie James (5):
  media: platform: Aspeed: Remove use of reset line
  clk: Aspeed: Setup video engine clocking
  ARM: dts: aspeed-g5: Add video engine
  ARM: dts: witherspoon: Enable video engine
  ARM: dts: romulus: Enable video engine

 arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts     | 12 +++++++
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 12 +++++++
 arch/arm/boot/dts/aspeed-g5.dtsi                 | 10 ++++++
 drivers/clk/clk-aspeed.c                         | 40 ++++++++++++++++++++++--
 drivers/media/platform/aspeed-video.c            | 39 +++++++++--------------
 5 files changed, 87 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Eddie James]

The reset line is toggled by enabling the clocks, so it's not necessary
to manually toggle the reset as well.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/media/platform/aspeed-video.c | 39 ++++++++++++++---------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 692e08e..9df38ad 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -14,7 +14,6 @@
 #include <linux/of_irq.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
-#include <linux/reset.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
@@ -46,9 +45,9 @@
 #define NUM_POLARITY_CHECKS		10
 #define INVALID_RESOLUTION_RETRIES	2
 #define INVALID_RESOLUTION_DELAY	msecs_to_jiffies(250)
-#define RESOLUTION_CHANGE_DELAY		msecs_to_jiffies(500)
-#define MODE_DETECT_TIMEOUT		msecs_to_jiffies(500)
-#define STOP_TIMEOUT			msecs_to_jiffies(1000)
+#define RESOLUTION_CHANGE_DELAY		msecs_to_jiffies(750)
+#define MODE_DETECT_TIMEOUT		msecs_to_jiffies(1000)
+#define STOP_TIMEOUT			msecs_to_jiffies(1500)
 #define DIRECT_FETCH_THRESHOLD		0x0c0000 /* 1024 * 768 */
 
 #define VE_MAX_SRC_BUFFER_SIZE		0x8ca000 /* 1920 * 1200, 32bpp */
@@ -208,7 +207,6 @@ struct aspeed_video {
 	void __iomem *base;
 	struct clk *eclk;
 	struct clk *vclk;
-	struct reset_control *rst;
 
 	struct device *dev;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
 	aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
 }
 
-static void aspeed_video_reset(struct aspeed_video *video)
-{
-	/* Reset the engine */
-	reset_control_assert(video->rst);
-
-	/* Don't usleep here; function may be called in interrupt context */
-	udelay(100);
-	reset_control_deassert(video->rst);
-}
-
 static void aspeed_video_off(struct aspeed_video *video)
 {
-	aspeed_video_reset(video);
+	/* Disable interrupts */
+	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
 
 	/* Turn off the relevant clocks */
 	clk_disable_unprepare(video->vclk);
@@ -508,7 +497,13 @@ static void aspeed_video_on(struct aspeed_video *video)
 	clk_prepare_enable(video->eclk);
 	clk_prepare_enable(video->vclk);
 
-	aspeed_video_reset(video);
+	/*
+	 * Delay (and don't sleep in case in interrupt context) to let the
+	 * clocks stabilize. Without this, mode detection sometimes fails.
+	 * Possibly the registers don't update too soon after clocks are
+	 * enabled.
+	 */
+	udelay(100);
 }
 
 static void aspeed_video_bufs_done(struct aspeed_video *video,
@@ -1464,7 +1459,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
 		 * Need to force stop any DMA and try and get HW into a good
 		 * state for future calls to start streaming again.
 		 */
-		aspeed_video_reset(video);
+		aspeed_video_off(video);
+		aspeed_video_on(video);
+
 		aspeed_video_init_regs(video);
 
 		aspeed_video_get_resolution(video);
@@ -1619,12 +1616,6 @@ static int aspeed_video_init(struct aspeed_video *video)
 		return PTR_ERR(video->vclk);
 	}
 
-	video->rst = devm_reset_control_get_exclusive(dev, NULL);
-	if (IS_ERR(video->rst)) {
-		dev_err(dev, "Unable to get VE reset\n");
-		return PTR_ERR(video->rst);
-	}
-
 	rc = of_reserved_mem_device_init(dev);
 	if (rc) {
 		dev_err(dev, "Unable to reserve memory\n");
-- 
1.8.3.1

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Jae Hyun Yoo]

>   static void aspeed_video_off(struct aspeed_video *video)
>   {
> -	aspeed_video_reset(video);
> +	/* Disable interrupts */
> +	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>   
>   	/* Turn off the relevant clocks */
>   	clk_disable_unprepare(video->vclk);
> @@ -508,7 +497,13 @@ static void aspeed_video_on(struct aspeed_video *video)
>   	clk_prepare_enable(video->eclk);
>   	clk_prepare_enable(video->vclk);
>   
> -	aspeed_video_reset(video);
> +	/*
> +	 * Delay (and don't sleep in case in interrupt context) to let the
> +	 * clocks stabilize. Without this, mode detection sometimes fails.
> +	 * Possibly the registers don't update too soon after clocks are
> +	 * enabled.
> +	 */
> +	udelay(100);
>   }

aspeed_video_off() will be called from interrupt context too. Are you
sure that this 100us delay is needed at here? If it's actually needed,
would it be batter to use devm_request_threaded_irq() instead of
devm_request_irq() in aspeed_video_init() function?

Cheers,
Jae

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Eddie James]

On 3/29/19 6:54 PM, Jae Hyun Yoo wrote:
>>   static void aspeed_video_off(struct aspeed_video *video)
>>   {
>> -    aspeed_video_reset(video);
>> +    /* Disable interrupts */
>> +    aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>>         /* Turn off the relevant clocks */
>>       clk_disable_unprepare(video->vclk);
>> @@ -508,7 +497,13 @@ static void aspeed_video_on(struct aspeed_video 
>> *video)
>>       clk_prepare_enable(video->eclk);
>>       clk_prepare_enable(video->vclk);
>>   -    aspeed_video_reset(video);
>> +    /*
>> +     * Delay (and don't sleep in case in interrupt context) to let the
>> +     * clocks stabilize. Without this, mode detection sometimes fails.
>> +     * Possibly the registers don't update too soon after clocks are
>> +     * enabled.
>> +     */
>> +    udelay(100);
>>   }
>
> aspeed_video_off() will be called from interrupt context too. Are you
> sure that this 100us delay is needed at here? If it's actually needed,
> would it be batter to use devm_request_threaded_irq() instead of
> devm_request_irq() in aspeed_video_init() function?

Well this isn't new to this patchset, it delayed 100us before in 
aspeed_video_reset.

I am quite sure it's needed.

Since the ast2500 has only one logical processor, I'm not sure a 
threaded irq would function any differently.

>
> Cheers,
> Jae
>

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Jae Hyun Yoo]

On 4/1/2019 8:11 AM, Eddie James wrote:
> 
> On 3/29/19 6:54 PM, Jae Hyun Yoo wrote:
>>>   static void aspeed_video_off(struct aspeed_video *video)
>>>   {
>>> -    aspeed_video_reset(video);
>>> +    /* Disable interrupts */
>>> +    aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>>>         /* Turn off the relevant clocks */
>>>       clk_disable_unprepare(video->vclk);
>>> @@ -508,7 +497,13 @@ static void aspeed_video_on(struct aspeed_video 
>>> *video)
>>>       clk_prepare_enable(video->eclk);
>>>       clk_prepare_enable(video->vclk);
>>>   -    aspeed_video_reset(video);
>>> +    /*
>>> +     * Delay (and don't sleep in case in interrupt context) to let the
>>> +     * clocks stabilize. Without this, mode detection sometimes fails.
>>> +     * Possibly the registers don't update too soon after clocks are
>>> +     * enabled.
>>> +     */
>>> +    udelay(100);
>>>   }
>>
>> aspeed_video_off() will be called from interrupt context too. Are you
>> sure that this 100us delay is needed at here? If it's actually needed,
>> would it be batter to use devm_request_threaded_irq() instead of
>> devm_request_irq() in aspeed_video_init() function?
> 
> Well this isn't new to this patchset, it delayed 100us before in 
> aspeed_video_reset.
> 
> I am quite sure it's needed.

This is previous code of aspeed_video_reset():
static void aspeed_video_reset(struct aspeed_video *video)
{
	/* Reset the engine */
	reset_control_assert(video->rst);

	/* Don't usleep here; function may be called in interrupt context */
	udelay(100);
	reset_control_deassert(video->rst);
}

The delay was added between assert and deassert to shape the reset
signal up, but the delay in this patch set is after the deasserting of
reset signal. Now in this patch set, you are going to use reset control
using the way that clk-aspeed module provides. The module already has
a 10msec delay in aspeed_clk_enable() for making the reset pulse so
you don't need to add an additional delay at here.

Practically, I tested this patch after removing the 100usec delay and
checked that this driver works well.

> Since the ast2500 has only one logical processor, I'm not sure a 
> threaded irq would function any differently.

Yes, good point. Differently from other Aspeed driver modules, it calls
clk_prepare_enable() in interrupt context which will cause at least
10msecs of busy waiting thru aspeed_clk_enable() in isr. In UP kernel,
this will disturb handling of most of all other interrupts so it's the
reason why I suggested threaded irq instead.

Try this change in aspeed_video_init():
rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
			       IRQF_ONESHOT | IRQF_SHARED, DEVICE_NAME,
			       video);

This threaded irq handling works well in my H/W.

Cheers,
Jae

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Eddie James]

On 4/1/19 12:25 PM, Jae Hyun Yoo wrote:
>
>
> On 4/1/2019 8:11 AM, Eddie James wrote:
>>
>> On 3/29/19 6:54 PM, Jae Hyun Yoo wrote:
>>>>   static void aspeed_video_off(struct aspeed_video *video)
>>>>   {
>>>> -    aspeed_video_reset(video);
>>>> +    /* Disable interrupts */
>>>> +    aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>>>>         /* Turn off the relevant clocks */
>>>>       clk_disable_unprepare(video->vclk);
>>>> @@ -508,7 +497,13 @@ static void aspeed_video_on(struct 
>>>> aspeed_video *video)
>>>>       clk_prepare_enable(video->eclk);
>>>>       clk_prepare_enable(video->vclk);
>>>>   -    aspeed_video_reset(video);
>>>> +    /*
>>>> +     * Delay (and don't sleep in case in interrupt context) to let 
>>>> the
>>>> +     * clocks stabilize. Without this, mode detection sometimes 
>>>> fails.
>>>> +     * Possibly the registers don't update too soon after clocks are
>>>> +     * enabled.
>>>> +     */
>>>> +    udelay(100);
>>>>   }
>>>
>>> aspeed_video_off() will be called from interrupt context too. Are you
>>> sure that this 100us delay is needed at here? If it's actually needed,
>>> would it be batter to use devm_request_threaded_irq() instead of
>>> devm_request_irq() in aspeed_video_init() function?
>>
>> Well this isn't new to this patchset, it delayed 100us before in 
>> aspeed_video_reset.
>>
>> I am quite sure it's needed.
>
> This is previous code of aspeed_video_reset():
> static void aspeed_video_reset(struct aspeed_video *video)
> {
>     /* Reset the engine */
>     reset_control_assert(video->rst);
>
>     /* Don't usleep here; function may be called in interrupt context */
>     udelay(100);
>     reset_control_deassert(video->rst);
> }
>
> The delay was added between assert and deassert to shape the reset
> signal up, but the delay in this patch set is after the deasserting of
> reset signal. Now in this patch set, you are going to use reset control
> using the way that clk-aspeed module provides. The module already has
> a 10msec delay in aspeed_clk_enable() for making the reset pulse so
> you don't need to add an additional delay at here.


That is the logical conclusion, but the results I got didn't agree. In 
my previous testing, I was only able to detect the resolution maybe 50% 
of the time without this delay.


>
> Practically, I tested this patch after removing the 100usec delay and
> checked that this driver works well.


Now I just ran my tests, and I can't recreate my previous problem with 
or without the delay... I don't understand that. Same code, same system, 
but different kernel build. Very odd. But that leaves me with no 
justification for this delay, so I'll remove it.


>
>> Since the ast2500 has only one logical processor, I'm not sure a 
>> threaded irq would function any differently.
>
> Yes, good point. Differently from other Aspeed driver modules, it calls
> clk_prepare_enable() in interrupt context which will cause at least
> 10msecs of busy waiting thru aspeed_clk_enable() in isr. In UP kernel,
> this will disturb handling of most of all other interrupts so it's the
> reason why I suggested threaded irq instead.
>
> Try this change in aspeed_video_init():
> rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
>                    IRQF_ONESHOT | IRQF_SHARED, DEVICE_NAME,
>                    video);


Looks good, but this should probably go in a different patch set.


>
> This threaded irq handling works well in my H/W.
>
> Cheers,
> Jae
>

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Milton Miller II]

Around 04/01/2019 12:26PM in some timezone, Jae Hyun Yoo wrote:
>On 4/1/2019 8:11 AM, Eddie James wrote:
>> 
>> On 3/29/19 6:54 PM, Jae Hyun Yoo wrote:
>>>>   static void aspeed_video_off(struct aspeed_video *video)
>>>>   {
>>>> -    aspeed_video_reset(video);
>>>> +    /* Disable interrupts */
>>>> +    aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>>>>         /* Turn off the relevant clocks */
>>>>       clk_disable_unprepare(video->vclk);
>>>> @@ -508,7 +497,13 @@ static void aspeed_video_on(struct
>aspeed_video 
>>>> *video)
>>>>       clk_prepare_enable(video->eclk);
>>>>       clk_prepare_enable(video->vclk);
>>>>   -    aspeed_video_reset(video);
>>>> +    /*
>>>> +     * Delay (and don't sleep in case in interrupt context) to
>let the
>>>> +     * clocks stabilize. Without this, mode detection sometimes
>fails.
>>>> +     * Possibly the registers don't update too soon after clocks
>are
>>>> +     * enabled.
>>>> +     */
>>>> +    udelay(100);
>>>>   }
>>>
>>> aspeed_video_off() will be called from interrupt context too. Are
>you
>>> sure that this 100us delay is needed at here? If it's actually
>needed,
>>> would it be batter to use devm_request_threaded_irq() instead of
>>> devm_request_irq() in aspeed_video_init() function?
>> 

More importantly, the prepare part of prepare_enable is sleepable 
and must be called from a thread context, so threaded irq is likely 
required if this needs to be called.

>> Well this isn't new to this patchset, it delayed 100us before in >> aspeed_video_reset.
>> 
>> I am quite sure it's needed.
>
>This is previous code of aspeed_video_reset():
>static void aspeed_video_reset(struct aspeed_video *video)
>{
>	/* Reset the engine */
>	reset_control_assert(video->rst);
>
>	/* Don't usleep here; function may be called in interrupt context */
>	udelay(100);
>	reset_control_deassert(video->rst);
>}
>
>The delay was added between assert and deassert to shape the reset
>signal up, but the delay in this patch set is after the deasserting
>of
>reset signal. Now in this patch set, you are going to use reset
>control
>using the way that clk-aspeed module provides. The module already has
>a 10msec delay in aspeed_clk_enable() for making the reset pulse so
>you don't need to add an additional delay at here.
>
>Practically, I tested this patch after removing the 100usec delay and
>checked that this driver works well.
>
>> Since the ast2500 has only one logical processor, I'm not sure a 
>> threaded irq would function any differently.
>
>Yes, good point. Differently from other Aspeed driver modules, it
>calls
>clk_prepare_enable() in interrupt context which will cause at least
>10msecs of busy waiting thru aspeed_clk_enable() in isr. In UP
>kernel,
>this will disturb handling of most of all other interrupts so it's
>the
>reason why I suggested threaded irq instead.
>
>Try this change in aspeed_video_init():
>rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
>			       IRQF_ONESHOT | IRQF_SHARED, DEVICE_NAME,
>			       video);

milton

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Joel Stanley]

On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
>
> The reset line is toggled by enabling the clocks, so it's not necessary
> to manually toggle the reset as well.

Hi Eddie,

You didn't include a changelog here. You said last week that without
the extra reset line toggle you would get interrupt storms. Can you
fill us in on what changed?

>  #define INVALID_RESOLUTION_DELAY       msecs_to_jiffies(250)
> -#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(500)
> -#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(500)
> -#define STOP_TIMEOUT                   msecs_to_jiffies(1000)
> +#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(750)
> +#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(1000)
> +#define STOP_TIMEOUT                   msecs_to_jiffies(1500)

These numbers changed but you didn't mention why.

>  #define DIRECT_FETCH_THRESHOLD         0x0c0000 /* 1024 * 768 */
>
>  #define VE_MAX_SRC_BUFFER_SIZE         0x8ca000 /* 1920 * 1200, 32bpp */
> @@ -208,7 +207,6 @@ struct aspeed_video {
>         void __iomem *base;
>         struct clk *eclk;
>         struct clk *vclk;
> -       struct reset_control *rst;
>
>         struct device *dev;
>         struct v4l2_ctrl_handler ctrl_handler;
> @@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
>         aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
>  }
>
> -static void aspeed_video_reset(struct aspeed_video *video)
> -{
> -       /* Reset the engine */
> -       reset_control_assert(video->rst);
> -
> -       /* Don't usleep here; function may be called in interrupt context */
> -       udelay(100);
> -       reset_control_deassert(video->rst);
> -}
> -
>  static void aspeed_video_off(struct aspeed_video *video)
>  {

> -       aspeed_video_reset(video);
> +       /*
> +        * Delay (and don't sleep in case in interrupt context) to let the
> +        * clocks stabilize. Without this, mode detection sometimes fails.
> +        * Possibly the registers don't update too soon after clocks are
> +        * enabled.
> +        */
> +       udelay(100);

How did you get to 100us?

> @@ -1464,7 +1459,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)

> -               aspeed_video_reset(video);
> +               aspeed_video_off(video);
> +               aspeed_video_on(video);
> +

This includes 5.1ms of sleeping in the clock driver's enable path; are
you sure we need to turn clocks off and on again? Can we avoid the
full reset by instead resetting the registers to a specific state?

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Eddie James]

On 4/1/19 1:23 AM, Joel Stanley wrote:
> On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
>> The reset line is toggled by enabling the clocks, so it's not necessary
>> to manually toggle the reset as well.
> Hi Eddie,
>
> You didn't include a changelog here. You said last week that without
> the extra reset line toggle you would get interrupt storms. Can you
> fill us in on what changed?
I disabled interrupts before turning the clocks off. That wasn't the 
only issue, so I had to add the delay after turning clocks on in order 
to reliably get the resolution.
>
>>   #define INVALID_RESOLUTION_DELAY       msecs_to_jiffies(250)
>> -#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(500)
>> -#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(500)
>> -#define STOP_TIMEOUT                   msecs_to_jiffies(1000)
>> +#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(750)
>> +#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(1000)
>> +#define STOP_TIMEOUT                   msecs_to_jiffies(1500)
> These numbers changed but you didn't mention why.
Increasing these helped improve stability when changing resolution.
>
>>   #define DIRECT_FETCH_THRESHOLD         0x0c0000 /* 1024 * 768 */
>>
>>   #define VE_MAX_SRC_BUFFER_SIZE         0x8ca000 /* 1920 * 1200, 32bpp */
>> @@ -208,7 +207,6 @@ struct aspeed_video {
>>          void __iomem *base;
>>          struct clk *eclk;
>>          struct clk *vclk;
>> -       struct reset_control *rst;
>>
>>          struct device *dev;
>>          struct v4l2_ctrl_handler ctrl_handler;
>> @@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
>>          aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
>>   }
>>
>> -static void aspeed_video_reset(struct aspeed_video *video)
>> -{
>> -       /* Reset the engine */
>> -       reset_control_assert(video->rst);
>> -
>> -       /* Don't usleep here; function may be called in interrupt context */
>> -       udelay(100);
>> -       reset_control_deassert(video->rst);
>> -}
>> -
>>   static void aspeed_video_off(struct aspeed_video *video)
>>   {
>> -       aspeed_video_reset(video);
>> +       /*
>> +        * Delay (and don't sleep in case in interrupt context) to let the
>> +        * clocks stabilize. Without this, mode detection sometimes fails.
>> +        * Possibly the registers don't update too soon after clocks are
>> +        * enabled.
>> +        */
>> +       udelay(100);
> How did you get to 100us?

I used the delay from between the reset assert/deassert used in the old 
aspeed_video_reset function.

>
>> @@ -1464,7 +1459,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
>> -               aspeed_video_reset(video);
>> +               aspeed_video_off(video);
>> +               aspeed_video_on(video);
>> +
> This includes 5.1ms of sleeping in the clock driver's enable path; are
> you sure we need to turn clocks off and on again? Can we avoid the
> full reset by instead resetting the registers to a specific state?

In this case the engine has failed to stop streaming, so it's not 
obeying commands via the registers. Only option is to reset the engine. 
It's an error condition.

>

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Joel Stanley]

On Mon, 1 Apr 2019 at 14:32, Eddie James <eajames@linux.ibm.com> wrote:
>
>
> On 4/1/19 1:23 AM, Joel Stanley wrote:
> > On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
> >> The reset line is toggled by enabling the clocks, so it's not necessary
> >> to manually toggle the reset as well.
> > Hi Eddie,
> >
> > You didn't include a changelog here. You said last week that without
> > the extra reset line toggle you would get interrupt storms. Can you
> > fill us in on what changed?
> I disabled interrupts before turning the clocks off. That wasn't the
> only issue, so I had to add the delay after turning clocks on in order
> to reliably get the resolution.
> >
> >>   #define INVALID_RESOLUTION_DELAY       msecs_to_jiffies(250)
> >> -#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(500)
> >> -#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(500)
> >> -#define STOP_TIMEOUT                   msecs_to_jiffies(1000)
> >> +#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(750)
> >> +#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(1000)
> >> +#define STOP_TIMEOUT                   msecs_to_jiffies(1500)
> > These numbers changed but you didn't mention why.
> Increasing these helped improve stability when changing resolution.

You need to mention why in your commit message.

> >
> >>   #define DIRECT_FETCH_THRESHOLD         0x0c0000 /* 1024 * 768 */
> >>
> >>   #define VE_MAX_SRC_BUFFER_SIZE         0x8ca000 /* 1920 * 1200, 32bpp */
> >> @@ -208,7 +207,6 @@ struct aspeed_video {
> >>          void __iomem *base;
> >>          struct clk *eclk;
> >>          struct clk *vclk;
> >> -       struct reset_control *rst;
> >>
> >>          struct device *dev;
> >>          struct v4l2_ctrl_handler ctrl_handler;
> >> @@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
> >>          aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
> >>   }
> >>
> >> -static void aspeed_video_reset(struct aspeed_video *video)
> >> -{
> >> -       /* Reset the engine */
> >> -       reset_control_assert(video->rst);
> >> -
> >> -       /* Don't usleep here; function may be called in interrupt context */
> >> -       udelay(100);
> >> -       reset_control_deassert(video->rst);
> >> -}
> >> -
> >>   static void aspeed_video_off(struct aspeed_video *video)
> >>   {
> >> -       aspeed_video_reset(video);
> >> +       /*
> >> +        * Delay (and don't sleep in case in interrupt context) to let the
> >> +        * clocks stabilize. Without this, mode detection sometimes fails.
> >> +        * Possibly the registers don't update too soon after clocks are
> >> +        * enabled.
> >> +        */
> >> +       udelay(100);
> > How did you get to 100us?
>
> I used the delay from between the reset assert/deassert used in the old
> aspeed_video_reset function.

Is this what aspeed said we needed? What happens when it's less?

>
> >
> >> @@ -1464,7 +1459,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
> >> -               aspeed_video_reset(video);
> >> +               aspeed_video_off(video);
> >> +               aspeed_video_on(video);
> >> +
> > This includes 5.1ms of sleeping in the clock driver's enable path; are
> > you sure we need to turn clocks off and on again? Can we avoid the
> > full reset by instead resetting the registers to a specific state?
>
> In this case the engine has failed to stop streaming, so it's not
> obeying commands via the registers. Only option is to reset the engine.
> It's an error condition.

This would be a good comment to add to the source code.

>
> >
>

Re: [PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Eddie James]

On 4/1/19 8:39 PM, Joel Stanley wrote:
> On Mon, 1 Apr 2019 at 14:32, Eddie James <eajames@linux.ibm.com> wrote:
>>
>> On 4/1/19 1:23 AM, Joel Stanley wrote:
>>> On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
>>>> The reset line is toggled by enabling the clocks, so it's not necessary
>>>> to manually toggle the reset as well.
>>> Hi Eddie,
>>>
>>> You didn't include a changelog here. You said last week that without
>>> the extra reset line toggle you would get interrupt storms. Can you
>>> fill us in on what changed?
>> I disabled interrupts before turning the clocks off. That wasn't the
>> only issue, so I had to add the delay after turning clocks on in order
>> to reliably get the resolution.
>>>>    #define INVALID_RESOLUTION_DELAY       msecs_to_jiffies(250)
>>>> -#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(500)
>>>> -#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(500)
>>>> -#define STOP_TIMEOUT                   msecs_to_jiffies(1000)
>>>> +#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(750)
>>>> +#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(1000)
>>>> +#define STOP_TIMEOUT                   msecs_to_jiffies(1500)
>>> These numbers changed but you didn't mention why.
>> Increasing these helped improve stability when changing resolution.
> You need to mention why in your commit message.


I've dropped those changes in the latest patch set.


>
>>>>    #define DIRECT_FETCH_THRESHOLD         0x0c0000 /* 1024 * 768 */
>>>>
>>>>    #define VE_MAX_SRC_BUFFER_SIZE         0x8ca000 /* 1920 * 1200, 32bpp */
>>>> @@ -208,7 +207,6 @@ struct aspeed_video {
>>>>           void __iomem *base;
>>>>           struct clk *eclk;
>>>>           struct clk *vclk;
>>>> -       struct reset_control *rst;
>>>>
>>>>           struct device *dev;
>>>>           struct v4l2_ctrl_handler ctrl_handler;
>>>> @@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
>>>>           aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
>>>>    }
>>>>
>>>> -static void aspeed_video_reset(struct aspeed_video *video)
>>>> -{
>>>> -       /* Reset the engine */
>>>> -       reset_control_assert(video->rst);
>>>> -
>>>> -       /* Don't usleep here; function may be called in interrupt context */
>>>> -       udelay(100);
>>>> -       reset_control_deassert(video->rst);
>>>> -}
>>>> -
>>>>    static void aspeed_video_off(struct aspeed_video *video)
>>>>    {
>>>> -       aspeed_video_reset(video);
>>>> +       /*
>>>> +        * Delay (and don't sleep in case in interrupt context) to let the
>>>> +        * clocks stabilize. Without this, mode detection sometimes fails.
>>>> +        * Possibly the registers don't update too soon after clocks are
>>>> +        * enabled.
>>>> +        */
>>>> +       udelay(100);
>>> How did you get to 100us?
>> I used the delay from between the reset assert/deassert used in the old
>> aspeed_video_reset function.
> Is this what aspeed said we needed? What happens when it's less?


I didn't do any testing between 1 and 99 us. The original value was 
derived from some of the comments in the clock change area of the 
ast2500 specification. In my testing of this patch set, 0 delay resulted 
in frequent failure to grab the resolution, but today I was unable to 
reproduce that behavior and so I have dropped the delay from the latest 
patch set.


>
>>>> @@ -1464,7 +1459,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
>>>> -               aspeed_video_reset(video);
>>>> +               aspeed_video_off(video);
>>>> +               aspeed_video_on(video);
>>>> +
>>> This includes 5.1ms of sleeping in the clock driver's enable path; are
>>> you sure we need to turn clocks off and on again? Can we avoid the
>>> full reset by instead resetting the registers to a specific state?
>> In this case the engine has failed to stop streaming, so it's not
>> obeying commands via the registers. Only option is to reset the engine.
>> It's an error condition.
> This would be a good comment to add to the source code.


There is a comment and dev_err message already, just not in the diff.


>

[PATCH dev-5.0 v2 2/5] clk: Aspeed: Setup video engine clocking

[Eddie James]

Add eclk mux and clock divider table.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/clk/clk-aspeed.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 5961367..133e80c 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -87,7 +87,7 @@ struct aspeed_clk_gate {
 /* TODO: ask Aspeed about the actual parent data */
 static const struct aspeed_gate_data aspeed_gates[] = {
 	/*				 clk rst   name			parent	flags */
-	[ASPEED_CLK_GATE_ECLK] =	{  0, -1, "eclk-gate",		"eclk",	0 }, /* Video Engine */
+	[ASPEED_CLK_GATE_ECLK] =	{  0,  6, "eclk-gate",		"eclk",	0 }, /* Video Engine */
 	[ASPEED_CLK_GATE_GCLK] =	{  1,  7, "gclk-gate",		NULL,	0 }, /* 2D engine */
 	[ASPEED_CLK_GATE_MCLK] =	{  2, -1, "mclk-gate",		"mpll",	CLK_IS_CRITICAL }, /* SDRAM */
 	[ASPEED_CLK_GATE_VCLK] =	{  3,  6, "vclk-gate",		NULL,	0 }, /* Video Capture */
@@ -113,6 +113,24 @@ struct aspeed_clk_gate {
 	[ASPEED_CLK_GATE_LHCCLK] =	{ 28, -1, "lhclk-gate",		"lhclk", 0 }, /* LPC master/LPC+ */
 };
 
+static const char * const eclk_parent_names[] = {
+	"mpll",
+	"hpll",
+	"dpll",
+};
+
+static const struct clk_div_table ast2500_eclk_div_table[] = {
+	{ 0x0, 2 },
+	{ 0x1, 2 },
+	{ 0x2, 3 },
+	{ 0x3, 4 },
+	{ 0x4, 5 },
+	{ 0x5, 6 },
+	{ 0x6, 7 },
+	{ 0x7, 8 },
+	{ 0 }
+};
+
 static const struct clk_div_table ast2500_mac_div_table[] = {
 	{ 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
 	{ 0x1, 4 },
@@ -192,18 +210,21 @@ static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
 
 struct aspeed_clk_soc_data {
 	const struct clk_div_table *div_table;
+	const struct clk_div_table *eclk_div_table;
 	const struct clk_div_table *mac_div_table;
 	struct clk_hw *(*calc_pll)(const char *name, u32 val);
 };
 
 static const struct aspeed_clk_soc_data ast2500_data = {
 	.div_table = ast2500_div_table,
+	.eclk_div_table = ast2500_eclk_div_table,
 	.mac_div_table = ast2500_mac_div_table,
 	.calc_pll = aspeed_ast2500_calc_pll,
 };
 
 static const struct aspeed_clk_soc_data ast2400_data = {
 	.div_table = ast2400_div_table,
+	.eclk_div_table = ast2400_div_table,
 	.mac_div_table = ast2400_div_table,
 	.calc_pll = aspeed_ast2400_calc_pll,
 };
@@ -522,6 +543,22 @@ static int aspeed_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(hw);
 	aspeed_clk_data->hws[ASPEED_CLK_24M] = hw;
 
+	hw = clk_hw_register_mux(dev, "eclk-mux", eclk_parent_names,
+				 ARRAY_SIZE(eclk_parent_names), 0,
+				 scu_base + ASPEED_CLK_SELECTION, 2, 0x3, 0,
+				 &aspeed_clk_lock);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
+
+	hw = clk_hw_register_divider_table(dev, "eclk", "eclk-mux", 0,
+					   scu_base + ASPEED_CLK_SELECTION, 28,
+					   3, 0, soc_data->eclk_div_table,
+					   &aspeed_clk_lock);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
+
 	/*
 	 * TODO: There are a number of clocks that not included in this driver
 	 * as more information is required:
@@ -531,7 +568,6 @@ static int aspeed_clk_probe(struct platform_device *pdev)
 	 *   RGMII
 	 *   RMII
 	 *   UART[1..5] clock source mux
-	 *   Video Engine (ECLK) mux and clock divider
 	 */
 
 	for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
-- 
1.8.3.1

Re: [PATCH dev-5.0 v2 2/5] clk: Aspeed: Setup video engine clocking

[Jae Hyun Yoo]

Hi Eddie,

On 3/29/2019 2:15 PM, Eddie James wrote:
>   static const struct aspeed_gate_data aspeed_gates[] = {
>   	/*				 clk rst   name			parent	flags */
> -	[ASPEED_CLK_GATE_ECLK] =	{  0, -1, "eclk-gate",		"eclk",	0 }, /* Video Engine */
> +	[ASPEED_CLK_GATE_ECLK] =	{  0,  6, "eclk-gate",		"eclk",	0 }, /* Video Engine */
>   	[ASPEED_CLK_GATE_GCLK] =	{  1,  7, "gclk-gate",		NULL,	0 }, /* 2D engine */
>   	[ASPEED_CLK_GATE_MCLK] =	{  2, -1, "mclk-gate",		"mpll",	CLK_IS_CRITICAL }, /* SDRAM */
>   	[ASPEED_CLK_GATE_VCLK] =	{  3,  6, "vclk-gate",		NULL,	0 }, /* Video Capture */

With this change, both eclk and vclk will be coupled with reset bit '6'.
Actually, the reset bit 6 is for video engine so this change seems
correct to me, but I think you should replace reset bit of vclk with -1
instead.

Cheers,
Jae

[PATCH dev-5.0 v2 3/5] ARM: dts: aspeed-g5: Add video engine

[Eddie James]

Add a node to describe the video engine on the AST2500.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-g5.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index a79e01f..d11c078 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -252,6 +252,16 @@
 				status = "disabled";
 			};
 
+			video: video@1e700000 {
+				compatible = "aspeed,ast2500-video-engine";
+				reg = <0x1e700000 0x1000>;
+				clocks = <&syscon ASPEED_CLK_GATE_VCLK>,
+					 <&syscon ASPEED_CLK_GATE_ECLK>;
+				clock-names = "vclk", "eclk";
+				interrupts = <7>;
+				status = "disabled";
+			};
+
 			sram: sram@1e720000 {
 				compatible = "mmio-sram";
 				reg = <0x1e720000 0x9000>;	// 36K
-- 
1.8.3.1

[PATCH dev-5.0 v2 4/5] ARM: dts: witherspoon: Enable video engine

[Eddie James]

Enable the video engine and add it's required reserved memory region.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index 1cdc96d..1faf8b4 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -33,6 +33,13 @@
 			compatible = "shared-dma-pool";
 			reusable;
 		};
+
+		ve_memory: jpegbuffer {
+			size = <0x04000000>;	/* 64M */
+			alignment = <0x01000000>;
+			compatible = "shared-dma-pool";
+			reusable;
+		};
 	};
 
 	gpio-keys {
@@ -664,4 +671,9 @@
 	status = "okay";
 };
 
+&video {
+	status = "okay";
+	memory-region = <&ve_memory>;
+};
+
 #include "ibm-power9-dual.dtsi"
-- 
1.8.3.1

[PATCH dev-5.0 v2 5/5] ARM: dts: romulus: Enable video engine

[Eddie James]

Enable the video engine and add it's required reserved memory region.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
index ee1a460..54427f4 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
@@ -42,6 +42,13 @@
 			compatible = "shared-dma-pool";
 			reusable;
 		};
+
+		ve_memory: jpegbuffer {
+			size = <0x04000000>;	/* 64M */
+			alignment = <0x01000000>;
+			compatible = "shared-dma-pool";
+			reusable;
+		};
 	};
 
 	leds {
@@ -315,4 +322,9 @@
      memory-region = <&gfx_memory>;
 };
 
+&video {
+	status = "okay";
+	memory-region = <&ve_memory>;
+};
+
 #include "ibm-power9-dual.dtsi"
-- 
1.8.3.1

Re: [PATCH dev-5.0 v2 5/5] ARM: dts: romulus: Enable video engine

[Joel Stanley]

On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
>
> Enable the video engine and add it's required reserved memory region.

The upstream bindings don't mention the reserved memory region at all.

>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> index ee1a460..54427f4 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> @@ -42,6 +42,13 @@
>                         compatible = "shared-dma-pool";
>                         reusable;
>                 };
> +
> +               ve_memory: jpegbuffer {

video_memory or video_engine_memory

> +                       size = <0x04000000>;    /* 64M */

How do we get to 64MB?


> +                       alignment = <0x01000000>;
> +                       compatible = "shared-dma-pool";
> +                       reusable;
> +               };
>         };
>
>         leds {
> @@ -315,4 +322,9 @@
>       memory-region = <&gfx_memory>;
>  };
>
> +&video {
> +       status = "okay";
> +       memory-region = <&ve_memory>;
> +};
> +
>  #include "ibm-power9-dual.dtsi"
> --
> 1.8.3.1
>

Re: [PATCH dev-5.0 v2 5/5] ARM: dts: romulus: Enable video engine

[Eddie James]

On 4/1/19 1:27 AM, Joel Stanley wrote:
> On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
>> Enable the video engine and add it's required reserved memory region.
> The upstream bindings don't mention the reserved memory region at all.
Whoops... I'll have to send a patch for that upstream.
>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
>> index ee1a460..54427f4 100644
>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
>> @@ -42,6 +42,13 @@
>>                          compatible = "shared-dma-pool";
>>                          reusable;
>>                  };
>> +
>> +               ve_memory: jpegbuffer {
> video_memory or video_engine_memory
>
>> +                       size = <0x04000000>;    /* 64M */
> How do we get to 64MB?
OK it looks like we can get away with 32MB. I hadn't run the 
calculations since the beginning of working on this driver where I was 
using a lot more memory for fixed buffers. But we do need 32MB since we 
need two source buffers of up to 1920x1200x32bpp.
>
>
>> +                       alignment = <0x01000000>;
>> +                       compatible = "shared-dma-pool";
>> +                       reusable;
>> +               };
>>          };
>>
>>          leds {
>> @@ -315,4 +322,9 @@
>>        memory-region = <&gfx_memory>;
>>   };
>>
>> +&video {
>> +       status = "okay";
>> +       memory-region = <&ve_memory>;
>> +};
>> +
>>   #include "ibm-power9-dual.dtsi"
>> --
>> 1.8.3.1
>>

Re: [PATCH dev-5.0 v2 5/5] ARM: dts: romulus: Enable video engine

[Joel Stanley]

On Mon, 1 Apr 2019 at 15:13, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>
>
> On 4/1/19 1:27 AM, Joel Stanley wrote:
> > On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames@linux.ibm.com> wrote:
> >> Enable the video engine and add it's required reserved memory region.
> > The upstream bindings don't mention the reserved memory region at all.
> Whoops... I'll have to send a patch for that upstream.

Cool.

> >
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> >> ---
> >>   arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> >> index ee1a460..54427f4 100644
> >> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> >> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> >> @@ -42,6 +42,13 @@
> >>                          compatible = "shared-dma-pool";
> >>                          reusable;
> >>                  };
> >> +
> >> +               ve_memory: jpegbuffer {
> > video_memory or video_engine_memory
> >
> >> +                       size = <0x04000000>;    /* 64M */
> > How do we get to 64MB?
> OK it looks like we can get away with 32MB. I hadn't run the
> calculations since the beginning of working on this driver where I was
> using a lot more memory for fixed buffers. But we do need 32MB since we
> need two source buffers of up to 1920x1200x32bpp.

Again, you need to add this information in the commit message.

I recommend putting the calculation in the commit message too.