linux-kernel - Re: [PATCH v4 3/4] gpu: ipu-ic: Add complete image conversion support with tiling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <be09a9dc-96f0-07f6-a527-1c144105af79@mentor.com>
Date:   Sat, 17 Sep 2016 11:46:23 -0700
From:   Steve Longerbeam <steve_longerbeam@...tor.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
CC:     Steve Longerbeam <slongerbeam@...il.com>, <plagnioj@...osoft.com>,
        <tomi.valkeinen@...com>, <dri-devel@...ts.freedesktop.org>,
        <linux-kernel@...r.kernel.org>, <linux-fbdev@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] gpu: ipu-ic: Add complete image conversion support
 with tiling



On 09/16/2016 07:16 AM, Philipp Zabel wrote:
> Hi Steve,
>
> thanks for the update.
>
> Am Mittwoch, den 14.09.2016, 18:45 -0700 schrieb Steve Longerbeam:
>> I added comment headers for all the image conversion prototypes.
>> It caused bloat in imx-ipu-v3.h, so I moved it to a new header:
>> include/video/imx-image-convert.h, but let me know if we should put
>> this somewhere else and/or under Documentation/ somewhere.
> I think that is the right place already. imx-image-convert.h could be
> renamed to imx-ipu-image-convert.h, to make clear that this is about the
> IPU image converter.

Ok, I'll send another update with the name change in the next
version (v7).
>
>>>> +#define MIN_W     128
>>>> +#define MIN_H     128
>>> Where does this minimum come from?
>> Nowhere really :) This is just some sane minimums, to pass
>> to clamp_align() when aligning input/output width/height in
>> ipu_image_convert_adjust().
> Let's use hardware minimum in the low level code. Sane defaults are for
> the V4L2 API. Would that be 8x2 pixels per input tile?

I searched the imx6 reference manual, I can't find any mention
of width/height minimums for the IC. So I suppose 8x2 would be fine,
or maybe 16x8, to account for planar and IRT conversions.

>
>>>> +	if (ipu_rot_mode_is_irt(ctx->rot_mode)) {
>>>> +		/* this is a rotation operation, just ignore */
>>>> +		spin_unlock_irqrestore(&cvt->irqlock, flags);
>>>> +		return IRQ_HANDLED;
>>>> +	}
>>> Why enable the out_chan EOF irq at all when using the IRT mode?
>> Because (see above), all the IPU resources that might be needed
>> for any conversion context that is queued to a image conversion
>> channel (IC task) are acquired when the first context is queued,
>> including rotation resources. So by acquiring the non-rotation EOF
>> irq, it will get fielded even for rotation conversions, so we have to
>> handle it.
> There is nothing wrong with acquiring the irq. It could still be
> disabled while it is not needed.

It would be difficult to disable the irq. Remember the irq handlers must
field all EOF interrupts in an ipu_image_convert_chan (IC task). If one
context in that channel disables the irq, it will break other runnings
contexts in that channel that are using it.

>
>>>> +/* Adjusts input/output images to IPU restrictions */
>>>> +int ipu_image_convert_adjust(struct ipu_image *in, struct ipu_image *out,
>>>> +			     enum ipu_rotate_mode rot_mode)
>>>> +{
>>>> +	const struct ipu_ic_pixfmt *infmt, *outfmt;
>>>> +	unsigned int num_in_rows, num_in_cols;
>>>> +	unsigned int num_out_rows, num_out_cols;
>>>> +	u32 w_align, h_align;
>>>> +
>>>> +	infmt = ipu_ic_get_format(in->pix.pixelformat);
>>>> +	outfmt = ipu_ic_get_format(out->pix.pixelformat);
>>>> +
>>>> +	/* set some defaults if needed */
>>> Is this our task at all?
>> ipu_image_convert_adjust() is meant to be called by v4l2 try_format(),
>> which should never return EINVAL but should return a supported format
>> when the passed format is not supported. So I added this here to return
>> some default pixel formats and width/heights if needed.
> I'd prefer to move this into the mem2mem driver try_format, then.

We could move the 0 width/height checks to v4l2, but the pixel
format defaults should probably remain in ipu-image-convert, since
it knows what formats it supports converting to/from.

Steve