[v3,4/5] media: vsp1: Split out pre-filter multiplier
diff mbox series

Message ID 20190411161256.19607-5-kieran.bingham+renesas@ideasonboard.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • media: vsp1: Phased partition overlap support
Related show

Commit Message

Kieran Bingham April 11, 2019, 4:12 p.m. UTC
The 'mp' value is used through many calculations in determining the scaling
factors of the UDS. Factor this out so that it can be reused in further
calculations, and also ensure that if the BLADV control is ever changed only a
single function needs to be modified.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_uds.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart April 18, 2019, 6:37 a.m. UTC | #1
Hi Kieran,

Thank you for the patch.

On Thu, Apr 11, 2019 at 05:12:55PM +0100, Kieran Bingham wrote:
> The 'mp' value is used through many calculations in determining the scaling
> factors of the UDS. Factor this out so that it can be reused in further
> calculations, and also ensure that if the BLADV control is ever changed only a
> single function needs to be modified.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_uds.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
> index 27012af973b2..c71c24363d54 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -46,6 +46,18 @@ void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_body *dlb,
>  		       alpha << VI6_UDS_ALPVAL_VAL0_SHIFT);
>  }
>  
> +/*
> + * Determine the pre-filter multiplication value.

This would benefit from a more detailed description, and in particular a
definition of what "pre-filter" means.

> + *
> + * This calculation assumes that the BLADV control is unset.

s/control/bit/ ?
s/unset/not set/ ?

> + */
> +static unsigned int uds_multiplier(int ratio)

Should the function be renamed to uds_pre_multiplier() ? And isn't it a
divisor ? :-)

> +{
> +	unsigned int mp = ratio / 4096;
> +
> +	return mp < 4 ? 1 : (mp < 8 ? 2 : 4);
> +}
> +
>  /*
>   * uds_output_size - Return the output size for an input size and scaling ratio
>   * @input: input size in pixels
> @@ -55,10 +67,7 @@ static unsigned int uds_output_size(unsigned int input, unsigned int ratio)
>  {
>  	if (ratio > 4096) {
>  		/* Down-scaling */
> -		unsigned int mp;
> -
> -		mp = ratio / 4096;
> -		mp = mp < 4 ? 1 : (mp < 8 ? 2 : 4);
> +		unsigned int mp = uds_multiplier(ratio);
>  
>  		return (input - 1) / mp * mp * 4096 / ratio + 1;
>  	} else {
> @@ -88,10 +97,7 @@ static unsigned int uds_passband_width(unsigned int ratio)
>  {
>  	if (ratio >= 4096) {
>  		/* Down-scaling */
> -		unsigned int mp;
> -
> -		mp = ratio / 4096;
> -		mp = mp < 4 ? 1 : (mp < 8 ? 2 : 4);
> +		unsigned int mp = uds_multiplier(ratio);
>  
>  		return 64 * 4096 * mp / ratio;
>  	} else {
Kieran Bingham May 10, 2019, 11:15 a.m. UTC | #2
Hi Laurent

On 18/04/2019 07:37, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Apr 11, 2019 at 05:12:55PM +0100, Kieran Bingham wrote:
>> The 'mp' value is used through many calculations in determining the scaling
>> factors of the UDS. Factor this out so that it can be reused in further
>> calculations, and also ensure that if the BLADV control is ever changed only a
>> single function needs to be modified.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/vsp1/vsp1_uds.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
>> index 27012af973b2..c71c24363d54 100644
>> --- a/drivers/media/platform/vsp1/vsp1_uds.c
>> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
>> @@ -46,6 +46,18 @@ void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_body *dlb,
>>  		       alpha << VI6_UDS_ALPVAL_VAL0_SHIFT);
>>  }
>>  
>> +/*
>> + * Determine the pre-filter multiplication value.
> 
> This would benefit from a more detailed description, and in particular a
> definition of what "pre-filter" means.

There is a pre-filter processing stage to the scaling filter. I think it
does some form of pixel binning. The specifics are not documented.

I could update this to:

/*
 * Determine the pre-filter binning divider
 *
 * The UDS uses a two stage filter scaler process. This determines the
 * rate at which pixels are reduced for large down-scaling ratios before
 * being fed into the bicubic filter.
 */

>> + *
>> + * This calculation assumes that the BLADV control is unset.
> 
> s/control/bit/ ?
> s/unset/not set/ ?
> 

Sure.

>> + */
>> +static unsigned int uds_multiplier(int ratio)
> 
> Should the function be renamed to uds_pre_multiplier() ? And isn't it a
> divisor ? :-)

