linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
To: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v3 2/4] rcar-vin: Add control for alpha component
Date: Thu, 4 Jul 2019 17:35:05 +0200	[thread overview]
Message-ID: <20190704153505.GE17685@bigcity.dyn.berto.se> (raw)
In-Reply-To: <c1c5dded-8a39-131e-4bef-ed13dc2fba47@ideasonboard.com>

Hi Kieran,

Thanks for your feedback.

On 2019-07-04 16:15:54 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 04/07/2019 02:58, Niklas Söderlund wrote:
> > In preparation to adding support for RGB pixel formats with an alpha
> > component add a control to allow the user to control which alpha value
> > should be used.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Comment/Question below, but:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 
> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++-
> >  drivers/media/platform/rcar-vin/rcar-dma.c  |  5 ++
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  5 ++
> >  3 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 64f9cf790445d14e..ee6e6cb39c749675 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin)
> >  	kref_put(&group->refcount, rvin_group_release);
> >  }
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Controls
> > + */
> > +
> > +static int rvin_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct rvin_dev *vin =
> > +		container_of(ctrl->handler, struct rvin_dev, ctrl_handler);
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_ALPHA_COMPONENT:
> > +		rvin_set_alpha(vin, ctrl->val);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
> > +	.s_ctrl = rvin_s_ctrl,
> > +};
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Async notifier
> >   */
> > @@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > +
> > +	if (vin->ctrl_handler.error) {
> > +		ret = vin->ctrl_handler.error;
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		return ret;
> > +	}
> > +
> >  	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> >  				    NULL, true);
> >  	if (ret < 0) {
> > @@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		rvin_group_put(vin);
> >  
> > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > +
> > +	if (vin->ctrl_handler.error) {
> > +		ret = vin->ctrl_handler.error;
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		return ret;
> > +	}
> > +
> > +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> 
> There's quite a few lines duplicated between rvin_mc_init() and
> rvin_parallel_subdevice_attach() to instantiate the controls. Could that
> code be shared in a single function, which would make it easier to
> extend with new controls?

Yes there is, and I have deliberately keep them so. Reason for this is 
that I'm trying to work up the courage to split this driver in two, one 
which is pure video device centric (Gen2 only) and one that is media 
device centric (Gen2 and Gen3). Then marking the Gen2 only driver as 
deprecated and in time delete it completely making rcar-vin media device 
centric only for both Gen2 and Gen3. Another option is of course to skip 
the splitting step and sometime in the future delete the video device 
centric mode directly.

In both cases it will be easier to do if the two modes init code are 
split to make it easier to move or delete. So I would prefer to keep the 
code as it is for now.

> 
> Perhaps no other controls are expected though.

No other controls are expected for Gen3.

> 
> Anyway, that's not so crucial - so RB tag added above.

Thanks.

> 
> 
> 
> > +
> >  	return ret;
> >  }
> >  
> > @@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  
> >  	vin->dev = &pdev->dev;
> >  	vin->info = of_device_get_match_data(&pdev->dev);
> > +	vin->alpha = 0xff;
> >  
> >  	/*
> >  	 * Special care is needed on r8a7795 ES1.x since it
> > @@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  	return 0;
> >  
> >  error_group_unregister:
> > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +
> >  	if (vin->info->use_mc) {
> >  		mutex_lock(&vin->group->lock);
> >  		if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> > @@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
> >  		}
> >  		mutex_unlock(&vin->group->lock);
> >  		rvin_group_put(vin);
> > -	} else {
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> >  	}
> >  
> > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +
> >  	rvin_dma_unregister(vin);
> >  
> >  	return 0;
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 2d146ecf93d66ad5..4e991cce5fb56a90 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >  
> >  	return 0;
> >  }
> > +
> > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
> > +{
> > +	vin->alpha = alpha;
> > +}
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 0b13b34d03e3dce4..365dfde06ec25add 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -178,6 +178,8 @@ struct rvin_info {
> >   * @compose:		active composing
> >   * @source:		active size of the video source
> >   * @std:		active video standard of the video source
> > + *
> > + * @alpha:		Alpha component to fill in for supported pixel formats
> >   */
> >  struct rvin_dev {
> >  	struct device *dev;
> > @@ -215,6 +217,8 @@ struct rvin_dev {
> >  	struct v4l2_rect compose;
> >  	struct v4l2_rect source;
> >  	v4l2_std_id std;
> > +
> > +	unsigned int alpha;
> >  };
> >  
> >  #define vin_to_source(vin)		((vin)->parallel->subdev)
> > @@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
> >  void rvin_crop_scale_comp(struct rvin_dev *vin);
> >  
> >  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
> >  
> >  #endif
> > 
> 

-- 
Regards,
Niklas Söderlund

  reply	other threads:[~2019-07-04 15:35 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-04  1:58 [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha Niklas Söderlund
2019-07-04  1:58 ` [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB Niklas Söderlund
2019-07-04 14:13   ` Kieran Bingham
2019-07-04  1:58 ` [PATCH v3 2/4] rcar-vin: Add control for alpha component Niklas Söderlund
2019-07-04 15:15   ` Kieran Bingham
2019-07-04 15:35     ` Niklas Söderlund [this message]
2019-07-04  1:58 ` [PATCH v3 3/4] rcar-vin: Add support for RGB formats with " Niklas Söderlund
2019-07-04 12:14   ` Laurent Pinchart
2019-07-04 15:50   ` Kieran Bingham
2019-07-04  1:58 ` [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device Niklas Söderlund
2019-07-04 12:16   ` Laurent Pinchart
2019-07-04 15:55   ` Kieran Bingham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190704153505.GE17685@bigcity.dyn.berto.se \
    --to=niklas.soderlund+renesas@ragnatech.se \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --subject='Re: [PATCH v3 2/4] rcar-vin: Add control for alpha component' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).