Re: [PATCH V2 1/3] drm/vkms/crc: Implement verify_crc_source callback

[Haneen Mohammed <hamohammed.sa@gmail.com>]

On Tue, Aug 21, 2018 at 09:53:11AM +0530, Kumar, Mahesh wrote:
> Hi Haneen,
> 
> 
> On 8/21/2018 4:09 AM, Haneen Mohammed wrote:
> > On Tue, Aug 14, 2018 at 08:31:03AM +0530, Mahesh Kumar wrote:
> > > This patch implements "verify_crc_source" callback function for
> > > Virtual KMS drm driver.
> > > 
> > > Changes Since V1:
> > > - update values_cnt in verify_crc_source
> > > 
> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/vkms/vkms_crc.c  | 38 ++++++++++++++++++++++++++++++++++----
> > >   drivers/gpu/drm/vkms/vkms_crtc.c |  1 +
> > >   drivers/gpu/drm/vkms/vkms_drv.h  |  2 ++
> > >   3 files changed, 37 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > index 37d717f38e3c..b2a484b4e2ad 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > @@ -70,6 +70,37 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >   	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> > >   }
> > > +static int vkms_crc_parse_source(const char *src_name, bool *enabled)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!src_name) {
> > > +		*enabled = false;
> > > +	} else if (strcmp(src_name, "auto") == 0) {
> > > +		*enabled = true;
> > > +	} else {
> > > +		*enabled = false;
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
> > > +			   size_t *values_cnt)
> > > +{
> > > +	bool enabled;
> > > +
> > > +	if (vkms_crc_parse_source(src_name, &enabled) < 0) {
> > > +		DRM_DEBUG_DRIVER("unknown source %s\n", src_name);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	*values_cnt = 1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > >   			size_t *values_cnt)
> > >   {
> > > @@ -78,10 +109,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > >   	unsigned long flags;
> > >   	int ret = 0;
> > > -	if (src_name && strcmp(src_name, "auto") == 0)
> > > -		enabled = true;
> > > -	else if (src_name)
> > > -		ret = -EINVAL;
> > > +	ret = vkms_crc_parse_source(src_name, &enabled);
> > > +	if (ret)
> > > +		return ret;
> > I think the return value after vkms_crc_parse_source should be called
> > once instead at the end of vkms_set_crc_source otherwise crc_enabled
> > won't be updated?
> Here I'm assuming if src_name is wrong we don't need to proceed further, let
> the CRC generation to continue whatever it was doing (enabled or disabled).
> But anyway that is changing the original design, will remove above return to
> keep the behavior same.
> 
> -Mahesh

hm I'm not really sure if my approach was the right way, but if it wasn't
then I think maybe this change would be better if it was either in a separate patch or
here with more details

Thank you,
Haneen

> > 
> > - Haneen
> > 
> > >   	*values_cnt = 1;
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > index bfe6e0312cc4..9d0b1a325a78 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > @@ -140,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > >   	.enable_vblank		= vkms_enable_vblank,
> > >   	.disable_vblank		= vkms_disable_vblank,
> > >   	.set_crc_source		= vkms_set_crc_source,
> > > +	.verify_crc_source	= vkms_verify_crc_source,
> > >   };
> > >   static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index f156c930366a..090c5e4f5544 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -125,6 +125,8 @@ void vkms_gem_vunmap(struct drm_gem_object *obj);
> > >   /* CRC Support */
> > >   int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > >   			size_t *values_cnt);
> > > +int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > > +			   size_t *values_cnt);
> > >   void vkms_crc_work_handle(struct work_struct *work);
> > >   #endif /* _VKMS_DRV_H_ */
> > > -- 
> > > 2.16.2
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx