On Thu, Mar 09, 2023 at 02:01:06PM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
>
> Thanks for the comments and suggestions. Please find my response inline:
>
> On 3/8/2023 8:56 PM, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2023 at 05:10:57PM +0200, Ville Syrjälä wrote:
> >> On Mon, Feb 27, 2023 at 09:33:14AM +0530, Ankit Nautiyal wrote:
> >>> For YCbCr420 output, scaler is required for downsampling.
> >>> Scaler can be used only when source size smaller than max_src_w and
> >>> max_src_h as defined by for the platform.
> >>> So go for native YCbCr420 only if there are no scaler constraints.
> >>>
> >>> v2: Corrected max-width based on Display Version.
> >>>
> >>> v3: Updated max-width as per latest Bspec change.
> >>>
> >>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/i915/display/intel_dp.c | 41 ++++++++++++++++++++++---
> >>> 1 file changed, 37 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >>> index 1a30cc021b25..e95fc0f0d13a 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >>> @@ -804,11 +804,36 @@ u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
> >>> return 0;
> >>> }
> >>>
> >>> +static bool
> >>> +ycbcr420_scaler_constraints(struct drm_i915_private *i915,
> >>> + const struct drm_display_mode *mode)
> >>> +{
> >>> + int max_src_w, max_src_h;
> >>> +
> >>> + if (DISPLAY_VER(i915) < 11) {
> >>> + max_src_w = 4096;
> >>> + max_src_h = 4096;
> >>> + } else if (DISPLAY_VER(i915) < 12) {
> >>> + max_src_w = 5120;
> >>> + max_src_h = 4096;
> >>> + } else if (DISPLAY_VER(i915) < 14) {
> >>> + max_src_w = 5120;
> >>> + max_src_h = 8192;
> >>> + } else {
> >>> + max_src_w = 4096;
> >>> + max_src_h = 8192;
> >>> + }
> >>> +
> >>> + return mode->hdisplay > max_src_w || mode->vdisplay > max_src_h;
> >>> +}
> >>> +
> >> I don't really like this. If we do something like this
> >> then it should be the scaler code that checks this stuff.
>
> Makes sense, this does belong to the scaler file and scaler checks.
>
>
> >>
> >> However, after pondering about this more I'm actually
> >> leaning towards using 4:4:4 output whenever possible,
> >> only going for 4:2:0 if absolutely necessary. That
> >> avoids having to deal with all the annoying scaler/etc
> >> limitations.
> > In fact perhaps best to try RGB first (also avoids the whole
> > pipe CSC mess on glk), then YCbCr 4:4:4 (still avoids the
> > scaler mess), and finally accepting that we need to do
> > native YCbCr 4:2:0 output.
>
> Ok so if I understand correctly, in intel_dp_output_format()
>
> If sink_format is YCBCR420:
>
> -first try with output_format as RGB and RGB->YCBCR420 conv via DFP (if
> conv supported)
>
> -Or else try with output_format as YCBCR444 and use YCBCR444->YCBCR420
> conv via DFP (if conv supported)
>
> -else try with output_format YCBCR420.
Yeah something along those lines. Maybe it can be expressed
in a pretty generic way even:
if (sink_format=RGB||dfp_can_convert_from_rgb(sink_format))
return RGB;
if (sink_format=YCbCr444||dfp_can_convert_from_ycbcr444(sink_format))
return YCbCr444;
return sink_format;
>
> If there are indeed scaler constraints, those are to be taken care in
> scaler check code.
>
> Shall I drop the scaler constraint for now and have that as a separate
> series?
Don't we already have sufficient checks in the scaler code?
Well, if not a separate series for that seems better.
--
Ville Syrjälä
Intel
