| Message ID | 1444418952-5671-17-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shashank, On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> wrote: > The color correction blob values are loaded during set_property > calls. This patch adds a function to find the blob and apply the > correction values to the display registers, during the atomic > commit call. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> > --- > drivers/gpu/drm/i915/intel_color_manager.c | 44 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_display.c | 2 ++ > drivers/gpu/drm/i915/intel_drv.h | 3 ++ > 3 files changed, 49 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c > index 433e50a..d5315b2 100644 > --- a/drivers/gpu/drm/i915/intel_color_manager.c > +++ b/drivers/gpu/drm/i915/intel_color_manager.c > @@ -307,6 +307,50 @@ static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, > return ret; > } > > +void intel_color_manager_crtc_commit(struct drm_device *dev, > + struct drm_crtc_state *crtc_state) > +{ > + struct drm_property_blob *blob; > + struct drm_crtc *crtc = crtc_state->crtc; > + int ret = -EINVAL; Most places/people advise against pre-emptively initializing ret. > + > + blob = crtc_state->palette_after_ctm_blob; > + if (blob) { > + /* Gamma correction is platform specific */ > + if (IS_CHERRYVIEW(dev)) > + ret = chv_set_gamma(dev, blob, crtc); > + > + if (ret) > + DRM_ERROR("set Gamma correction failed\n"); Do we really want to spam dmesg on for each non Cherryview device ? > + else > + DRM_DEBUG_DRIVER("Gamma correction success\n"); > + } > + > + blob = crtc_state->palette_before_ctm_blob; > + if (blob) { > + /* Degamma correction */ > + if (IS_CHERRYVIEW(dev)) > + ret = chv_set_degamma(dev, blob, crtc); > + > + if (ret) > + DRM_ERROR("set degamma correction failed\n"); > + else > + DRM_DEBUG_DRIVER("degamma correction success\n"); > + } > + > + blob = crtc_state->ctm_blob; > + if (blob) { > + /* CSC correction */ > + if (IS_CHERRYVIEW(dev)) > + ret = chv_set_csc(dev, blob, crtc); > + > + if (ret) > + DRM_ERROR("set CSC correction failed\n"); > + else > + DRM_DEBUG_DRIVER("CSC correction success\n"); > + } > +} > + > void intel_attach_color_properties_to_crtc(struct drm_device *dev, > struct drm_crtc *crtc) > { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 98cc97a..8235341 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13528,6 +13528,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > intel_update_pipe_config(intel_crtc, old_intel_state); > else if (INTEL_INFO(dev)->gen >= 9) > skl_detach_scalers(intel_crtc); > + > + intel_color_manager_crtc_commit(dev, crtc->state); > } > > static void intel_finish_crtc_commit(struct drm_crtc *crtc, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ed66a4f..d100e81 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1469,4 +1469,7 @@ void intel_plane_destroy_state(struct drm_plane *plane, > struct drm_plane_state *state); > extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; > > +/* intel_color_mnager.c */ Typo -> manager. Regards, Emil
Regards Shashank On 10/10/2015 4:54 AM, Emil Velikov wrote: > Hi Shashank, > > On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> wrote: >> The color correction blob values are loaded during set_property >> calls. This patch adds a function to find the blob and apply the >> correction values to the display registers, during the atomic >> commit call. >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_color_manager.c | 44 ++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_display.c | 2 ++ >> drivers/gpu/drm/i915/intel_drv.h | 3 ++ >> 3 files changed, 49 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c >> index 433e50a..d5315b2 100644 >> --- a/drivers/gpu/drm/i915/intel_color_manager.c >> +++ b/drivers/gpu/drm/i915/intel_color_manager.c >> @@ -307,6 +307,50 @@ static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, >> return ret; >> } >> >> +void intel_color_manager_crtc_commit(struct drm_device *dev, >> + struct drm_crtc_state *crtc_state) >> +{ >> + struct drm_property_blob *blob; >> + struct drm_crtc *crtc = crtc_state->crtc; >> + int ret = -EINVAL; > Most places/people advise against pre-emptively initializing ret. > Just in this case, if makes sense to keep one, coz there might be multiple blobs which we are applying in the commit action, and every blob can return error, from the core function. Assume that there is a situation where both palette_after_ctm(gamma) and CTM is in place, then we will be going through multiple if() cases and have to check the return values. >> + >> + blob = crtc_state->palette_after_ctm_blob; >> + if (blob) { >> + /* Gamma correction is platform specific */ >> + if (IS_CHERRYVIEW(dev)) >> + ret = chv_set_gamma(dev, blob, crtc); >> + >> + if (ret) >> + DRM_ERROR("set Gamma correction failed\n"); > Do we really want to spam dmesg on for each non Cherryview device ? If you see the complete implementation, as IS_GEN8()/IS_SKL is coming up here after IS_CHV(patch 19,20,21) which will cover BDW/SKL/BXT. Anything else which comes here, deserves a DRM_ERROR() as this platform will not be supported. > >> + else >> + DRM_DEBUG_DRIVER("Gamma correction success\n"); >> + } >> + >> + blob = crtc_state->palette_before_ctm_blob; >> + if (blob) { >> + /* Degamma correction */ >> + if (IS_CHERRYVIEW(dev)) >> + ret = chv_set_degamma(dev, blob, crtc); >> + >> + if (ret) >> + DRM_ERROR("set degamma correction failed\n"); >> + else >> + DRM_DEBUG_DRIVER("degamma correction success\n"); >> + } >> + >> + blob = crtc_state->ctm_blob; >> + if (blob) { >> + /* CSC correction */ >> + if (IS_CHERRYVIEW(dev)) >> + ret = chv_set_csc(dev, blob, crtc); >> + >> + if (ret) >> + DRM_ERROR("set CSC correction failed\n"); >> + else >> + DRM_DEBUG_DRIVER("CSC correction success\n"); >> + } >> +} >> + >> void intel_attach_color_properties_to_crtc(struct drm_device *dev, >> struct drm_crtc *crtc) >> { >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 98cc97a..8235341 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13528,6 +13528,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, >> intel_update_pipe_config(intel_crtc, old_intel_state); >> else if (INTEL_INFO(dev)->gen >= 9) >> skl_detach_scalers(intel_crtc); >> + >> + intel_color_manager_crtc_commit(dev, crtc->state); >> } >> >> static void intel_finish_crtc_commit(struct drm_crtc *crtc, >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index ed66a4f..d100e81 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1469,4 +1469,7 @@ void intel_plane_destroy_state(struct drm_plane *plane, >> struct drm_plane_state *state); >> extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; >> >> +/* intel_color_mnager.c */ > Typo -> manager. > Oops, thanks. > Regards, > Emil >
On 10 October 2015 at 06:20, Sharma, Shashank <shashank.sharma@intel.com> wrote: > On 10/10/2015 4:54 AM, Emil Velikov wrote: >> >> Hi Shashank, >> >> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> >> wrote: >>> >>> The color correction blob values are loaded during set_property >>> calls. This patch adds a function to find the blob and apply the >>> correction values to the display registers, during the atomic >>> commit call. >>> >>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> >>> --- >>> drivers/gpu/drm/i915/intel_color_manager.c | 44 >>> ++++++++++++++++++++++++++++++ >>> drivers/gpu/drm/i915/intel_display.c | 2 ++ >>> drivers/gpu/drm/i915/intel_drv.h | 3 ++ >>> 3 files changed, 49 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c >>> b/drivers/gpu/drm/i915/intel_color_manager.c >>> index 433e50a..d5315b2 100644 >>> --- a/drivers/gpu/drm/i915/intel_color_manager.c >>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c >>> @@ -307,6 +307,50 @@ static int chv_set_gamma(struct drm_device *dev, >>> struct drm_property_blob *blob, >>> return ret; >>> } >>> >>> +void intel_color_manager_crtc_commit(struct drm_device *dev, >>> + struct drm_crtc_state *crtc_state) >>> +{ >>> + struct drm_property_blob *blob; >>> + struct drm_crtc *crtc = crtc_state->crtc; >>> + int ret = -EINVAL; >> >> Most places/people advise against pre-emptively initializing ret. >> > Just in this case, if makes sense to keep one, coz there might be multiple > blobs which we are applying in the commit action, and every blob can return > error, from the core function. > Assume that there is a situation where both palette_after_ctm(gamma) and CTM > is in place, then we will be going through multiple if() cases and have to > check the return values. When you have an exception of any rule, this implies that you are using a suboptimal way of doing things. >>> >>> + >>> + blob = crtc_state->palette_after_ctm_blob; >>> + if (blob) { >>> + /* Gamma correction is platform specific */ >>> + if (IS_CHERRYVIEW(dev)) >>> + ret = chv_set_gamma(dev, blob, crtc); >>> + >>> + if (ret) >>> + DRM_ERROR("set Gamma correction failed\n"); >> >> Do we really want to spam dmesg on for each non Cherryview device ? > > If you see the complete implementation, as IS_GEN8()/IS_SKL is coming up > here after IS_CHV(patch 19,20,21) which will cover BDW/SKL/BXT. Anything > else which comes here, deserves a DRM_ERROR() as this platform will not be > supported. > Your patches should be independent changes. You cannot say "I'm adding something iffy here, but it will be fixed with another patch". Otherwise you'll get developer/user X bisecting the kernel, which will end up with broken system (flooded dmesg in this case) half way through the bisect. Regards, Emil
Regards Shashank On 10/13/2015 6:47 PM, Emil Velikov wrote: > On 10 October 2015 at 06:20, Sharma, Shashank <shashank.sharma@intel.com> wrote: >> On 10/10/2015 4:54 AM, Emil Velikov wrote: >>> >>> Hi Shashank, >>> >>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> >>> wrote: >>>> >>>> The color correction blob values are loaded during set_property >>>> calls. This patch adds a function to find the blob and apply the >>>> correction values to the display registers, during the atomic >>>> commit call. >>>> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_color_manager.c | 44 >>>> ++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_display.c | 2 ++ >>>> drivers/gpu/drm/i915/intel_drv.h | 3 ++ >>>> 3 files changed, 49 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c >>>> b/drivers/gpu/drm/i915/intel_color_manager.c >>>> index 433e50a..d5315b2 100644 >>>> --- a/drivers/gpu/drm/i915/intel_color_manager.c >>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c >>>> @@ -307,6 +307,50 @@ static int chv_set_gamma(struct drm_device *dev, >>>> struct drm_property_blob *blob, >>>> return ret; >>>> } >>>> >>>> +void intel_color_manager_crtc_commit(struct drm_device *dev, >>>> + struct drm_crtc_state *crtc_state) >>>> +{ >>>> + struct drm_property_blob *blob; >>>> + struct drm_crtc *crtc = crtc_state->crtc; >>>> + int ret = -EINVAL; >>> >>> Most places/people advise against pre-emptively initializing ret. >>> >> Just in this case, if makes sense to keep one, coz there might be multiple >> blobs which we are applying in the commit action, and every blob can return >> error, from the core function. >> Assume that there is a situation where both palette_after_ctm(gamma) and CTM >> is in place, then we will be going through multiple if() cases and have to >> check the return values. > When you have an exception of any rule, this implies that you are > using a suboptimal way of doing things. > Not sure, but if you think its that serious, I will gladly change it to as you suggested :) >>>> >>>> + >>>> + blob = crtc_state->palette_after_ctm_blob; >>>> + if (blob) { >>>> + /* Gamma correction is platform specific */ >>>> + if (IS_CHERRYVIEW(dev)) >>>> + ret = chv_set_gamma(dev, blob, crtc); >>>> + >>>> + if (ret) >>>> + DRM_ERROR("set Gamma correction failed\n"); >>> >>> Do we really want to spam dmesg on for each non Cherryview device ? >> >> If you see the complete implementation, as IS_GEN8()/IS_SKL is coming up >> here after IS_CHV(patch 19,20,21) which will cover BDW/SKL/BXT. Anything >> else which comes here, deserves a DRM_ERROR() as this platform will not be >> supported. >> > Your patches should be independent changes. You cannot say "I'm adding > something iffy here, but it will be fixed with another patch". > Otherwise you'll get developer/user X bisecting the kernel, which will > end up with broken system (flooded dmesg in this case) half way > through the bisect. > There is no confusion here, and its an independent change. Till this patch, we have color management implemented for CHV only and any other platforms, should not even register this property, so naturally it must not hit this part of code. If its hitting, yes I would like to show this DRM_ERROR. So there is nothing wrong even if we bisect. > Regards, > Emil >
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 433e50a..d5315b2 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -307,6 +307,50 @@ static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, return ret; } +void intel_color_manager_crtc_commit(struct drm_device *dev, + struct drm_crtc_state *crtc_state) +{ + struct drm_property_blob *blob; + struct drm_crtc *crtc = crtc_state->crtc; + int ret = -EINVAL; + + blob = crtc_state->palette_after_ctm_blob; + if (blob) { + /* Gamma correction is platform specific */ + if (IS_CHERRYVIEW(dev)) + ret = chv_set_gamma(dev, blob, crtc); + + if (ret) + DRM_ERROR("set Gamma correction failed\n"); + else + DRM_DEBUG_DRIVER("Gamma correction success\n"); + } + + blob = crtc_state->palette_before_ctm_blob; + if (blob) { + /* Degamma correction */ + if (IS_CHERRYVIEW(dev)) + ret = chv_set_degamma(dev, blob, crtc); + + if (ret) + DRM_ERROR("set degamma correction failed\n"); + else + DRM_DEBUG_DRIVER("degamma correction success\n"); + } + + blob = crtc_state->ctm_blob; + if (blob) { + /* CSC correction */ + if (IS_CHERRYVIEW(dev)) + ret = chv_set_csc(dev, blob, crtc); + + if (ret) + DRM_ERROR("set CSC correction failed\n"); + else + DRM_DEBUG_DRIVER("CSC correction success\n"); + } +} + void intel_attach_color_properties_to_crtc(struct drm_device *dev, struct drm_crtc *crtc) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 98cc97a..8235341 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13528,6 +13528,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, intel_update_pipe_config(intel_crtc, old_intel_state); else if (INTEL_INFO(dev)->gen >= 9) skl_detach_scalers(intel_crtc); + + intel_color_manager_crtc_commit(dev, crtc->state); } static void intel_finish_crtc_commit(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ed66a4f..d100e81 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1469,4 +1469,7 @@ void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; +/* intel_color_mnager.c */ +void intel_color_manager_crtc_commit(struct drm_device *dev, + struct drm_crtc_state *crtc_state); #endif /* __INTEL_DRV_H__ */