subject:"Re\: \[PATCH v1 3\/5\] \[media\] stm32\-dcmi\: crop sensor image to match user resolution"

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

2017-06-26 Thread Hugues FRUCHET



On 06/26/2017 12:07 PM, Hans Verkuil wrote:
> On 26/06/17 11:53, Hugues FRUCHET wrote:
>> Hi Hans, thanks for review.
>>
>> Reply below.
>>
>> BR
>> Hugues.
>>
>> On 06/22/2017 05:19 PM, Hans Verkuil wrote:
>>> On 06/22/2017 05:12 PM, Hugues Fruchet wrote:
 Add flexibility on supported resolutions by cropping sensor
 image to fit user resolution format request.

 Signed-off-by: Hugues Fruchet 
 ---
 drivers/media/platform/stm32/stm32-dcmi.c | 54 
 ++-
 1 file changed, 53 insertions(+), 1 deletion(-)

 diff --git a/drivers/media/platform/stm32/stm32-dcmi.c 
 b/drivers/media/platform/stm32/stm32-dcmi.c
 index 75d53aa..bc5e052 100644
 --- a/drivers/media/platform/stm32/stm32-dcmi.c
 +++ b/drivers/media/platform/stm32/stm32-dcmi.c
 @@ -131,6 +131,8 @@ struct stm32_dcmi {
struct v4l2_async_notifier  notifier;
struct dcmi_graph_entityentity;
struct v4l2_format  fmt;
 +  struct v4l2_rectcrop;
 +  booldo_crop;
 
const struct dcmi_format**user_formats;
unsigned intnum_user_formats;
 @@ -538,6 +540,27 @@ static int dcmi_start_streaming(struct vb2_queue *vq, 
 unsigned int count)
if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
val |= CR_PCKPOL;
 
 +  if (dcmi->do_crop) {
 +  u32 size, start;
 +
 +  /* Crop resolution */
 +  size = ((dcmi->crop.height - 1) << 16) |
 +  ((dcmi->crop.width << 1) - 1);
 +  reg_write(dcmi->regs, DCMI_CWSIZE, size);
 +
 +  /* Crop start point */
 +  start = ((dcmi->crop.top) << 16) |
 +   ((dcmi->crop.left << 1));
 +  reg_write(dcmi->regs, DCMI_CWSTRT, start);
 +
 +  dev_dbg(dcmi->dev, "Cropping to %ux%u@%u:%u\n",
 +  dcmi->crop.width, dcmi->crop.height,
 +  dcmi->crop.left, dcmi->crop.top);
 +
 +  /* Enable crop */
 +  val |= CR_CROP;
 +  };
 +
reg_write(dcmi->regs, DCMI_CR, val);
 
/* Enable dcmi */
 @@ -707,6 +730,8 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, 
 struct v4l2_format *f,
.which = V4L2_SUBDEV_FORMAT_TRY,
};
int ret;
 +  __u32 width, height;
 +  struct v4l2_mbus_framefmt *mf = 
 
dcmi_fmt = find_format_by_fourcc(dcmi, pixfmt->pixelformat);
if (!dcmi_fmt) {
 @@ -724,8 +749,18 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, 
 struct v4l2_format *f,
if (ret < 0)
return ret;
 
 +  /* Align format on what sensor can do */
 +  width = pixfmt->width;
 +  height = pixfmt->height;
v4l2_fill_pix_format(pixfmt, );
 
 +  /* We can do any resolution thanks to crop */
 +  if ((mf->width > width) || (mf->height > height)) {
 +  /* Restore width/height */
 +  pixfmt->width = width;
 +  pixfmt->height = height;
 +  };
 +
pixfmt->field = V4L2_FIELD_NONE;
pixfmt->bytesperline = pixfmt->width * dcmi_fmt->bpp;
pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
 @@ -741,6 +776,8 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, 
 struct v4l2_format *f)
struct v4l2_subdev_format format = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
};
 +  struct v4l2_mbus_framefmt *mf = 
 +  struct v4l2_pix_format *pixfmt = >fmt.pix;
const struct dcmi_format *current_fmt;
int ret;
 
 @@ -748,13 +785,28 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, 
 struct v4l2_format *f)
