Re: [PATCH v3 2/4] [media] exynos-scaler: Add core functionality for the SCALER driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester,

Thanks for the comments.

On Mon, Sep 16, 2013 at 3:58 AM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi Shaik,
>
> Thanks for addressing my comments, it really looks much better now.
> I have few more comments, mostly minor issues.

[...]

>> +
>> +const struct scaler_fmt *scaler_find_fmt(u32 *pixelformat,
>> +                               u32 *mbus_code, u32 index)
>> +{
>> +       const struct scaler_fmt *fmt, *def_fmt = NULL;
>> +       unsigned int i;
>> +
>> +       if (index>= ARRAY_SIZE(scaler_formats))
>> +               return NULL;
>> +
>> +       for (i = 0; i<  ARRAY_SIZE(scaler_formats); ++i) {
>> +               fmt = scaler_get_format(i);
>> +               if (pixelformat&&  fmt->pixelformat == *pixelformat)
>> +                       return fmt;
>
>
>> +               if (mbus_code&&  fmt->mbus_code == *mbus_code)
>> +                       return fmt;
>
>
> is mbus_code ever used ?

Yes. Currently not used. Will remove this field for now..

>
>> +               if (index == i)
>> +                       def_fmt = fmt;
>> +       }
>> +
>> +       return def_fmt;
>> +}

[...]

>> +
>> +int scaler_try_fmt_mplane(struct scaler_ctx *ctx, struct v4l2_format *f)
>> +{
>> +       struct scaler_dev *scaler = ctx->scaler_dev;
>> +       struct device *dev =&scaler->pdev->dev;
>>
>> +       struct scaler_variant *variant = scaler->variant;
>> +       struct v4l2_pix_format_mplane *pix_mp =&f->fmt.pix_mp;
>>

[...]

>> +
>> +       /*
>> +        * Nothing mentioned about the colorspace in SCALER. Default value
>> is
>> +        * set to V4L2_COLORSPACE_REC709.
>> +        */
>
>
> Isn't scaler_hw_set_csc_coef() function configuring the colorspace ?

Actually speaking this function should do the color space setting part.
What the SCALER ip supports is CSC offset value for Y

YCbCr to RGB : Zero offset of -16 offset for input
RGB to YCbCr : Zero offset of +16 offset for output

I think user should provide this information through some controls.
Anyways, will take it later.

>
>> +       pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>> +
>> +       for (i = 0; i<  pix_mp->num_planes; ++i) {
>> +               int bpl = (pix_mp->width * fmt->depth[i])>>  3;
>> +               pix_mp->plane_fmt[i].bytesperline = bpl;
>> +               pix_mp->plane_fmt[i].sizeimage = bpl * pix_mp->height;
>> +
>> +               scaler_dbg(scaler, "[%d]: bpl: %d, sizeimage: %d",
>> +                               i, bpl, pix_mp->plane_fmt[i].sizeimage);
>> +       }
>> +
>> +       return 0;
>> +}
>> +

[...]

>> +static int scaler_runtime_resume(struct device *dev)
>> +{
>> +       struct scaler_dev *scaler = dev_get_drvdata(dev);
>> +       int ret = 0;
>> +       scaler_dbg(scaler, "state: 0x%lx", scaler->state);
>> +
>> +       ret = clk_enable(scaler->clock);
>> +       if (ret<  0)
>> +               return ret;
>> +
>> +       scaler_sw_reset(scaler);
>> +
>> +       return scaler_m2m_resume(scaler);
>
>
> Shouldn't there be clk_disable() when this function fails ?

this funciton scaler_m2m_resume() never fails.


Regards,
Shaik Ameer Basha
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux