[2/2] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC
diff mbox series

Message ID 20200604190015.12069-2-dafna.hirschfeld@collabora.com
State New
Headers show
Series
  • [1/2] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
Related show

Commit Message

Dafna Hirschfeld June 4, 2020, 7 p.m. UTC
When setting the sink format of the 'rkisp1_resizer'
the format should be supported by 'rkisp1_isp' on
the video source pad. This patch checks this condition
and set the format to default if the condition is false.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h  | 4 ++++
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 4 ----
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Comments

Helen Koike June 4, 2020, 7:07 p.m. UTC | #1
On 6/4/20 4:00 PM, Dafna Hirschfeld wrote:
> When setting the sink format of the 'rkisp1_resizer'
> the format should be supported by 'rkisp1_isp' on
> the video source pad. This patch checks this condition
> and set the format to default if the condition is false.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h  | 4 ++++
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 4 ----
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 0c4fe503adc9..39d8e46d8d8a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -22,6 +22,10 @@
>  #include "rkisp1-regs.h"
>  #include "uapi/rkisp1-config.h"
>  
> +#define RKISP1_DIR_SRC BIT(0)
> +#define RKISP1_DIR_SINK BIT(1)
> +#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)

I think we can rename this macro to state explicitly it is the direction from the rkisp1_isp subdevice node
point of view.

Maybe prefix it with RKISP1_ISP_SD_ ?

Thanks
Helen

> +
>  #define RKISP1_ISP_MAX_WIDTH		4032
>  #define RKISP1_ISP_MAX_HEIGHT		3024
>  #define RKISP1_ISP_MIN_WIDTH		32
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index dc2b59a0160a..e66e87d6ea8b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -23,10 +23,6 @@
>  
>  #define RKISP1_ISP_DEV_NAME	RKISP1_DRIVER_NAME "_isp"
>  
> -#define RKISP1_DIR_SRC BIT(0)
> -#define RKISP1_DIR_SINK BIT(1)
> -#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
> -
>  /*
>   * NOTE: MIPI controller and input MUX are also configured in this file.
>   * This is because ISP Subdev describes not only ISP submodule (input size,
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index d64c064bdb1d..fa28f4bd65c0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -542,7 +542,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
>  					    which);
>  	sink_fmt->code = format->code;
>  	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
> -	if (!mbus_info) {
> +	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SRC)) {
>  		sink_fmt->code = RKISP1_DEF_FMT;
>  		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
>  	}
>
Dafna Hirschfeld June 8, 2020, 5:50 p.m. UTC | #2
On 04.06.20 21:07, Helen Koike wrote:
> 
> 
> On 6/4/20 4:00 PM, Dafna Hirschfeld wrote:
>> When setting the sink format of the 'rkisp1_resizer'
>> the format should be supported by 'rkisp1_isp' on
>> the video source pad. This patch checks this condition
>> and set the format to default if the condition is false.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-common.h  | 4 ++++
>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 4 ----
>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 2 +-
>>   3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 0c4fe503adc9..39d8e46d8d8a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -22,6 +22,10 @@
>>   #include "rkisp1-regs.h"
>>   #include "uapi/rkisp1-config.h"
>>   
>> +#define RKISP1_DIR_SRC BIT(0)
>> +#define RKISP1_DIR_SINK BIT(1)
>> +#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
> 
> I think we can rename this macro to state explicitly it is the direction from the rkisp1_isp subdevice node
> point of view.
> 
> Maybe prefix it with RKISP1_ISP_SD_ ?

yes, it's a better name.

> 
> Thanks
> Helen
> 
>> +
>>   #define RKISP1_ISP_MAX_WIDTH		4032
>>   #define RKISP1_ISP_MAX_HEIGHT		3024
>>   #define RKISP1_ISP_MIN_WIDTH		32
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index dc2b59a0160a..e66e87d6ea8b 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -23,10 +23,6 @@
>>   
>>   #define RKISP1_ISP_DEV_NAME	RKISP1_DRIVER_NAME "_isp"
>>   
>> -#define RKISP1_DIR_SRC BIT(0)
>> -#define RKISP1_DIR_SINK BIT(1)
>> -#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
>> -
>>   /*
>>    * NOTE: MIPI controller and input MUX are also configured in this file.
>>    * This is because ISP Subdev describes not only ISP submodule (input size,
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> index d64c064bdb1d..fa28f4bd65c0 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> @@ -542,7 +542,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
>>   					    which);
>>   	sink_fmt->code = format->code;
>>   	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
>> -	if (!mbus_info) {
>> +	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SRC)) {

I also think the field 'direction' better change to isp_pads_flags.
It indicates better what this field is.

Thanks,
Dafna

>>   		sink_fmt->code = RKISP1_DEF_FMT;
>>   		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
>>   	}
>>

Patch
diff mbox series

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 0c4fe503adc9..39d8e46d8d8a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -22,6 +22,10 @@ 
 #include "rkisp1-regs.h"
 #include "uapi/rkisp1-config.h"
 
+#define RKISP1_DIR_SRC BIT(0)
+#define RKISP1_DIR_SINK BIT(1)
+#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
+
 #define RKISP1_ISP_MAX_WIDTH		4032
 #define RKISP1_ISP_MAX_HEIGHT		3024
 #define RKISP1_ISP_MIN_WIDTH		32
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index dc2b59a0160a..e66e87d6ea8b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -23,10 +23,6 @@ 
 
 #define RKISP1_ISP_DEV_NAME	RKISP1_DRIVER_NAME "_isp"
 
-#define RKISP1_DIR_SRC BIT(0)
-#define RKISP1_DIR_SINK BIT(1)
-#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
-
 /*
  * NOTE: MIPI controller and input MUX are also configured in this file.
  * This is because ISP Subdev describes not only ISP submodule (input size,
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index d64c064bdb1d..fa28f4bd65c0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -542,7 +542,7 @@  static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 					    which);
 	sink_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
-	if (!mbus_info) {
+	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SRC)) {
 		sink_fmt->code = RKISP1_DEF_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	}