| Message ID | f00e9d55ce20b256177222588780c660aa587cc3.1576081155.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsc: fixes for ICL DSI DSC | expand |
On Wed, Dec 11, 2019 at 06:23:47PM +0200, Jani Nikula wrote: > The check for cpu_transcoder != TRANSCODER_A is more magic than > necessary, and potentially misleading. Before TGL, DSC is supported on > pipe A if, and only if, it's used with eDP or DSI transcoders. No > functional changes. > Hmm, so we could still use PIPE_A but if its eDP or DSI it would use TRANSCODER_EDP or TRANSCODER_DSI and that should still work? So its simpler to say that if it is PIPE_A && transcoder_A then it doesnt support DSC? Wouldnt it be simpler to change the condition to : if (INTEL_GEN(i915) >= 10 && !(pipe_A && transcode_A) return true; Regards Manasi > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Vandita Kulkarni <vandita.kulkarni@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_vdsc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c > index e6f60be9ee84..41718f721484 100644 > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > @@ -337,7 +337,10 @@ static const struct rc_parameters *get_rc_params(u16 compressed_bpp, > bool intel_dsc_source_support(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state) > { > + const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > + enum pipe pipe = crtc->pipe; > > if (!INTEL_INFO(i915)->display.has_dsc) > return false; > @@ -347,7 +350,10 @@ bool intel_dsc_source_support(struct intel_encoder *encoder, > return true; > > if (INTEL_GEN(i915) >= 10 && > - crtc_state->cpu_transcoder != TRANSCODER_A) > + (pipe != PIPE_A || > + (cpu_transcoder == TRANSCODER_EDP || > + cpu_transcoder == TRANSCODER_DSI_0 || > + cpu_transcoder == TRANSCODER_DSI_1))) > return true; > > return false; > -- > 2.20.1 >
On Wed, 11 Dec 2019, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Wed, Dec 11, 2019 at 06:23:47PM +0200, Jani Nikula wrote: >> The check for cpu_transcoder != TRANSCODER_A is more magic than >> necessary, and potentially misleading. Before TGL, DSC is supported on >> pipe A if, and only if, it's used with eDP or DSI transcoders. No >> functional changes. >> > > Hmm, so we could still use PIPE_A but if its eDP or DSI it would use > TRANSCODER_EDP or TRANSCODER_DSI and that should still work? Correct, because on gen 11 eDP/DSI have a DSC engine in front of the transcoder. > So its simpler to say that if it is PIPE_A && transcoder_A then it doesnt > support DSC? > Wouldnt it be simpler to change the condition to : > if (INTEL_GEN(i915) >= 10 && !(pipe_A && transcode_A) > return true; The simplest is really the code as it is... but it's not clear, and would deserve a comment. But I very much prefer self-documenting code over comments explaining surprising code. So I'd like to spell out the eDP/DSI transcoders here. BR, Jani. > > Regards > Manasi > >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> Cc: Vandita Kulkarni <vandita.kulkarni@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_vdsc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c >> index e6f60be9ee84..41718f721484 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >> @@ -337,7 +337,10 @@ static const struct rc_parameters *get_rc_params(u16 compressed_bpp, >> bool intel_dsc_source_support(struct intel_encoder *encoder, >> const struct intel_crtc_state *crtc_state) >> { >> + const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> struct drm_i915_private *i915 = to_i915(encoder->base.dev); >> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >> + enum pipe pipe = crtc->pipe; >> >> if (!INTEL_INFO(i915)->display.has_dsc) >> return false; >> @@ -347,7 +350,10 @@ bool intel_dsc_source_support(struct intel_encoder *encoder, >> return true; >> >> if (INTEL_GEN(i915) >= 10 && >> - crtc_state->cpu_transcoder != TRANSCODER_A) >> + (pipe != PIPE_A || >> + (cpu_transcoder == TRANSCODER_EDP || >> + cpu_transcoder == TRANSCODER_DSI_0 || >> + cpu_transcoder == TRANSCODER_DSI_1))) >> return true; >> >> return false; >> -- >> 2.20.1 >>
diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c index e6f60be9ee84..41718f721484 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -337,7 +337,10 @@ static const struct rc_parameters *get_rc_params(u16 compressed_bpp, bool intel_dsc_source_support(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state) { + const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *i915 = to_i915(encoder->base.dev); + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; + enum pipe pipe = crtc->pipe; if (!INTEL_INFO(i915)->display.has_dsc) return false; @@ -347,7 +350,10 @@ bool intel_dsc_source_support(struct intel_encoder *encoder, return true; if (INTEL_GEN(i915) >= 10 && - crtc_state->cpu_transcoder != TRANSCODER_A) + (pipe != PIPE_A || + (cpu_transcoder == TRANSCODER_EDP || + cpu_transcoder == TRANSCODER_DSI_0 || + cpu_transcoder == TRANSCODER_DSI_1))) return true; return false;
The check for cpu_transcoder != TRANSCODER_A is more magic than necessary, and potentially misleading. Before TGL, DSC is supported on pipe A if, and only if, it's used with eDP or DSI transcoders. No functional changes. Cc: Manasi Navare <manasi.d.navare@intel.com> Cc: Vandita Kulkarni <vandita.kulkarni@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/intel_vdsc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)