| Message ID | 1440425649-9768-8-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2015? 08? 24? 23:14, Tobias Jakobi wrote: > The G2D headers define a number of modes through enums > (like e.g. color, select, repeat, etc.). > > This introduces g2d_validate_select_mode() and > g2d_validate_blending_op() which validate a > select mode or blending operation respectively. > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > --- > exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c > index 2e04f4a..781aff5 100644 > --- a/exynos/exynos_fimg2d.c > +++ b/exynos/exynos_fimg2d.c > @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx, > } > > /* > + * g2d_validate_select_mode - validate select mode. > + * > + * @mode: the mode to validate > + */ > +static unsigned int g2d_validate_select_mode( > + enum e_g2d_select_mode mode) > +{ > + switch (mode) { > + case G2D_SELECT_MODE_NORMAL: > + case G2D_SELECT_MODE_FGCOLOR: > + case G2D_SELECT_MODE_BGCOLOR: > + return 0; > + } It's strange use a bit. Just check the range like below, First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and if (G2D_SELECT_MODE_MAX < mode) { fprintf(strerr, "invalid command(0x%x)\n", mode); return -EINVAL; } And I think it'd be better to change return type of this function to int, > + > + return 1; so return 0 > +} > + > +/* > + * g2d_validate_blending_op - validate blending operation. > + * > + * @operation: the operation to validate > + */ > +static unsigned int g2d_validate_blending_op( > + enum e_g2d_op operation) > +{ > + switch (operation) { > + case G2D_OP_CLEAR: > + case G2D_OP_SRC: > + case G2D_OP_DST: > + case G2D_OP_OVER: > + case G2D_OP_INTERPOLATE: > + case G2D_OP_DISJOINT_CLEAR: > + case G2D_OP_DISJOINT_SRC: > + case G2D_OP_DISJOINT_DST: > + case G2D_OP_CONJOINT_CLEAR: > + case G2D_OP_CONJOINT_SRC: > + case G2D_OP_CONJOINT_DST: > + return 0; Ditto, You could modify it like above. Thanks, Inki Dae > + } > + > + return 1; > +} > + > +/* > * g2d_add_cmd - set given command and value to user side command buffer. > * > * @ctx: a pointer to g2d_context structure. >
On 31 August 2015 at 14:18, Inki Dae <inki.dae@samsung.com> wrote: > On 2015? 08? 24? 23:14, Tobias Jakobi wrote: >> The G2D headers define a number of modes through enums >> (like e.g. color, select, repeat, etc.). >> >> This introduces g2d_validate_select_mode() and >> g2d_validate_blending_op() which validate a >> select mode or blending operation respectively. >> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> --- >> exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c >> index 2e04f4a..781aff5 100644 >> --- a/exynos/exynos_fimg2d.c >> +++ b/exynos/exynos_fimg2d.c >> @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx, >> } >> >> /* >> + * g2d_validate_select_mode - validate select mode. >> + * >> + * @mode: the mode to validate >> + */ >> +static unsigned int g2d_validate_select_mode( >> + enum e_g2d_select_mode mode) >> +{ >> + switch (mode) { >> + case G2D_SELECT_MODE_NORMAL: >> + case G2D_SELECT_MODE_FGCOLOR: >> + case G2D_SELECT_MODE_BGCOLOR: >> + return 0; >> + } > > It's strange use a bit. Just check the range like below, > > First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and > > if (G2D_SELECT_MODE_MAX < mode) { > fprintf(strerr, "invalid command(0x%x)\n", mode); > return -EINVAL; > } > Listing every value might seem like an overkill, indeed. Yet I think it's the more robust approach. By adding _MAX to the API we effectively lock it down. That can be avoided, plus the compiler will warn us when new values are added to the enum. If someone starts getting smart (due to the _MAX) and adds G2D_SELECT_MODE_FOO = -1, the above check will fail :( > And I think it'd be better to change return type of this function to int, > Good idea. Cheers, Emil
Hello, Emil Velikov wrote: > On 31 August 2015 at 14:18, Inki Dae <inki.dae@samsung.com> wrote: >> On 2015? 08? 24? 23:14, Tobias Jakobi wrote: >>> The G2D headers define a number of modes through enums >>> (like e.g. color, select, repeat, etc.). >>> >>> This introduces g2d_validate_select_mode() and >>> g2d_validate_blending_op() which validate a >>> select mode or blending operation respectively. >>> >>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>> --- >>> exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c >>> index 2e04f4a..781aff5 100644 >>> --- a/exynos/exynos_fimg2d.c >>> +++ b/exynos/exynos_fimg2d.c >>> @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx, >>> } >>> >>> /* >>> + * g2d_validate_select_mode - validate select mode. >>> + * >>> + * @mode: the mode to validate >>> + */ >>> +static unsigned int g2d_validate_select_mode( >>> + enum e_g2d_select_mode mode) >>> +{ >>> + switch (mode) { >>> + case G2D_SELECT_MODE_NORMAL: >>> + case G2D_SELECT_MODE_FGCOLOR: >>> + case G2D_SELECT_MODE_BGCOLOR: >>> + return 0; >>> + } >> >> It's strange use a bit. Just check the range like below, >> >> First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and >> >> if (G2D_SELECT_MODE_MAX < mode) { >> fprintf(strerr, "invalid command(0x%x)\n", mode); >> return -EINVAL; >> } >> > Listing every value might seem like an overkill, indeed. Yet I think > it's the more robust approach. that's my thinking as well. The overhead shouldn't be too high and the compiler probably optimizes this anyway. > By adding _MAX to the API we effectively lock it down. That can be > avoided, plus the compiler will warn us when new values are added to > the enum. If someone starts getting smart (due to the _MAX) and adds > G2D_SELECT_MODE_FOO = -1, the above check will fail :( > >> And I think it'd be better to change return type of this function to int, >> > Good idea. What would be the benefit of this? We're just returning only 0 and 1 anyway. My first reaction was to use a bool here. > > Cheers, > Emil > With best wishes, Tobias
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 2e04f4a..781aff5 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx, } /* + * g2d_validate_select_mode - validate select mode. + * + * @mode: the mode to validate + */ +static unsigned int g2d_validate_select_mode( + enum e_g2d_select_mode mode) +{ + switch (mode) { + case G2D_SELECT_MODE_NORMAL: + case G2D_SELECT_MODE_FGCOLOR: + case G2D_SELECT_MODE_BGCOLOR: + return 0; + } + + return 1; +} + +/* + * g2d_validate_blending_op - validate blending operation. + * + * @operation: the operation to validate + */ +static unsigned int g2d_validate_blending_op( + enum e_g2d_op operation) +{ + switch (operation) { + case G2D_OP_CLEAR: + case G2D_OP_SRC: + case G2D_OP_DST: + case G2D_OP_OVER: + case G2D_OP_INTERPOLATE: + case G2D_OP_DISJOINT_CLEAR: + case G2D_OP_DISJOINT_SRC: + case G2D_OP_DISJOINT_DST: + case G2D_OP_CONJOINT_CLEAR: + case G2D_OP_CONJOINT_SRC: + case G2D_OP_CONJOINT_DST: + return 0; + } + + return 1; +} + +/* * g2d_add_cmd - set given command and value to user side command buffer. * * @ctx: a pointer to g2d_context structure.
The G2D headers define a number of modes through enums (like e.g. color, select, repeat, etc.). This introduces g2d_validate_select_mode() and g2d_validate_blending_op() which validate a select mode or blending operation respectively. Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> --- exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)