| Message ID | 1370870586-24141-4-git-send-email-arun.kk@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Arun, I have read your patches. They seem alright, I back comments made by Hans and Sylwester. I have one question, which follows inline. Best wishes,
Hi Kamil, Thank you for the review. >> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : > 0) >> +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : > 0) > > According to this, MFC v7 is also detected as MFC v6. Was this intended? Yes this was intentional as most of v7 will be reusing the v6 code and only minor changes are there w.r.t firmware interface. > I think that it would be much better to use this in code: > if (IS_MFCV6(dev) || IS_MFCV7(dev)) > And change the define to detect only single MFC revision: > #define IS_MFCV6(dev) (dev->variant->version >= 0x60 && > dev->variant->version < 0x70) > I kept it like that since the macro IS_MFCV6() is used quite frequently in the driver. Also if MFCv8 comes which is again similar to v6 (not sure about this), then it will add another OR condition to this check. > Other possibility I see is to change the name of the check. Although > IS_MFCV6_OR_NEWER(dev) seems too long :) > How about making it IS_MFCV6_PLUS()? Regards Arun -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote: > Hi Kamil, > > Thank you for the review. > > >>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : >> 0) >>> +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : >> 0) >> >> According to this, MFC v7 is also detected as MFC v6. Was this intended? > > Yes this was intentional as most of v7 will be reusing the v6 code and > only minor > changes are there w.r.t firmware interface. > > >> I think that it would be much better to use this in code: >> if (IS_MFCV6(dev) || IS_MFCV7(dev)) >> And change the define to detect only single MFC revision: >> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 && >> dev->variant->version < 0x70) >> > > I kept it like that since the macro IS_MFCV6() is used quite frequently > in the driver. Also if MFCv8 comes which is again similar to v6 (not > sure about this), > then it will add another OR condition to this check. > >> Other possibility I see is to change the name of the check. Although >> IS_MFCV6_OR_NEWER(dev) seems too long :) >> > > How about making it IS_MFCV6_PLUS()? Technically #define IS_MFCV6(dev) (dev->variant->version >= 0x60...) means all lower versions are also higher versions. This may not cause much of a problem (other than the macro being a misnomer) as all current higher versions are supersets of lower versions. But this is not guaranteed(?). Hence changing the definition of the macro to (dev->variant->version >= 0x60 && dev->variant->version < 0x70) as Kamil suggested or renaming it to IS_MFCV6_PLUS() makes sense. OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc? If not we can make it just: #define IS_MFCV6(dev) (dev->variant->version == 0x60 ? 1 : 0)
Hi Sachin, On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote: > On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote: >> Hi Kamil, >> >> Thank you for the review. >> >> >>>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : >>> 0) >>>> +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : >>> 0) >>> >>> According to this, MFC v7 is also detected as MFC v6. Was this intended? >> >> Yes this was intentional as most of v7 will be reusing the v6 code and >> only minor >> changes are there w.r.t firmware interface. >> >> >>> I think that it would be much better to use this in code: >>> if (IS_MFCV6(dev) || IS_MFCV7(dev)) >>> And change the define to detect only single MFC revision: >>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 && >>> dev->variant->version < 0x70) >>> >> >> I kept it like that since the macro IS_MFCV6() is used quite frequently >> in the driver. Also if MFCv8 comes which is again similar to v6 (not >> sure about this), >> then it will add another OR condition to this check. >> >>> Other possibility I see is to change the name of the check. Although >>> IS_MFCV6_OR_NEWER(dev) seems too long :) >>> >> >> How about making it IS_MFCV6_PLUS()? > > Technically > #define IS_MFCV6(dev) (dev->variant->version >= 0x60...) > means all lower versions are also higher versions. > This may not cause much of a problem (other than the macro being a > misnomer) as all current higher versions are supersets of lower > versions. > But this is not guaranteed(?). > Till now we havent encountered otherwise and we can only hope that it remains like this :) > Hence changing the definition of the macro to (dev->variant->version >>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or > renaming it to > IS_MFCV6_PLUS() makes sense. > > OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc? > > If not we can make it just: > #define IS_MFCV6(dev) (dev->variant->version == 0x60 ? 1 : 0) > The v6 version we use is actually v6.5 and v7 is v7.2. In mainline we havent used any FW sub-versions yet. Regards Arun -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arun, On 18 June 2013 11:25, Arun Kumar K <arunkk.samsung@gmail.com> wrote: > Hi Sachin, > > > On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote: >> On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote: >>> Hi Kamil, >>> >>> Thank you for the review. >>> >>> >>>>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : >>>> 0) >>>>> +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : >>>> 0) >>>> >>>> According to this, MFC v7 is also detected as MFC v6. Was this intended? >>> >>> Yes this was intentional as most of v7 will be reusing the v6 code and >>> only minor >>> changes are there w.r.t firmware interface. >>> >>> >>>> I think that it would be much better to use this in code: >>>> if (IS_MFCV6(dev) || IS_MFCV7(dev)) >>>> And change the define to detect only single MFC revision: >>>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 && >>>> dev->variant->version < 0x70) >>>> >>> >>> I kept it like that since the macro IS_MFCV6() is used quite frequently >>> in the driver. Also if MFCv8 comes which is again similar to v6 (not >>> sure about this), >>> then it will add another OR condition to this check. >>> >>>> Other possibility I see is to change the name of the check. Although >>>> IS_MFCV6_OR_NEWER(dev) seems too long :) >>>> >>> >>> How about making it IS_MFCV6_PLUS()? >> >> Technically >> #define IS_MFCV6(dev) (dev->variant->version >= 0x60...) >> means all lower versions are also higher versions. >> This may not cause much of a problem (other than the macro being a >> misnomer) as all current higher versions are supersets of lower >> versions. >> But this is not guaranteed(?). >> > > Till now we havent encountered otherwise and we can only hope that > it remains like this :) > > >> Hence changing the definition of the macro to (dev->variant->version >>>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or >> renaming it to >> IS_MFCV6_PLUS() makes sense. >> >> OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc? >> >> If not we can make it just: >> #define IS_MFCV6(dev) (dev->variant->version == 0x60 ? 1 : 0) >> > > The v6 version we use is actually v6.5 and v7 is v7.2. > In mainline we havent used any FW sub-versions yet. OK. Do they co-exist or is there a possibility for that (to have v6.5 and say v6.7 or v7.2 and v7.4, etc). Just asking.
Hi Sachin, On Tue, Jun 18, 2013 at 11:42 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote: > Hi Arun, > > On 18 June 2013 11:25, Arun Kumar K <arunkk.samsung@gmail.com> wrote: >> Hi Sachin, >> >> >> On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote: >>> On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote: >>>> Hi Kamil, >>>> >>>> Thank you for the review. >>>> >>>> >>>>>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : >>>>> 0) >>>>>> +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : >>>>> 0) >>>>> >>>>> According to this, MFC v7 is also detected as MFC v6. Was this intended? >>>> >>>> Yes this was intentional as most of v7 will be reusing the v6 code and >>>> only minor >>>> changes are there w.r.t firmware interface. >>>> >>>> >>>>> I think that it would be much better to use this in code: >>>>> if (IS_MFCV6(dev) || IS_MFCV7(dev)) >>>>> And change the define to detect only single MFC revision: >>>>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 && >>>>> dev->variant->version < 0x70) >>>>> >>>> >>>> I kept it like that since the macro IS_MFCV6() is used quite frequently >>>> in the driver. Also if MFCv8 comes which is again similar to v6 (not >>>> sure about this), >>>> then it will add another OR condition to this check. >>>> >>>>> Other possibility I see is to change the name of the check. Although >>>>> IS_MFCV6_OR_NEWER(dev) seems too long :) >>>>> >>>> >>>> How about making it IS_MFCV6_PLUS()? >>> >>> Technically >>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60...) >>> means all lower versions are also higher versions. >>> This may not cause much of a problem (other than the macro being a >>> misnomer) as all current higher versions are supersets of lower >>> versions. >>> But this is not guaranteed(?). >>> >> >> Till now we havent encountered otherwise and we can only hope that >> it remains like this :) >> >> >>> Hence changing the definition of the macro to (dev->variant->version >>>>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or >>> renaming it to >>> IS_MFCV6_PLUS() makes sense. >>> >>> OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc? >>> >>> If not we can make it just: >>> #define IS_MFCV6(dev) (dev->variant->version == 0x60 ? 1 : 0) >>> >> >> The v6 version we use is actually v6.5 and v7 is v7.2. >> In mainline we havent used any FW sub-versions yet. > > OK. Do they co-exist or is there a possibility for that (to have v6.5 > and say v6.7 or v7.2 and v7.4, etc). Just asking. > > For these sub-versions, the driver interface remains mostly the same and only internal firmware implementations change (atleast that's what I have seen till date). For mainline purpose, we choose one of the versions and stick to that. Regards Arun -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arun, Sachin, > -----Original Message----- > From: Sachin Kamat [mailto:sachin.kamat@linaro.org] > Sent: Tuesday, June 18, 2013 7:27 AM > To: Arun Kumar K > Cc: Kamil Debski; Arun Kumar K; LMML; jtp.park@samsung.com; Sylwester > Nawrocki; avnd.kiran@samsung.com > Subject: Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7 > > On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote: > > Hi Kamil, > > > > Thank you for the review. > > > > > >>> #define IS_MFCV6(dev) (dev->variant->version >= > 0x60 ? 1 : > >> 0) > >>> +#define IS_MFCV7(dev) (dev->variant->version >= > 0x70 ? 1 : > >> 0) > >> > >> According to this, MFC v7 is also detected as MFC v6. Was this > intended? > > > > Yes this was intentional as most of v7 will be reusing the v6 code > and > > only minor changes are there w.r.t firmware interface. > > > > > >> I think that it would be much better to use this in code: > >> if (IS_MFCV6(dev) || IS_MFCV7(dev)) And change the define to > >> detect only single MFC revision: > >> #define IS_MFCV6(dev) (dev->variant->version >= > 0x60 && > >> dev->variant->version < 0x70) > >> > > > > I kept it like that since the macro IS_MFCV6() is used quite > > frequently in the driver. Also if MFCv8 comes which is again similar > > to v6 (not sure about this), then it will add another OR condition to > > this check. > > > >> Other possibility I see is to change the name of the check. Although > >> IS_MFCV6_OR_NEWER(dev) seems too long :) > >> > > > > How about making it IS_MFCV6_PLUS()? > > Technically > #define IS_MFCV6(dev) (dev->variant->version >= 0x60...) > means all lower versions are also higher versions. > This may not cause much of a problem (other than the macro being a > misnomer) as all current higher versions are supersets of lower > versions. > But this is not guaranteed(?). MFC versions 5+ have very much in common. However there are two previous MFC versions - 4 (s5pc100?) and 1 (s3c6410). These versions are much different if I remember correctly. Drivers for these version are not present in mainline, but I know that there are community members that provide support and keep adding new drivers for older SoCs. Maybe some day they will be added. > > Hence changing the definition of the macro to (dev->variant->version > >= 0x60 && dev->variant->version < 0x70) as Kamil suggested or > renaming it to > IS_MFCV6_PLUS() makes sense. I agree, this name will be easier to understand. > > OTOH, do we really have intermediate version numbers? For e.g. 0x61, > 0x72, etc? > > If not we can make it just: > #define IS_MFCV6(dev) (dev->variant->version == 0x60 ? > 1 : 0) > > Best wishes,
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c index d12faa6..d6be52f 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c @@ -1391,6 +1391,32 @@ static struct s5p_mfc_variant mfc_drvdata_v6 = { .fw_name = "s5p-mfc-v6.fw", }; +struct s5p_mfc_buf_size_v6 mfc_buf_size_v7 = { + .dev_ctx = MFC_CTX_BUF_SIZE_V7, + .h264_dec_ctx = MFC_H264_DEC_CTX_BUF_SIZE_V7, + .other_dec_ctx = MFC_OTHER_DEC_CTX_BUF_SIZE_V7, + .h264_enc_ctx = MFC_H264_ENC_CTX_BUF_SIZE_V7, + .other_enc_ctx = MFC_OTHER_ENC_CTX_BUF_SIZE_V7, +}; + +struct s5p_mfc_buf_size buf_size_v7 = { + .fw = MAX_FW_SIZE_V7, + .cpb = MAX_CPB_SIZE_V7, + .priv = &mfc_buf_size_v7, +}; + +struct s5p_mfc_buf_align mfc_buf_align_v7 = { + .base = 0, +}; + +static struct s5p_mfc_variant mfc_drvdata_v7 = { + .version = MFC_VERSION_V7, + .port_num = MFC_NUM_PORTS_V7, + .buf_size = &buf_size_v7, + .buf_align = &mfc_buf_align_v7, + .fw_name = "s5p-mfc-v7.fw", +}; + static struct platform_device_id mfc_driver_ids[] = { { .name = "s5p-mfc", @@ -1401,6 +1427,9 @@ static struct platform_device_id mfc_driver_ids[] = { }, { .name = "s5p-mfc-v6", .driver_data = (unsigned long)&mfc_drvdata_v6, + }, { + .name = "s5p-mfc-v7", + .driver_data = (unsigned long)&mfc_drvdata_v7, }, {}, }; @@ -1413,6 +1442,9 @@ static const struct of_device_id exynos_mfc_match[] = { }, { .compatible = "samsung,mfc-v6", .data = &mfc_drvdata_v6, + }, { + .compatible = "samsung,mfc-v7", + .data = &mfc_drvdata_v7, }, {}, }; diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h index ef4074c..7281de2 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h @@ -24,6 +24,7 @@ #include <media/videobuf2-core.h> #include "regs-mfc.h" #include "regs-mfc-v6.h" +#include "regs-mfc-v7.h" /* Definitions related to MFC memory */ @@ -684,5 +685,6 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx); (dev->variant->port_num ? 1 : 0) : 0) : 0) #define IS_TWOPORT(dev) (dev->variant->port_num == 2 ? 1 : 0) #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : 0) +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : 0) #endif /* S5P_MFC_COMMON_H_ */
Adds variant data and core support for the MFC v7 firmware Signed-off-by: Arun Kumar K <arun.kk@samsung.com> --- drivers/media/platform/s5p-mfc/s5p_mfc.c | 32 +++++++++++++++++++++++ drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++ 2 files changed, 34 insertions(+)