Indeed in the pipeline, the component is used when  downscaling, and so
I believe it is part of a pre-filter divider.

How about: uds_binning_ratio() ?




>> +{
>> +	unsigned int mp = ratio / 4096;
>> +
>> +	return mp < 4 ? 1 : (mp < 8 ? 2 : 4);
>> +}
>> +
>>  /*
>>   * uds_output_size - Return the output size for an input size and scaling ratio
>>   * @input: input size in pixels
>> @@ -55,10 +67,7 @@ static unsigned int uds_output_size(unsigned int input, unsigned int ratio)
>>  {
>>  	if (ratio > 4096) {
>>  		/* Down-scaling */
>> -		unsigned int mp;
>> -
>> -		mp = ratio / 4096;
>> -		mp = mp < 4 ? 1 : (mp < 8 ? 2 : 4);
>> +		unsigned int mp = uds_multiplier(ratio);
>>  
>>  		return (input - 1) / mp * mp * 4096 / ratio + 1;
>>  	} else {
>> @@ -88,10 +97,7 @@ static unsigned int uds_passband_width(unsigned int ratio)
>>  {
>>  	if (ratio >= 4096) {
>>  		/* Down-scaling */
>> -		unsigned int mp;
>> -
>> -		mp = ratio / 4096;
>> -		mp = mp < 4 ? 1 : (mp < 8 ? 2 : 4);
>> +		unsigned int mp = uds_multiplier(ratio);
>>  
>>  		return 64 * 4096 * mp / ratio;
>>  	} else {
> 
--
Regards

Kieran

Patch
diff mbox series

diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index 27012af973b2..c71c24363d54 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -46,6 +46,18 @@  void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_body *dlb,
 		       alpha << VI6_UDS_ALPVAL_VAL0_SHIFT);
 }
 
+/*
+ * Determine the pre-filter multiplication value.
+ *
+ * This calculation assumes that the BLADV control is unset.
+ */
+static unsigned int uds_multiplier(int ratio)
+{
+	unsigned int mp = ratio / 4096;
+
+	return mp < 4 ? 1 : (mp < 8 ? 2 : 4);
+}
+
 /*
  * uds_output_size - Return the output size for an input size and scaling ratio
  * @input: input size in pixels
@@ -55,10 +67,7 @@  static unsigned int uds_output_size(unsigned int input, unsigned int ratio)
 {
 	if (ratio > 4096) {
 		/* Down-scaling */
-		unsigned int mp;
-
-		mp = ratio / 4096;
-		mp = mp < 4 ? 1 : (mp < 8 ? 2 : 4);
+		unsigned int mp = uds_multiplier(ratio);
 
 		return (input - 1) / mp * mp * 4096 / ratio + 1;
 	} else {
@@ -88,10 +97,7 @@  static unsigned int uds_passband_width(unsigned int ratio)
 {
 	if (ratio >= 4096) {
 		/* Down-scaling */
-		unsigned int mp;
-
-		mp = ratio / 4096;
-		mp = mp < 4 ? 1 : (mp < 8 ? 2 : 4);
+		unsigned int mp = uds_multiplier(ratio);
 
 		return 64 * 4096 * mp / ratio;
 	} else {