if (ret)
return ret;
 
 -  v4l2_fill_mbus_format(, >fmt.pix,
 +  v4l2_fill_mbus_format(, pixfmt,
  current_fmt->mbus_code);
ret = v4l2_subdev_call(dcmi->entity.subdev, pad,
   set_fmt, NULL, );
if (ret < 0)
return ret;
 
 +  /* Enable crop if sensor resolution is larger than request */
 +  dcmi->do_crop = false;
 +  if ((mf->width > pixfmt->width) || (mf->height > pixfmt->height)) {
 +  dcmi->crop.width = pixfmt->width;
 +  dcmi->crop.height = pixfmt->height;
 +  dcmi->crop.left = (mf->width - pixfmt->width) / 2;
 +  dcmi->crop.top = (mf->height - pixfmt->height) / 2;
 +  dcmi->do_crop = true;
>>>
>>> Why not implement the

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

2017-06-26 Thread Hans Verkuil

On 26/06/17 11:53, Hugues FRUCHET wrote:
> Hi Hans, thanks for review.
> 
> Reply below.
> 
> BR
> Hugues.
> 
> On 06/22/2017 05:19 PM, Hans Verkuil wrote:
>> On 06/22/2017 05:12 PM, Hugues Fruchet wrote:
>>> Add flexibility on supported resolutions by cropping sensor
>>> image to fit user resolution format request.
>>>
>>> Signed-off-by: Hugues Fruchet 
>>> ---
>>>drivers/media/platform/stm32/stm32-dcmi.c | 54 
>>> ++-
>>>1 file changed, 53 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c 
>>> b/drivers/media/platform/stm32/stm32-dcmi.c
>>> index 75d53aa..bc5e052 100644
>>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>>> @@ -131,6 +131,8 @@ struct stm32_dcmi {
>>> struct v4l2_async_notifier  notifier;
>>> struct dcmi_graph_entityentity;
>>> struct v4l2_format  fmt;
>>> +   struct v4l2_rectcrop;
>>> +   booldo_crop;
>>>
>>> const struct dcmi_format**user_formats;
>>> unsigned intnum_user_formats;
>>> @@ -538,6 +540,27 @@ static int dcmi_start_streaming(struct vb2_queue *vq, 
>>> unsigned int count)
>>> if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>>> val |= CR_PCKPOL;
>>>
>>> +   if (dcmi->do_crop) {
>>> +   u32 size, start;
>>> +
>>> +   /* Crop resolution */
>>> +   size = ((dcmi->crop.height - 1) << 16) |
>>> +   ((dcmi->crop.width << 1) - 1);
>>> +   reg_write(dcmi->regs, DCMI_CWSIZE, size);
>>> +
>>> +   /* Crop start point */
>>> +   start = ((dcmi->crop.top) << 16) |
>>> +((dcmi->crop.left << 1));
>>> +   reg_write(dcmi->regs, DCMI_CWSTRT, start);
>>> +
>>> +   dev_dbg(dcmi->dev, "Cropping to %ux%u@%u:%u\n",
>>> +   dcmi->crop.width, dcmi->crop.height,
>>> +   dcmi->crop.left, dcmi->crop.top);
>>> +
>>> +   /* Enable crop */
>>> +   val |= CR_CROP;
>>> +   };
>>> +
>>> reg_write(dcmi->regs, DCMI_CR, val);
>>>
>>> /* Enable dcmi */
>>> @@ -707,6 +730,8 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct 
>>> v4l2_format *f,
>>> .which = V4L2_SUBDEV_FORMAT_TRY,
>>> };
>>> int ret;
>>> +   __u32 width, height;
>>> +   struct v4l2_mbus_framefmt *mf = 
>>>
>>> dcmi_fmt = find_format_by_fourcc(dcmi, pixfmt->pixelformat);
>>> if (!dcmi_fmt) {
>>> @@ -724,8 +749,18 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, 
>>> struct v4l2_format *f,
>>> if (ret < 0)
>>> return ret;
>>>
>>> +   /* Align format on what sensor can do */
>>> +   width = pixfmt->width;
>>> +   height = pixfmt->height;
>>> v4l2_fill_pix_format(pixfmt, );
>>>
>>> +   /* We can do any resolution thanks to crop */
>>> +   if ((mf->width > width) || (mf->height > height)) {
>>> +   /* Restore width/height */
>>> +   pixfmt->width = width;
>>> +   pixfmt->height = height;
>>> +   };
>>> +
>>> pixfmt->field = V4L2_FIELD_NONE;
>>> pixfmt->bytesperline = pixfmt->width * dcmi_fmt->bpp;
>>> pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
>>> @@ -741,6 +776,8 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct 
>>> v4l2_format *f)
>>> struct v4l2_subdev_format format = {
>>> .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>> };
>>> +   struct v4l2_mbus_framefmt *mf = 
>>> +   struct v4l2_pix_format *pixfmt = >fmt.pix;
>>> const struct dcmi_format *current_fmt;
>>> int ret;
>>>
>>> @@ -748,13 +785,28 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, 
>>> struct v4l2_format *f)
>>> if (ret)
>>> return ret;
>>>
>>> -   v4l2_fill_mbus_format(, >fmt.pix,
>>> +   v4l2_fill_mbus_format(, pixfmt,
>>>   current_fmt->mbus_code);
>>> ret = v4l2_subdev_call(dcmi->entity.subdev, pad,
>>>set_fmt, NULL, );
>>> if (ret < 0)
>>> return ret;
>>>
>>> +   /* Enable crop if sensor resolution is larger than request */
>>> +   dcmi->do_crop = false;
>>> +   if ((mf->width > pixfmt->width) || (mf->height > pixfmt->height)) {
>>> +   dcmi->crop.width = pixfmt->width;
>>> +   dcmi->crop.height = pixfmt->height;
>>> +   dcmi->crop.left = (mf->width - pixfmt->width) / 2;
>>> +   dcmi->crop.top = (mf->height - pixfmt->height) / 2;
>>> +   dcmi->do_crop = true;
>>
>> Why not implement the selection API instead? I assume that you can crop from 
>> any
>> region of the sensor, not just the center part.
> 
> The point here was to add some flexibility for user in term of 
> resolution and also less memory consumption.
> For example here I want to make a 480x272 preview:
> - without this change: S_FMT(480x272) returns VGA (the OV9655 larger 
>

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

2017-06-26 Thread Hugues FRUCHET

Hi Hans, thanks for review.

Reply below.

BR
Hugues.

On 06/22/2017 05:19 PM, Hans Verkuil wrote:
> On 06/22/2017 05:12 PM, Hugues Fruchet wrote:
>> Add flexibility on supported resolutions by cropping sensor
>> image to fit user resolution format request.
>>
>> Signed-off-by: Hugues Fruchet 
>> ---
>>drivers/media/platform/stm32/stm32-dcmi.c | 54 
>> ++-
>>1 file changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c 
>> b/drivers/media/platform/stm32/stm32-dcmi.c
>> index 75d53aa..bc5e052 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -131,6 +131,8 @@ struct stm32_dcmi {
>>  struct v4l2_async_notifier  notifier;
>>  struct dcmi_graph_entityentity;
>>  struct v4l2_format  fmt;
>> +struct v4l2_rectcrop;
>> +booldo_crop;
>>
>>  const struct dcmi_format**user_formats;
>>  unsigned intnum_user_formats;
>> @@ -538,6 +540,27 @@ static int dcmi_start_streaming(struct vb2_queue *vq, 
>> unsigned int count)
>>  if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>>  val |= CR_PCKPOL;
>>
>> +if (dcmi->do_crop) {
>> +u32 size, start;
>> +
>> +/* Crop resolution */
>> +size = ((dcmi->crop.height - 1) << 16) |
>> +((dcmi->crop.width << 1) - 1);
>> +reg_write(dcmi->regs, DCMI_CWSIZE, size);
>> +
>> +/* Crop start point */
>> +start = ((dcmi->crop.top) << 16) |
>> + ((dcmi->crop.left << 1));
>> +reg_write(dcmi->regs, DCMI_CWSTRT, start);
>> +
>> +dev_dbg(dcmi->dev, "Cropping to %ux%u@%u:%u\n",
>> +dcmi->crop.width, dcmi->crop.height,
>> +dcmi->crop.left, dcmi->crop.top);
>> +
>> +/* Enable crop */
>> +val |= CR_CROP;
>> +};
>> +
>>  reg_write(dcmi->regs, DCMI_CR, val);
>>
>>  /* Enable dcmi */
>> @@ -707,6 +730,8 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct 
>> v4l2_format *f,
>>  .which = V4L2_SUBDEV_FORMAT_TRY,
>>  };
>>  int ret;
>> +__u32 width, height;
>> +struct v4l2_mbus_framefmt *mf = 
>>
>>  dcmi_fmt = find_format_by_fourcc(dcmi, pixfmt->pixelformat);
>>  if (!dcmi_fmt) {
>> @@ -724,8 +749,18 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct 
>> v4l2_format *f,
>>  if (ret < 0)
>>  return ret;
>>
>> +/* Align format on what sensor can do */
>> +width = pixfmt->width;
>> +height = pixfmt->height;
>>  v4l2_fill_pix_format(pixfmt, );
>>
>> +/* We can do any resolution thanks to crop */
>> +if ((mf->width > width) || (mf->height > height)) {
>> +/* Restore width/height */
>> +pixfmt->width = width;
>> +pixfmt->height = height;
>> +};
>> +
>>  pixfmt->field = V4L2_FIELD_NONE;
>>  pixfmt->bytesperline = pixfmt->width * dcmi_fmt->bpp;
>>  pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
>> @@ -741,6 +776,8 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct 
>> v4l2_format *f)
>>  struct v4l2_subdev_format format = {
>>  .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>  };
>> +struct v4l2_mbus_framefmt *mf = 
>> +struct v4l2_pix_format *pixfmt = >fmt.pix;
>>  const struct dcmi_format *current_fmt;
>>  int ret;
>>
>> @@ -748,13 +785,28 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, 
>> struct v4l2_format *f)
>>  if (ret)
>>  return ret;
>>
>> -v4l2_fill_mbus_format(, >fmt.pix,
>> +v4l2_fill_mbus_format(, pixfmt,
>>current_fmt->mbus_code);
>>  ret = v4l2_subdev_call(dcmi->entity.subdev, pad,
>> set_fmt, NULL, );
>>  if (ret < 0)
>>  return ret;
>>
>> +/* Enable crop if sensor resolution is larger than request */
>> +dcmi->do_crop = false;
>> +if ((mf->width > pixfmt->width) || (mf->height > pixfmt->height)) {
>> +dcmi->crop.width = pixfmt->width;
>> +dcmi->crop.height = pixfmt->height;
>> +dcmi->crop.left = (mf->width - pixfmt->width) / 2;
>> +dcmi->crop.top = (mf->height - pixfmt->height) / 2;
>> +dcmi->do_crop = true;
> 
> Why not implement the selection API instead? I assume that you can crop from 
> any
> region of the sensor, not just the center part.

The point here was to add some flexibility for user in term of 
resolution and also less memory consumption.
For example here I want to make a 480x272 preview:
- without this change: S_FMT(480x272) returns VGA (the OV9655 larger 
discrete resolution), then app has to capture VGA frames then crop to 
fit 480x272 frame buffer.
- with this change:

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

2017-06-25 Thread kbuild test robot

Hi Hugues,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on next-20170623]
[cannot apply to v4.12-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Hugues-Fruchet/Camera-support-on-STM32F746G-DISCO-board/20170625-204425
base:   git://linuxtv.org/media_tree.git master


coccinelle warnings: (new ones prefixed by >>)

>> drivers/media/platform/stm32/stm32-dcmi.c:808:2-3: Unneeded semicolon
   drivers/media/platform/stm32/stm32-dcmi.c:562:2-3: Unneeded semicolon
   drivers/media/platform/stm32/stm32-dcmi.c:762:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

2017-06-22 Thread Hans Verkuil


On 06/22/2017 05:12 PM, Hugues Fruchet wrote:

Add flexibility on supported resolutions by cropping sensor
image to fit user resolution format request.

Signed-off-by: Hugues Fruchet 
---
  drivers/media/platform/stm32/stm32-dcmi.c | 54 ++-
  1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c 
b/drivers/media/platform/stm32/stm32-dcmi.c
index 75d53aa..bc5e052 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -131,6 +131,8 @@ struct stm32_dcmi {
struct v4l2_async_notifier  notifier;
struct dcmi_graph_entityentity;
struct v4l2_format  fmt;
+   struct v4l2_rectcrop;
+   booldo_crop;
  
  	const struct dcmi_format	**user_formats;

unsigned intnum_user_formats;
@@ -538,6 +540,27 @@ static int dcmi_start_streaming(struct vb2_queue *vq, 
unsigned int count)
if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
val |= CR_PCKPOL;
  
+	if (dcmi->do_crop) {

+   u32 size, start;
+
+   /* Crop resolution */
+   size = ((dcmi->crop.height - 1) << 16) |
+   ((dcmi->crop.width << 1) - 1);
+   reg_write(dcmi->regs, DCMI_CWSIZE, size);
+
+   /* Crop start point */
+   start = ((dcmi->crop.top) << 16) |
+((dcmi->crop.left << 1));
+   reg_write(dcmi->regs, DCMI_CWSTRT, start);
+
+   dev_dbg(dcmi->dev, "Cropping to %ux%u@%u:%u\n",
+   dcmi->crop.width, dcmi->crop.height,
+   dcmi->crop.left, dcmi->crop.top);
+
+   /* Enable crop */
+   val |= CR_CROP;
+   };
+
reg_write(dcmi->regs, DCMI_CR, val);
  
  	/* Enable dcmi */

@@ -707,6 +730,8 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct 
v4l2_format *f,
.which = V4L2_SUBDEV_FORMAT_TRY,
};
int ret;
+   __u32 width, height;
+   struct v4l2_mbus_framefmt *mf = 
  
  	dcmi_fmt = find_format_by_fourcc(dcmi, pixfmt->pixelformat);

if (!dcmi_fmt) {
@@ -724,8 +749,18 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct 
v4l2_format *f,
if (ret < 0)
return ret;
  
+	/* Align format on what sensor can do */

+   width = pixfmt->width;
+   height = pixfmt->height;
v4l2_fill_pix_format(pixfmt, );
  
+	/* We can do any resolution thanks to crop */

+   if ((mf->width > width) || (mf->height > height)) {
+   /* Restore width/height */
+   pixfmt->width = width;
+   pixfmt->height = height;
+   };
+
pixfmt->field = V4L2_FIELD_NONE;
pixfmt->bytesperline = pixfmt->width * dcmi_fmt->bpp;
pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
@@ -741,6 +776,8 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct 
v4l2_format *f)
struct v4l2_subdev_format format = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
};
+   struct v4l2_mbus_framefmt *mf = 
+   struct v4l2_pix_format *pixfmt = >fmt.pix;
const struct dcmi_format *current_fmt;
int ret;
  
@@ -748,13 +785,28 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)

if (ret)
return ret;
  
-	v4l2_fill_mbus_format(, >fmt.pix,

+   v4l2_fill_mbus_format(, pixfmt,
  current_fmt->mbus_code);
ret = v4l2_subdev_call(dcmi->entity.subdev, pad,
   set_fmt, NULL, );
if (ret < 0)
return ret;
  
+	/* Enable crop if sensor resolution is larger than request */

+   dcmi->do_crop = false;
+   if ((mf->width > pixfmt->width) || (mf->height > pixfmt->height)) {
+   dcmi->crop.width = pixfmt->width;
+   dcmi->crop.height = pixfmt->height;
+   dcmi->crop.left = (mf->width - pixfmt->width) / 2;
+   dcmi->crop.top = (mf->height - pixfmt->height) / 2;
+   dcmi->do_crop = true;


Why not implement the selection API instead? I assume that you can crop from any
region of the sensor, not just the center part.

Regards,

Hans


+
+   dev_dbg(dcmi->dev, "%ux%u cropped to %ux%u@(%u,%u)\n",
+   mf->width, mf->height,
+   dcmi->crop.width, dcmi->crop.height,
+   dcmi->crop.left, dcmi->crop.top);
+   };
+
dcmi->fmt = *f;
dcmi->current_fmt = current_fmt;

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution

5 matches

Site Navigation

Mail list logo

Footer information