| Message ID | 20200625163525.5119-4-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hantro low-hanging cleanups | expand |
Hi Ezequiel, On Thu, 2020-06-25 at 13:35 -0300, Ezequiel Garcia wrote: > So far we've been using the .buf_finish hook to distinguish > decoder from encoder. This is unnecessarily obfuscated. > > Moreover, we want to move the buf_finish, so use a cleaner > scheme to distinguish the driver decoder/encoder type. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/hantro/hantro.h | 2 ++ > drivers/staging/media/hantro/hantro_drv.c | 4 +++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > index 3005207fc6fb..028b788ad50f 100644 > --- a/drivers/staging/media/hantro/hantro.h > +++ b/drivers/staging/media/hantro/hantro.h > @@ -199,6 +199,7 @@ struct hantro_dev { > * > * @dev: VPU driver data to which the context belongs. > * @fh: V4L2 file handler. > + * @is_encoder: Decoder or encoder context? > * > * @sequence_cap: Sequence counter for capture queue > * @sequence_out: Sequence counter for output queue > @@ -223,6 +224,7 @@ struct hantro_dev { > struct hantro_ctx { > struct hantro_dev *dev; > struct v4l2_fh fh; > + bool is_encoder; > > u32 sequence_cap; > u32 sequence_out; > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index 0db8ad455160..112ed556eb90 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -197,7 +197,7 @@ static void device_run(void *priv) > > bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx) > { > - return ctx->buf_finish == hantro_enc_buf_finish; > + return ctx->is_encoder; > } Now this function feels a little superfluous. Why not replace hantro_is_encoder_ctx(ctx) with ctx->is_encoder everywhere? regards Philipp
Hi Ezequiel, On 2020-06-25 17:35, Ezequiel Garcia wrote: > So far we've been using the .buf_finish hook to distinguish > decoder from encoder. This is unnecessarily obfuscated. > > Moreover, we want to move the buf_finish, so use a cleaner > scheme to distinguish the driver decoder/encoder type. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/hantro/hantro.h | 2 ++ > drivers/staging/media/hantro/hantro_drv.c | 4 +++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > index 3005207fc6fb..028b788ad50f 100644 > --- a/drivers/staging/media/hantro/hantro.h > +++ b/drivers/staging/media/hantro/hantro.h > @@ -199,6 +199,7 @@ struct hantro_dev { > * > * @dev: VPU driver data to which the context belongs. > * @fh: V4L2 file handler. > + * @is_encoder: Decoder or encoder context? > * > * @sequence_cap: Sequence counter for capture queue > * @sequence_out: Sequence counter for output queue > @@ -223,6 +224,7 @@ struct hantro_dev { > struct hantro_ctx { > struct hantro_dev *dev; > struct v4l2_fh fh; > + bool is_encoder; > > u32 sequence_cap; > u32 sequence_out; > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index 0db8ad455160..112ed556eb90 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -197,7 +197,7 @@ static void device_run(void *priv) > > bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx) > { > - return ctx->buf_finish == hantro_enc_buf_finish; > + return ctx->is_encoder; FWIW I'd suggest removing the wrapper function entirely now - it makes sense when the check itself is implemented in a weird and non-obvious way, but a simple boolean flag named exactly what it means is already about as clear as it can get. Robin. > } > > static struct v4l2_m2m_ops vpu_m2m_ops = { > @@ -420,8 +420,10 @@ static int hantro_open(struct file *filp) > if (func->id == MEDIA_ENT_F_PROC_VIDEO_ENCODER) { > allowed_codecs = vpu->variant->codec & HANTRO_ENCODERS; > ctx->buf_finish = hantro_enc_buf_finish; > + ctx->is_encoder = true; > } else if (func->id == MEDIA_ENT_F_PROC_VIDEO_DECODER) { > allowed_codecs = vpu->variant->codec & HANTRO_DECODERS; > + ctx->is_encoder = false; > } else { > ret = -ENODEV; > goto err_ctx_free; >
On Fri, 2020-06-26 at 09:58 +0200, Philipp Zabel wrote: > Hi Ezequiel, > > On Thu, 2020-06-25 at 13:35 -0300, Ezequiel Garcia wrote: > > So far we've been using the .buf_finish hook to distinguish > > decoder from encoder. This is unnecessarily obfuscated. > > > > Moreover, we want to move the buf_finish, so use a cleaner > > scheme to distinguish the driver decoder/encoder type. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/staging/media/hantro/hantro.h | 2 ++ > > drivers/staging/media/hantro/hantro_drv.c | 4 +++- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > > index 3005207fc6fb..028b788ad50f 100644 > > --- a/drivers/staging/media/hantro/hantro.h > > +++ b/drivers/staging/media/hantro/hantro.h > > @@ -199,6 +199,7 @@ struct hantro_dev { > > * > > * @dev: VPU driver data to which the context belongs. > > * @fh: V4L2 file handler. > > + * @is_encoder: Decoder or encoder context? > > * > > * @sequence_cap: Sequence counter for capture queue > > * @sequence_out: Sequence counter for output queue > > @@ -223,6 +224,7 @@ struct hantro_dev { > > struct hantro_ctx { > > struct hantro_dev *dev; > > struct v4l2_fh fh; > > + bool is_encoder; > > > > u32 sequence_cap; > > u32 sequence_out; > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > index 0db8ad455160..112ed556eb90 100644 > > --- a/drivers/staging/media/hantro/hantro_drv.c > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > @@ -197,7 +197,7 @@ static void device_run(void *priv) > > > > bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx) > > { > > - return ctx->buf_finish == hantro_enc_buf_finish; > > + return ctx->is_encoder; > > } > > Now this function feels a little superfluous. Why not replace > hantro_is_encoder_ctx(ctx) > with > ctx->is_encoder > everywhere? > Absolutely. Thanks for reviewing, Ezequiel
On Fri, 2020-06-26 at 10:52 +0100, Robin Murphy wrote: > Hi Ezequiel, > > On 2020-06-25 17:35, Ezequiel Garcia wrote: > > So far we've been using the .buf_finish hook to distinguish > > decoder from encoder. This is unnecessarily obfuscated. > > > > Moreover, we want to move the buf_finish, so use a cleaner > > scheme to distinguish the driver decoder/encoder type. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/staging/media/hantro/hantro.h | 2 ++ > > drivers/staging/media/hantro/hantro_drv.c | 4 +++- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > > index 3005207fc6fb..028b788ad50f 100644 > > --- a/drivers/staging/media/hantro/hantro.h > > +++ b/drivers/staging/media/hantro/hantro.h > > @@ -199,6 +199,7 @@ struct hantro_dev { > > * > > * @dev: VPU driver data to which the context belongs. > > * @fh: V4L2 file handler. > > + * @is_encoder: Decoder or encoder context? > > * > > * @sequence_cap: Sequence counter for capture queue > > * @sequence_out: Sequence counter for output queue > > @@ -223,6 +224,7 @@ struct hantro_dev { > > struct hantro_ctx { > > struct hantro_dev *dev; > > struct v4l2_fh fh; > > + bool is_encoder; > > > > u32 sequence_cap; > > u32 sequence_out; > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > index 0db8ad455160..112ed556eb90 100644 > > --- a/drivers/staging/media/hantro/hantro_drv.c > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > @@ -197,7 +197,7 @@ static void device_run(void *priv) > > > > bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx) > > { > > - return ctx->buf_finish == hantro_enc_buf_finish; > > + return ctx->is_encoder; > > FWIW I'd suggest removing the wrapper function entirely now - it makes > sense when the check itself is implemented in a weird and non-obvious > way, but a simple boolean flag named exactly what it means is already > about as clear as it can get. > Indeed, Philipp pointed out the same thing. Makes perfect sense, and thanks a lot for reviewing. Ezequiel
diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index 3005207fc6fb..028b788ad50f 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -199,6 +199,7 @@ struct hantro_dev { * * @dev: VPU driver data to which the context belongs. * @fh: V4L2 file handler. + * @is_encoder: Decoder or encoder context? * * @sequence_cap: Sequence counter for capture queue * @sequence_out: Sequence counter for output queue @@ -223,6 +224,7 @@ struct hantro_dev { struct hantro_ctx { struct hantro_dev *dev; struct v4l2_fh fh; + bool is_encoder; u32 sequence_cap; u32 sequence_out; diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index 0db8ad455160..112ed556eb90 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -197,7 +197,7 @@ static void device_run(void *priv) bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx) { - return ctx->buf_finish == hantro_enc_buf_finish; + return ctx->is_encoder; } static struct v4l2_m2m_ops vpu_m2m_ops = { @@ -420,8 +420,10 @@ static int hantro_open(struct file *filp) if (func->id == MEDIA_ENT_F_PROC_VIDEO_ENCODER) { allowed_codecs = vpu->variant->codec & HANTRO_ENCODERS; ctx->buf_finish = hantro_enc_buf_finish; + ctx->is_encoder = true; } else if (func->id == MEDIA_ENT_F_PROC_VIDEO_DECODER) { allowed_codecs = vpu->variant->codec & HANTRO_DECODERS; + ctx->is_encoder = false; } else { ret = -ENODEV; goto err_ctx_free;
So far we've been using the .buf_finish hook to distinguish decoder from encoder. This is unnecessarily obfuscated. Moreover, we want to move the buf_finish, so use a cleaner scheme to distinguish the driver decoder/encoder type. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/hantro/hantro.h | 2 ++ drivers/staging/media/hantro/hantro_drv.c | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-)