| Message ID | 20180814030103.11215-1-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/3] drm/vkms/crc: Implement verify_crc_source callback | expand |
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? - 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 >
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 > > - 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 >>
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 > > > >
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; *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_ */
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(-)