Hi Mr.Sakari
On Sun, Mar 18, 2012 at 12:27 AM, Sakari Ailus <[email protected]> wrote:
> Hi Ajay,
>
> Thanks for the patch. I have a few comments below.
>
> On Sat, Mar 17, 2012 at 04:52:14PM +0530, Ajay Kumar wrote:
>> Modify the G2D driver(which initially supported only FIMG2D v3 style H/W)
>> to support FIMG2D v41 style hardware present on Exynos4x12 and Exynos52x0
>> SOC.
>>
>> -- Set the SRC and DST type to 'memory' instead of using reset values.
>> -- FIMG2D v41 H/W uses different logic for stretching(scaling).
>> -- Use CACHECTL_REG only with FIMG2D v3.
>>
>> Signed-off-by: Ajay Kumar <[email protected]>
>> ---
>> drivers/media/video/s5p-g2d/g2d-hw.c | 39
>> ++++++++++++++++++++++++++++---
>> drivers/media/video/s5p-g2d/g2d-regs.h | 6 +++++
>> drivers/media/video/s5p-g2d/g2d.c | 23 +++++++++++++++++-
>> drivers/media/video/s5p-g2d/g2d.h | 9 ++++++-
>> 4 files changed, 70 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/video/s5p-g2d/g2d-hw.c
>> b/drivers/media/video/s5p-g2d/g2d-hw.c
>> index 5b86cbe..f8225b8 100644
>> --- a/drivers/media/video/s5p-g2d/g2d-hw.c
>> +++ b/drivers/media/video/s5p-g2d/g2d-hw.c
>> @@ -28,6 +28,8 @@ void g2d_set_src_size(struct g2d_dev *d, struct g2d_frame
>> *f)
>> {
>> u32 n;
>>
>> + w(0, SRC_SELECT_REG);
>> +
>> w(f->stride & 0xFFFF, SRC_STRIDE_REG);
>>
>> n = f->o_height & 0xFFF;
>> @@ -52,6 +54,8 @@ void g2d_set_dst_size(struct g2d_dev *d, struct g2d_frame
>> *f)
>> {
>> u32 n;
>>
>> + w(0, DST_SELECT_REG);
>> +
>> w(f->stride & 0xFFFF, DST_STRIDE_REG);
>>
>> n = f->o_height & 0xFFF;
>> @@ -82,10 +86,36 @@ void g2d_set_flip(struct g2d_dev *d, u32 r)
>> w(r, SRC_MSK_DIRECT_REG);
>> }
>>
>> -u32 g2d_cmd_stretch(u32 e)
>> +/**
>> + * g2d_calc_scale_factor - convert scale factor to fixed pint 16
>
> Point, perhaps?
Ok. Typo.
>> + * @n: numerator
>> + * @d: denominator
>> + */
>> +static unsigned long g2d_calc_scale_factor(int n, int d)
>> +{
>> + return (n << 16) / d;
>> +}
>> +
>> +void g2d_set_v41_stretch(struct g2d_dev *d, struct g2d_frame *src,
>> + struct g2d_frame *dst)
>> {
>> - e &= 1;
>> - return e << 4;
>> + int src_w, src_h, dst_w, dst_h;
>
> Is int intentional --- width and height usually can't be negative.
I will use 'unsigned int'.
>> + u32 wcfg, hcfg;
>> +
>> + w(DEFAULT_SCALE_MODE, SRC_SCALE_CTRL_REG);
>> +
>> + src_w = src->c_width;
>> + src_h = src->c_height;
>> +
>> + dst_w = dst->c_width;
>> + dst_h = dst->c_height;
>> +
>> + /* inversed scaling factor: src is numerator */
>> + wcfg = g2d_calc_scale_factor(src_w, dst_w);
>> + hcfg = g2d_calc_scale_factor(src_h, dst_h);
>
> I think this would be more simple without that many temporary variables and
> an extra function.
You are right. I will Change it.
>> + w(wcfg, SRC_XSCALE_REG);
>> + w(hcfg, SRC_YSCALE_REG);
>> }
>>
>> void g2d_set_cmd(struct g2d_dev *d, u32 c)
>> @@ -96,7 +126,8 @@ void g2d_set_cmd(struct g2d_dev *d, u32 c)
>> void g2d_start(struct g2d_dev *d)
>> {
>> /* Clear cache */
>> - w(0x7, CACHECTL_REG);
>> + if (d->device_type == TYPE_G2D_3X)
>> + w(0x7, CACHECTL_REG);
>> /* Enable interrupt */
>> w(1, INTEN_REG);
>> /* Start G2D engine */
>
> Kind regards,
>
> --
> Sakari Ailus
> e-mail: [email protected] jabber/XMPP/Gmail: [email protected]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for your review and comments. I will resubmit the patch with changes.
Regards,
Ajay
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
