Re: [Intel-gfx] [PATCH v2 4/9] drm/i915/display: Add new members to configure PCON color conversion

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 13, 2022 at 10:55:24AM +0530, Nautiyal, Ankit K wrote:
> 
> On 9/13/2022 12:25 AM, Ville Syrjälä wrote:
> > On Thu, Sep 01, 2022 at 11:30:56AM +0530, Ankit Nautiyal wrote:
> >> The decision to use DFP output format conversion capabilities should be
> >> during compute_config phase.
> >>
> >> This patch adds new members to crtc_state to help configure the DFP
> >> output related conversions. Also adds a member to intel_dp to store
> >> ycbcr420 pass through capability.
> >>
> >> This will help to store only the format conversion capabilities of the
> >> DP device in intel_dp->dfp, and use crtc_state to compute and store the
> >> configuration for color/format conversion for a given mode.
> >>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_display_types.h | 7 +++++++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> index 0da9b208d56e..065ed19a5dd3 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> @@ -1311,6 +1311,12 @@ struct intel_crtc_state {
> >>   
> >>   	/* for loading single buffered registers during vblank */
> >>   	struct drm_vblank_work vblank_work;
> >> +
> >> +	/* DP DFP color configuration */
> >> +	struct {
> >> +		bool rgb_to_ycbcr;
> >> +		bool ycbcr_444_to_420;
> >> +	} dp_dfp_config;
> > Hmm. I'm thinking just something like
> >   enum intel_output_format sink_format;
> > might be easier to deal with.
> 
> Thanks Ville for looking into this.
> 
> We indeed can compute the final sink_format during compute config (based 
> on pcon color conversion capability + platform capability)
> 
> IIUC, then during config_protocol_converter we need to compute what 
> conversion need to be done based on crtc->output_format and 
> crtc->sink_format
> 
> eg. For sink_format YCBCR420:
> 
> 1. if output_format is RGB -> configure  : rgb_to_ycbcr420 and 
> ycbcr444_to_420
> 
> 2. if output_format is YCBCR444 -> configure  : ycbcr444_to_420
> 
> 3. if output_format is YCBCR420 -> configure : Do nothing YCBCR420 
> passthrough
> 
> So we store the sink_format and configuration is implicit in this case.
> 
> The idea with the dp_dfp_config was to store the conversion config to be 
> done by PCON with given output_format.
> 
> The sink_format in that case is implicit.
> 
> I am open to storing the sink_format too.
> 
> Can this be handy for other encoder as well, or this will be used for DP 
> DFPs only?

If other output types end up getting some kind of converterts
then yeah sink_format would work there as well. I think initally
we should just populate it the same as output_format for all
encoder types, and add it to the crtc state dump. Unfortunately
I don't think we can add it to the state checker on account of
the user being able to yank out the dongle mid-modeset.

> 
> Regards,
> 
> Ankit
> 
> 
> 
> >>   };
> >>   
> >>   enum intel_pipe_crc_source {
> >> @@ -1704,6 +1710,7 @@ struct intel_dp {
> >>   		int pcon_max_frl_bw;
> >>   		u8 max_bpc;
> >>   		bool ycbcr_444_to_420;
> >> +		bool ycbcr420_passthrough;
> >>   		bool rgb_to_ycbcr;
> >>   	} dfp;
> >>   
> >> -- 
> >> 2.25.1

-- 
Ville Syrjälä
Intel