From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v2 5/6] media: Add controls for jpeg quantization tables Date: Fri, 17 Aug 2018 11:10:00 +0900 Message-ID: References: <20180802200010.24365-1-ezequiel@collabora.com> <20180802200010.24365-6-ezequiel@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180802200010.24365-6-ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Ezequiel Garcia , Hans Verkuil Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kernel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org, "open list:ARM/Rockchip SoC..." , Rob Herring , Nicolas Dufresne , Shunqian Zheng , Linux Media Mailing List List-Id: devicetree@vger.kernel.org Hi Ezequiel, On Fri, Aug 3, 2018 at 5:00 AM Ezequiel Garcia wrote: > > From: Shunqian Zheng > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > configure the JPEG quantization tables. > > Signed-off-by: Shunqian Zheng > Signed-off-by: Ezequiel Garcia > --- > Documentation/media/uapi/v4l/extended-controls.rst | 9 +++++++++ > drivers/media/v4l2-core/v4l2-ctrls.c | 4 ++++ > include/uapi/linux/v4l2-controls.h | 3 +++ > 3 files changed, 16 insertions(+) Thanks for this series and sorry for being late with review. Please see my comments inline. > > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst > index 9f7312bf3365..80e26f81900b 100644 > --- a/Documentation/media/uapi/v4l/extended-controls.rst > +++ b/Documentation/media/uapi/v4l/extended-controls.rst > @@ -3354,6 +3354,15 @@ JPEG Control IDs > Specify which JPEG markers are included in compressed stream. This > control is valid only for encoders. > > +.. _jpeg-quant-tables-control: > + > +``V4L2_CID_JPEG_LUMA_QUANTIZATION (__u8 matrix)`` > + Sets the luma quantization table to be used for encoding > + or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is > + expected to be in JPEG zigzag order, as per the JPEG specification. Should we also specify this to be 8x8? > + > +``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)`` > + Sets the chroma quantization table. > nit: I guess we aff something like "See also V4L2_CID_JPEG_LUMA_QUANTIZATION for details." to avoid repeating the V4L2_PIX_FMT_JPEG_RAW and zigzag order bits? Or maybe just repeating is better? > > .. flat-table:: > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 599c1cbff3b9..5c62c3101851 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -999,6 +999,8 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_JPEG_RESTART_INTERVAL: return "Restart Interval"; > case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality"; > case V4L2_CID_JPEG_ACTIVE_MARKER: return "Active Markers"; > + case V4L2_CID_JPEG_LUMA_QUANTIZATION: return "Luminance Quantization Matrix"; > + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: return "Chrominance Quantization Matrix"; > > /* Image source controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > @@ -1284,6 +1286,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > *flags |= V4L2_CTRL_FLAG_READ_ONLY; > break; > case V4L2_CID_DETECT_MD_REGION_GRID: > + case V4L2_CID_JPEG_LUMA_QUANTIZATION: > + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: It looks like with this setup, the driver has to explicitly set dims to { 8, 8 } and min/max to 0/255. At least for min and max, we could set them here. For dims, i don't see it handled in generic code, so I guess we can leave it to the driver now and add move into generic code, if another driver shows up. Hans, what do you think? Best regards, Tomasz