[PATCH] drm/i915/color: Use correct variable to load lut

[Kandpal, Suraj]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, October 9, 2024 2:21 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>
> Cc: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; Shankar,
> Uma <uma.shankar at intel.com>; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah at intel.com>
> Subject: Re: [PATCH] drm/i915/color: Use correct variable to load lut
> 
> On Wed, Oct 09, 2024 at 08:46:20AM +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Sent: Wednesday, October 9, 2024 12:25 PM
> > > To: Kandpal, Suraj <suraj.kandpal at intel.com>
> > > Cc: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org;
> > > Shankar, Uma <uma.shankar at intel.com>; Borah, Chaitanya Kumar
> > > <chaitanya.kumar.borah at intel.com>
> > > Subject: Re: [PATCH] drm/i915/color: Use correct variable to load
> > > lut
> > >
> > > On Wed, Oct 09, 2024 at 12:07:53PM +0530, Suraj Kandpal wrote:
> > > > Use the blob variable instead of post_csc_lut as it may end up
> > > > being null.
> > >
> > > Not possible. We always have a LUT for each half in split gamma
> > > mode, and if we don't then someone screwed up much earlier before
> we end up here.
> > >
> >
> > If that is the case then the code line const struct drm_property_blob
> > *blob = post_csc_lut ?: pre_csc_lut; wouldn't be needed but it has
> > been added  presumably because there are times
> 
> That is for the non-split gamma cases.
> 
> > that we reach here with post csc as null and in that case we at least
> > make sure we are loading the correct lut hence the fix.
> 
> post_csc_lut is the correct one to load here. Trying to load the pre_csc_lut
> twice is just complete nonsense. And if post_csc_lut could somehow be
> NULL so could pre_csc_lut.
> 

Hi Ville,
do you think it make sense to have a documentation above this code to explain why this made sense
here and not the other places(this being the split gamma case)? to avoid confusion especially from newer
people as myself who aren't as well versed with the history of this function.

Regards,
Suraj Kandpal
> >
> > Regards,
> > Suraj Kandpal
> >
> > > >
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_color.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > > > b/drivers/gpu/drm/i915/display/intel_color.c
> > > > index caf1af039960..22b7090c4f6f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > > @@ -1503,7 +1503,7 @@ static void ivb_load_luts(const struct
> > > intel_crtc_state *crtc_state)
> > > >  		ivb_load_lut_10(crtc_state, pre_csc_lut,
> > > PAL_PREC_SPLIT_MODE |
> > > >  				PAL_PREC_INDEX_VALUE(0));
> > > >  		ivb_load_lut_ext_max(crtc_state);
> > > > -		ivb_load_lut_10(crtc_state, post_csc_lut,
> > > PAL_PREC_SPLIT_MODE |
> > > > +		ivb_load_lut_10(crtc_state, blob, PAL_PREC_SPLIT_MODE |
> > > >  				PAL_PREC_INDEX_VALUE(512));
> > > >  		break;
> > > >  	case GAMMA_MODE_MODE_10BIT:
> > > > @@ -1531,7 +1531,7 @@ static void bdw_load_luts(const struct
> > > intel_crtc_state *crtc_state)
> > > >  		bdw_load_lut_10(crtc_state, pre_csc_lut,
> > > PAL_PREC_SPLIT_MODE |
> > > >  				PAL_PREC_INDEX_VALUE(0));
> > > >  		ivb_load_lut_ext_max(crtc_state);
> > > > -		bdw_load_lut_10(crtc_state, post_csc_lut,
> > > PAL_PREC_SPLIT_MODE |
> > > > +		bdw_load_lut_10(crtc_state, blob, PAL_PREC_SPLIT_MODE |
> > > >  				PAL_PREC_INDEX_VALUE(512));
> > > >  		break;
> > > >  	case GAMMA_MODE_MODE_10BIT:
> > > > --
> > > > 2.43.2
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> 
> --
> Ville Syrjälä
> Intel