| Message ID | 1498041253-16426-11-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: > To get a YCBCR420 output from intel platforms, we need one > scaler to scale down YCBCR444 samples to YCBCR420 samples. > > This patch: > - Does scaler allocation for HDMI ycbcr420 outputs. > - Programs PIPE_MISC register for ycbcr420 output. > - Adds a new scaler user "HDMI output" to plug-into existing > scaler framework. This output type is identified using bit > 30 of the scaler users bitmap. > > V2: rebase > V3: rebase > V4: rebase > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- > drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ > drivers/gpu/drm/i915/intel_panel.c | 3 ++- > 5 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 36d4e63..a8c9ac5 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, > > /* panel fitter case: assign as a crtc scaler */ > scaler_id = &scaler_state->scaler_id; > + } else if (i == SKL_HDMI_OUTPUT_INDEX) { > + name = "HDMI-OUTPUT"; > + idx = intel_crtc->base.base.id; > + > + /* hdmi output case: needs a pipe scaler */ > + scaler_id = &scaler_state->scaler_id; > } else { > name = "PLANE"; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index da29536..983f581 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > */ > need_scaling = src_w != dst_w || src_h != dst_h; > > + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 > + && scaler_user == SKL_HDMI_OUTPUT_INDEX) Is it really necessary to check for both? If it is, what's the point of creating SKL_HDMI_OUTPUT_INDEX? > + /* YCBCR 444 -> 420 conversion needs a scaler */ > + need_scaling = true; > + > /* > * if plane is being disabled or scaler is no more required or force detach > * - free scaler binded to this plane/crtc > @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > } > > /** > + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. > + * HDMI YCBCR 420 output needs a scaler, for downsampling. > + * > + * @state: crtc's scaler state > + * > + * Return > + * 0 - scaler_usage updated successfully > + * error - requested scaling cannot be supported or other error condition > + */ > +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) > +{ > + const struct drm_display_mode *mode = &state->base.adjusted_mode; > + > + return skl_update_scaler(state, !state->base.active, > + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, > + state->pipe_src_w, state->pipe_src_h, > + mode->crtc_hdisplay, mode->crtc_vdisplay); > +} > + > +/** > * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. > * > * @state: crtc's scaler state > @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > u32 val = 0; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 38fe56c..2206aa8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { > * > * If a bit is set, a user is using a scaler. > * Here user can be a plane or crtc as defined below: > - * bits 0-30 - plane (bit position is index from drm_plane_index) > + * bits 0-29 - plane (bit position is index from drm_plane_index) > + * bit 30 - hdmi output > * bit 31 - crtc > * > * Instead of creating a new index to cover planes and crtc, using > @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { > * avilability. > */ > #define SKL_CRTC_INDEX 31 > + > + /* > + * HDMI YCBCR 420 output consume a scaler. So adding a user > + * for HDMI output 420 requirement. > + */ > +#define SKL_HDMI_OUTPUT_INDEX 30 I'm not convinced about this. The scaler is still used as a pipe scaler and the patch even adds a call intel_pch_panel_fitting(). > unsigned scaler_users; > > /* scaler used by crtc for panel fitting purpose */ > @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, > struct intel_crtc_state *pipe_config); > > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); > int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); > > static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 22da5cd..8d5aa1e 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, > } > > if (type == DRM_HDMI_OUTPUT_YCBCR420) { > + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); > > /* YCBCR420 TMDS rate requirement is half the pixel clock */ > config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, > *clock_12bpc /= 2; > *clock_8bpc /= 2; > > + /* YCBCR 420 output conversion needs a scaler */ > + if (skl_update_scaler_crtc_hdmi_output(config)) { > + DRM_ERROR("Scaler allocation for output failed\n"); > + return DRM_HDMI_OUTPUT_INVALID; > + } > + > + /* Bind this scaler to pipe */ > + intel_pch_panel_fitting(intel_crtc, config, > + DRM_MODE_SCALE_FULLSCREEN); > } > > /* Encoder is capable of this output, lets commit to CRTC */ > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index 96c2cbd..b6a32c4 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, > > /* Native modes don't need fitting */ > if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && > - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) > + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && > + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) I don't like that the knowledge that YCBCR420 needs the scaler is spread everywhere, including this function that should only deal with panel fitting. I would rather add a force parameter to intel_pch_panel_fitting() and skl_update_scaler() that would mean setup the scaler even if looks like it's not necessary. That way the hdmi code would just call them with force enabled, without sprinkling "hdmi_output == YCBCR420" everywhere. Ander > goto done; > > switch (fitting_mode) {
Regards Shashank On 6/27/2017 5:46 PM, Ander Conselvan De Oliveira wrote: > On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: >> To get a YCBCR420 output from intel platforms, we need one >> scaler to scale down YCBCR444 samples to YCBCR420 samples. >> >> This patch: >> - Does scaler allocation for HDMI ycbcr420 outputs. >> - Programs PIPE_MISC register for ycbcr420 output. >> - Adds a new scaler user "HDMI output" to plug-into existing >> scaler framework. This output type is identified using bit >> 30 of the scaler users bitmap. >> >> V2: rebase >> V3: rebase >> V4: rebase >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ >> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- >> drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ >> drivers/gpu/drm/i915/intel_panel.c | 3 ++- >> 5 files changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >> index 36d4e63..a8c9ac5 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, >> >> /* panel fitter case: assign as a crtc scaler */ >> scaler_id = &scaler_state->scaler_id; >> + } else if (i == SKL_HDMI_OUTPUT_INDEX) { >> + name = "HDMI-OUTPUT"; >> + idx = intel_crtc->base.base.id; >> + >> + /* hdmi output case: needs a pipe scaler */ >> + scaler_id = &scaler_state->scaler_id; >> } else { >> name = "PLANE"; >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index da29536..983f581 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, >> */ >> need_scaling = src_w != dst_w || src_h != dst_h; >> >> + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 >> + && scaler_user == SKL_HDMI_OUTPUT_INDEX) > Is it really necessary to check for both? If it is, what's the point of creating > SKL_HDMI_OUTPUT_INDEX? Yes, else this will affect scaler update for planes, as this function gets called to update the plane scalers too, at that time the output will be still valid (as its at CRTC level), but the scaler user would be different. > >> + /* YCBCR 444 -> 420 conversion needs a scaler */ >> + need_scaling = true; >> + >> /* >> * if plane is being disabled or scaler is no more required or force detach >> * - free scaler binded to this plane/crtc >> @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, >> } >> >> /** >> + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. >> + * HDMI YCBCR 420 output needs a scaler, for downsampling. >> + * >> + * @state: crtc's scaler state >> + * >> + * Return >> + * 0 - scaler_usage updated successfully >> + * error - requested scaling cannot be supported or other error condition >> + */ >> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) >> +{ >> + const struct drm_display_mode *mode = &state->base.adjusted_mode; >> + >> + return skl_update_scaler(state, !state->base.active, >> + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, >> + state->pipe_src_w, state->pipe_src_h, >> + mode->crtc_hdisplay, mode->crtc_vdisplay); >> +} >> + >> +/** >> * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. >> * >> * @state: crtc's scaler state >> @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->dev); >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; >> >> if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { >> u32 val = 0; >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 38fe56c..2206aa8 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { >> * >> * If a bit is set, a user is using a scaler. >> * Here user can be a plane or crtc as defined below: >> - * bits 0-30 - plane (bit position is index from drm_plane_index) >> + * bits 0-29 - plane (bit position is index from drm_plane_index) >> + * bit 30 - hdmi output >> * bit 31 - crtc >> * >> * Instead of creating a new index to cover planes and crtc, using >> @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { >> * avilability. >> */ >> #define SKL_CRTC_INDEX 31 >> + >> + /* >> + * HDMI YCBCR 420 output consume a scaler. So adding a user >> + * for HDMI output 420 requirement. >> + */ >> +#define SKL_HDMI_OUTPUT_INDEX 30 > I'm not convinced about this. The scaler is still used as a pipe scaler and the > patch even adds a call intel_pch_panel_fitting(). This is a special mode usage of scalar defined in the bspec, when we want to run the scalar into 5x3 auto mode for YCBCR444->YCBCR420 scale down. It can be used only when the PIPE_MISC is also programmed to act accordingly. So this is different from using it as a panel fitter, and that's why added a new scalar user all together. > >> unsigned scaler_users; >> >> /* scaler used by crtc for panel fitting purpose */ >> @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, >> struct intel_crtc_state *pipe_config); >> >> int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); >> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); >> int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); >> >> static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index 22da5cd..8d5aa1e 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, >> } >> >> if (type == DRM_HDMI_OUTPUT_YCBCR420) { >> + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); >> >> /* YCBCR420 TMDS rate requirement is half the pixel clock */ >> config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; >> @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, >> *clock_12bpc /= 2; >> *clock_8bpc /= 2; >> >> + /* YCBCR 420 output conversion needs a scaler */ >> + if (skl_update_scaler_crtc_hdmi_output(config)) { >> + DRM_ERROR("Scaler allocation for output failed\n"); >> + return DRM_HDMI_OUTPUT_INVALID; >> + } >> + >> + /* Bind this scaler to pipe */ >> + intel_pch_panel_fitting(intel_crtc, config, >> + DRM_MODE_SCALE_FULLSCREEN); >> } >> >> /* Encoder is capable of this output, lets commit to CRTC */ >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index 96c2cbd..b6a32c4 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, >> >> /* Native modes don't need fitting */ >> if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && >> - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) >> + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && >> + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) > I don't like that the knowledge that YCBCR420 needs the scaler is spread > everywhere, including this function that should only deal with panel fitting. I > would rather add a force parameter to intel_pch_panel_fitting() and > skl_update_scaler() that would mean setup the scaler even if looks like it's not > necessary. That way the hdmi code would just call them with force enabled, > without sprinkling "hdmi_output == YCBCR420" everywhere. As explained in the comment above, its special case usage of pipe scalar, and that's why being checked specifically. - Shashank > > Ander > >> goto done; >> >> switch (fitting_mode) {
On Fri, 2017-06-30 at 11:20 +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 6/27/2017 5:46 PM, Ander Conselvan De Oliveira wrote: > > On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: > > > To get a YCBCR420 output from intel platforms, we need one > > > scaler to scale down YCBCR444 samples to YCBCR420 samples. > > > > > > This patch: > > > - Does scaler allocation for HDMI ycbcr420 outputs. > > > - Programs PIPE_MISC register for ycbcr420 output. > > > - Adds a new scaler user "HDMI output" to plug-into existing > > > scaler framework. This output type is identified using bit > > > 30 of the scaler users bitmap. > > > > > > V2: rebase > > > V3: rebase > > > V4: rebase > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- > > > drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ > > > drivers/gpu/drm/i915/intel_panel.c | 3 ++- > > > 5 files changed, 53 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > > index 36d4e63..a8c9ac5 100644 > > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > > @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, > > > > > > /* panel fitter case: assign as a crtc scaler */ > > > scaler_id = &scaler_state->scaler_id; > > > + } else if (i == SKL_HDMI_OUTPUT_INDEX) { > > > + name = "HDMI-OUTPUT"; > > > + idx = intel_crtc->base.base.id; > > > + > > > + /* hdmi output case: needs a pipe scaler */ > > > + scaler_id = &scaler_state->scaler_id; > > > } else { > > > name = "PLANE"; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index da29536..983f581 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > > */ > > > need_scaling = src_w != dst_w || src_h != dst_h; > > > > > > + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 > > > + && scaler_user == SKL_HDMI_OUTPUT_INDEX) > > > > Is it really necessary to check for both? If it is, what's the point of creating > > SKL_HDMI_OUTPUT_INDEX? > > Yes, else this will affect scaler update for planes, as this function > gets called to update the plane scalers too, at that time the output > will be still valid (as its at CRTC level), but the > scaler user would be different. Right, so there is a need to check for scaler_user == HDMI_OUTPUT. But then wouldn't it be a bug if hdmi_output != YCBCR420 ? Point is, if the caller asked for a HDMI_OUTPUT scaler hopefully it knows what its doing, no need to second guess it. > > > > > + /* YCBCR 444 -> 420 conversion needs a scaler */ > > > + need_scaling = true; > > > + > > > /* > > > * if plane is being disabled or scaler is no more required or force detach > > > * - free scaler binded to this plane/crtc > > > @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > > } > > > > > > /** > > > + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. > > > + * HDMI YCBCR 420 output needs a scaler, for downsampling. > > > + * > > > + * @state: crtc's scaler state > > > + * > > > + * Return > > > + * 0 - scaler_usage updated successfully > > > + * error - requested scaling cannot be supported or other error condition > > > + */ > > > +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) > > > +{ > > > + const struct drm_display_mode *mode = &state->base.adjusted_mode; > > > + > > > + return skl_update_scaler(state, !state->base.active, > > > + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, > > > + state->pipe_src_w, state->pipe_src_h, > > > + mode->crtc_hdisplay, mode->crtc_vdisplay); > > > +} > > > + > > > +/** > > > * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. > > > * > > > * @state: crtc's scaler state > > > @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > > > { > > > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; > > > > > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > > > u32 val = 0; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index 38fe56c..2206aa8 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { > > > * > > > * If a bit is set, a user is using a scaler. > > > * Here user can be a plane or crtc as defined below: > > > - * bits 0-30 - plane (bit position is index from drm_plane_index) > > > + * bits 0-29 - plane (bit position is index from drm_plane_index) > > > + * bit 30 - hdmi output > > > * bit 31 - crtc > > > * > > > * Instead of creating a new index to cover planes and crtc, using > > > @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { > > > * avilability. > > > */ > > > #define SKL_CRTC_INDEX 31 > > > + > > > + /* > > > + * HDMI YCBCR 420 output consume a scaler. So adding a user > > > + * for HDMI output 420 requirement. > > > + */ > > > +#define SKL_HDMI_OUTPUT_INDEX 30 > > > > I'm not convinced about this. The scaler is still used as a pipe scaler and the > > patch even adds a call intel_pch_panel_fitting(). > > This is a special mode usage of scalar defined in the bspec, when we > want to run the scalar into 5x3 auto mode for YCBCR444->YCBCR420 scale down. > It can be used only when the PIPE_MISC is also programmed to act > accordingly. So this is different from using it as a panel fitter, and > that's why added > a new scalar user all together. > > > > > unsigned scaler_users; > > > > > > /* scaler used by crtc for panel fitting purpose */ > > > @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, > > > struct intel_crtc_state *pipe_config); > > > > > > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > > > +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); > > > int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); > > > > > > static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > > index 22da5cd..8d5aa1e 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, > > > } > > > > > > if (type == DRM_HDMI_OUTPUT_YCBCR420) { > > > + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); > > > > > > /* YCBCR420 TMDS rate requirement is half the pixel clock */ > > > config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > > > @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, > > > *clock_12bpc /= 2; > > > *clock_8bpc /= 2; > > > > > > + /* YCBCR 420 output conversion needs a scaler */ > > > + if (skl_update_scaler_crtc_hdmi_output(config)) { > > > + DRM_ERROR("Scaler allocation for output failed\n"); > > > + return DRM_HDMI_OUTPUT_INVALID; > > > + } > > > + > > > + /* Bind this scaler to pipe */ > > > + intel_pch_panel_fitting(intel_crtc, config, > > > + DRM_MODE_SCALE_FULLSCREEN); > > > } > > > > > > /* Encoder is capable of this output, lets commit to CRTC */ > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > > index 96c2cbd..b6a32c4 100644 > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, > > > > > > /* Native modes don't need fitting */ > > > if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && > > > - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) > > > + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && > > > + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) > > > > I don't like that the knowledge that YCBCR420 needs the scaler is spread > > everywhere, including this function that should only deal with panel fitting. I > > would rather add a force parameter to intel_pch_panel_fitting() and > > skl_update_scaler() that would mean setup the scaler even if looks like it's not > > necessary. That way the hdmi code would just call them with force enabled, > > without sprinkling "hdmi_output == YCBCR420" everywhere. > > As explained in the comment above, its special case usage of pipe > scalar, and that's why being checked specifically. I'm still not convinced about this. If this is a special scaler mode that is not panel fitting, then this should definitely not touch a function in intel_panel.c, specially one with "panel fitting" in the name. My point is, if this is panel fitting, then it should use panel fitting code without special casing. If it isn't, then it should have its own code paths. But this patch just creates a blurry line where the panel fitting code needs to be special cased for when the HDMI output is YCBCR420 and we are *not* actually doing panel fitting. Ander
Regards Shashank On 6/30/2017 5:04 PM, Ander Conselvan De Oliveira wrote: > On Fri, 2017-06-30 at 11:20 +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 6/27/2017 5:46 PM, Ander Conselvan De Oliveira wrote: >>> On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: >>>> To get a YCBCR420 output from intel platforms, we need one >>>> scaler to scale down YCBCR444 samples to YCBCR420 samples. >>>> >>>> This patch: >>>> - Does scaler allocation for HDMI ycbcr420 outputs. >>>> - Programs PIPE_MISC register for ycbcr420 output. >>>> - Adds a new scaler user "HDMI output" to plug-into existing >>>> scaler framework. This output type is identified using bit >>>> 30 of the scaler users bitmap. >>>> >>>> V2: rebase >>>> V3: rebase >>>> V4: rebase >>>> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ >>>> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- >>>> drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ >>>> drivers/gpu/drm/i915/intel_panel.c | 3 ++- >>>> 5 files changed, 53 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >>>> index 36d4e63..a8c9ac5 100644 >>>> --- a/drivers/gpu/drm/i915/intel_atomic.c >>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c >>>> @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, >>>> >>>> /* panel fitter case: assign as a crtc scaler */ >>>> scaler_id = &scaler_state->scaler_id; >>>> + } else if (i == SKL_HDMI_OUTPUT_INDEX) { >>>> + name = "HDMI-OUTPUT"; >>>> + idx = intel_crtc->base.base.id; >>>> + >>>> + /* hdmi output case: needs a pipe scaler */ >>>> + scaler_id = &scaler_state->scaler_id; >>>> } else { >>>> name = "PLANE"; >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index da29536..983f581 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, >>>> */ >>>> need_scaling = src_w != dst_w || src_h != dst_h; >>>> >>>> + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 >>>> + && scaler_user == SKL_HDMI_OUTPUT_INDEX) >>> Is it really necessary to check for both? If it is, what's the point of creating >>> SKL_HDMI_OUTPUT_INDEX? >> Yes, else this will affect scaler update for planes, as this function >> gets called to update the plane scalers too, at that time the output >> will be still valid (as its at CRTC level), but the >> scaler user would be different. > Right, so there is a need to check for scaler_user == HDMI_OUTPUT. But then > wouldn't it be a bug if hdmi_output != YCBCR420 ? Point is, if the caller asked > for a HDMI_OUTPUT scaler hopefully it knows what its doing, no need to second > guess it. skl_update_scaler function gets called for all the users, which are: - panel fitter - all the planes - newly added user hdmi_output what about the case (assume) when we have handled hdmi_output and now we are handling for a plane, which doesnt need scaling. in this case, the code will look like: if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) need_scaling = true /* which is wrong, as this function call is for plane scalar, and scaler_user = scaler */ if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 && scaler_user == HDMI_OUT) need_scaling = true /* this is correct, as I want to consume scalar only when both the conditions are true. >>>> + /* YCBCR 444 -> 420 conversion needs a scaler */ >>>> + need_scaling = true; >>>> + >>>> /* >>>> * if plane is being disabled or scaler is no more required or force detach >>>> * - free scaler binded to this plane/crtc >>>> @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, >>>> } >>>> >>>> /** >>>> + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. >>>> + * HDMI YCBCR 420 output needs a scaler, for downsampling. >>>> + * >>>> + * @state: crtc's scaler state >>>> + * >>>> + * Return >>>> + * 0 - scaler_usage updated successfully >>>> + * error - requested scaling cannot be supported or other error condition >>>> + */ >>>> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) >>>> +{ >>>> + const struct drm_display_mode *mode = &state->base.adjusted_mode; >>>> + >>>> + return skl_update_scaler(state, !state->base.active, >>>> + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, >>>> + state->pipe_src_w, state->pipe_src_h, >>>> + mode->crtc_hdisplay, mode->crtc_vdisplay); >>>> +} >>>> + >>>> +/** >>>> * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. >>>> * >>>> * @state: crtc's scaler state >>>> @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) >>>> { >>>> struct drm_i915_private *dev_priv = to_i915(crtc->dev); >>>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; >>>> >>>> if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { >>>> u32 val = 0; >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>> index 38fe56c..2206aa8 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { >>>> * >>>> * If a bit is set, a user is using a scaler. >>>> * Here user can be a plane or crtc as defined below: >>>> - * bits 0-30 - plane (bit position is index from drm_plane_index) >>>> + * bits 0-29 - plane (bit position is index from drm_plane_index) >>>> + * bit 30 - hdmi output >>>> * bit 31 - crtc >>>> * >>>> * Instead of creating a new index to cover planes and crtc, using >>>> @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { >>>> * avilability. >>>> */ >>>> #define SKL_CRTC_INDEX 31 >>>> + >>>> + /* >>>> + * HDMI YCBCR 420 output consume a scaler. So adding a user >>>> + * for HDMI output 420 requirement. >>>> + */ >>>> +#define SKL_HDMI_OUTPUT_INDEX 30 >>> I'm not convinced about this. The scaler is still used as a pipe scaler and the >>> patch even adds a call intel_pch_panel_fitting(). >> This is a special mode usage of scalar defined in the bspec, when we >> want to run the scalar into 5x3 auto mode for YCBCR444->YCBCR420 scale down. >> It can be used only when the PIPE_MISC is also programmed to act >> accordingly. So this is different from using it as a panel fitter, and >> that's why added >> a new scalar user all together. >>>> unsigned scaler_users; >>>> >>>> /* scaler used by crtc for panel fitting purpose */ >>>> @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, >>>> struct intel_crtc_state *pipe_config); >>>> >>>> int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); >>>> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); >>>> int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); >>>> >>>> static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>> index 22da5cd..8d5aa1e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>> @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, >>>> } >>>> >>>> if (type == DRM_HDMI_OUTPUT_YCBCR420) { >>>> + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); >>>> >>>> /* YCBCR420 TMDS rate requirement is half the pixel clock */ >>>> config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; >>>> @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, >>>> *clock_12bpc /= 2; >>>> *clock_8bpc /= 2; >>>> >>>> + /* YCBCR 420 output conversion needs a scaler */ >>>> + if (skl_update_scaler_crtc_hdmi_output(config)) { >>>> + DRM_ERROR("Scaler allocation for output failed\n"); >>>> + return DRM_HDMI_OUTPUT_INVALID; >>>> + } >>>> + >>>> + /* Bind this scaler to pipe */ >>>> + intel_pch_panel_fitting(intel_crtc, config, >>>> + DRM_MODE_SCALE_FULLSCREEN); >>>> } >>>> >>>> /* Encoder is capable of this output, lets commit to CRTC */ >>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >>>> index 96c2cbd..b6a32c4 100644 >>>> --- a/drivers/gpu/drm/i915/intel_panel.c >>>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>>> @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, >>>> >>>> /* Native modes don't need fitting */ >>>> if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && >>>> - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) >>>> + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && >>>> + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) >>> I don't like that the knowledge that YCBCR420 needs the scaler is spread >>> everywhere, including this function that should only deal with panel fitting. I >>> would rather add a force parameter to intel_pch_panel_fitting() and >>> skl_update_scaler() that would mean setup the scaler even if looks like it's not >>> necessary. That way the hdmi code would just call them with force enabled, >>> without sprinkling "hdmi_output == YCBCR420" everywhere. >> As explained in the comment above, its special case usage of pipe >> scalar, and that's why being checked specifically. > I'm still not convinced about this. If this is a special scaler mode that is not > panel fitting, then this should definitely not touch a function in > intel_panel.c, specially one with "panel fitting" in the name. > > My point is, if this is panel fitting, then it should use panel fitting code > without special casing. If it isn't, then it should have its own code paths. But > this patch just creates a blurry line where the panel fitting code needs to be > special cased for when the HDMI output is YCBCR420 and we are *not* actually > doing panel fitting. My understanding of the code is (please correct me if that's not) that any pipe scalar usages we are keeping it at panel_fitting level, whereas any plane level stuff is called plane_scalar. As this new user was a usage of pipe_scalar with a special configuration and setting, I accommodated the code in panel fitter level. We can also create new code for it, but that would be unnecessary duplication of code which does the same thing, isn't it ? For this usage, we just need to attach a pipe scalar, and then program the PIPEMISC (which we do in crtc->mode_set). Do you think its any good reason for write a new code path altogether ? - Shashank > > Ander >
On Fri, 2017-06-30 at 17:29 +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 6/30/2017 5:04 PM, Ander Conselvan De Oliveira wrote: > > On Fri, 2017-06-30 at 11:20 +0530, Sharma, Shashank wrote: > > > Regards > > > > > > Shashank > > > > > > > > > On 6/27/2017 5:46 PM, Ander Conselvan De Oliveira wrote: > > > > On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: > > > > > To get a YCBCR420 output from intel platforms, we need one > > > > > scaler to scale down YCBCR444 samples to YCBCR420 samples. > > > > > > > > > > This patch: > > > > > - Does scaler allocation for HDMI ycbcr420 outputs. > > > > > - Programs PIPE_MISC register for ycbcr420 output. > > > > > - Adds a new scaler user "HDMI output" to plug-into existing > > > > > scaler framework. This output type is identified using bit > > > > > 30 of the scaler users bitmap. > > > > > > > > > > V2: rebase > > > > > V3: rebase > > > > > V4: rebase > > > > > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ > > > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ > > > > > drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- > > > > > drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ > > > > > drivers/gpu/drm/i915/intel_panel.c | 3 ++- > > > > > 5 files changed, 53 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > > > > index 36d4e63..a8c9ac5 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > > > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > > > > @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, > > > > > > > > > > /* panel fitter case: assign as a crtc scaler */ > > > > > scaler_id = &scaler_state->scaler_id; > > > > > + } else if (i == SKL_HDMI_OUTPUT_INDEX) { > > > > > + name = "HDMI-OUTPUT"; > > > > > + idx = intel_crtc->base.base.id; > > > > > + > > > > > + /* hdmi output case: needs a pipe scaler */ > > > > > + scaler_id = &scaler_state->scaler_id; > > > > > } else { > > > > > name = "PLANE"; > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index da29536..983f581 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > > > > */ > > > > > need_scaling = src_w != dst_w || src_h != dst_h; > > > > > > > > > > + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 > > > > > + && scaler_user == SKL_HDMI_OUTPUT_INDEX) > > > > > > > > Is it really necessary to check for both? If it is, what's the point of creating > > > > SKL_HDMI_OUTPUT_INDEX? > > > > > > Yes, else this will affect scaler update for planes, as this function > > > gets called to update the plane scalers too, at that time the output > > > will be still valid (as its at CRTC level), but the > > > scaler user would be different. > > > > Right, so there is a need to check for scaler_user == HDMI_OUTPUT. But then > > wouldn't it be a bug if hdmi_output != YCBCR420 ? Point is, if the caller asked > > for a HDMI_OUTPUT scaler hopefully it knows what its doing, no need to second > > guess it. > > skl_update_scaler function gets called for all the users, which are: > - panel fitter > - all the planes > - newly added user hdmi_output > what about the case (assume) when we have handled hdmi_output and now we > are handling for a plane, which doesnt need scaling. > > in this case, the code will look like: > if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) > need_scaling = true /* which is wrong, as this function call is > for plane scalar, and scaler_user = scaler */ > > if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 && > scaler_user == HDMI_OUT) > need_scaling = true /* this is correct, as I want to consume scalar only when both the conditions are true. I meant to do just if (scaler_user == SKL_HDMI_OUTPUT_INDEX) ...; I failed to notice the ambiguity in abbreviating to just HDMI_OUTPUT. But the caller shouldn't request it if it doesn't need it, right? > > > > > > + /* YCBCR 444 -> 420 conversion needs a scaler */ > > > > > + need_scaling = true; > > > > > + > > > > > /* > > > > > * if plane is being disabled or scaler is no more required or force detach > > > > > * - free scaler binded to this plane/crtc > > > > > @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > > > > } > > > > > > > > > > /** > > > > > + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. > > > > > + * HDMI YCBCR 420 output needs a scaler, for downsampling. > > > > > + * > > > > > + * @state: crtc's scaler state > > > > > + * > > > > > + * Return > > > > > + * 0 - scaler_usage updated successfully > > > > > + * error - requested scaling cannot be supported or other error condition > > > > > + */ > > > > > +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) > > > > > +{ > > > > > + const struct drm_display_mode *mode = &state->base.adjusted_mode; > > > > > + > > > > > + return skl_update_scaler(state, !state->base.active, > > > > > + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, > > > > > + state->pipe_src_w, state->pipe_src_h, > > > > > + mode->crtc_hdisplay, mode->crtc_vdisplay); > > > > > +} > > > > > + > > > > > +/** > > > > > * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. > > > > > * > > > > > * @state: crtc's scaler state > > > > > @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > > > > > { > > > > > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > > > > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > > + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; > > > > > > > > > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > > > > > u32 val = 0; > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > > > index 38fe56c..2206aa8 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { > > > > > * > > > > > * If a bit is set, a user is using a scaler. > > > > > * Here user can be a plane or crtc as defined below: > > > > > - * bits 0-30 - plane (bit position is index from drm_plane_index) > > > > > + * bits 0-29 - plane (bit position is index from drm_plane_index) > > > > > + * bit 30 - hdmi output > > > > > * bit 31 - crtc > > > > > * > > > > > * Instead of creating a new index to cover planes and crtc, using > > > > > @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { > > > > > * avilability. > > > > > */ > > > > > #define SKL_CRTC_INDEX 31 > > > > > + > > > > > + /* > > > > > + * HDMI YCBCR 420 output consume a scaler. So adding a user > > > > > + * for HDMI output 420 requirement. > > > > > + */ > > > > > +#define SKL_HDMI_OUTPUT_INDEX 30 > > > > > > > > I'm not convinced about this. The scaler is still used as a pipe scaler and the > > > > patch even adds a call intel_pch_panel_fitting(). > > > > > > This is a special mode usage of scalar defined in the bspec, when we > > > want to run the scalar into 5x3 auto mode for YCBCR444->YCBCR420 scale down. > > > It can be used only when the PIPE_MISC is also programmed to act > > > accordingly. So this is different from using it as a panel fitter, and > > > that's why added > > > a new scalar user all together. > > > > > unsigned scaler_users; > > > > > > > > > > /* scaler used by crtc for panel fitting purpose */ > > > > > @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, > > > > > struct intel_crtc_state *pipe_config); > > > > > > > > > > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > > > > > +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); > > > > > int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); > > > > > > > > > > static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > > > > index 22da5cd..8d5aa1e 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > > > @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, > > > > > } > > > > > > > > > > if (type == DRM_HDMI_OUTPUT_YCBCR420) { > > > > > + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); > > > > > > > > > > /* YCBCR420 TMDS rate requirement is half the pixel clock */ > > > > > config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > > > > > @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, > > > > > *clock_12bpc /= 2; > > > > > *clock_8bpc /= 2; > > > > > > > > > > + /* YCBCR 420 output conversion needs a scaler */ > > > > > + if (skl_update_scaler_crtc_hdmi_output(config)) { > > > > > + DRM_ERROR("Scaler allocation for output failed\n"); > > > > > + return DRM_HDMI_OUTPUT_INVALID; > > > > > + } > > > > > + > > > > > + /* Bind this scaler to pipe */ > > > > > + intel_pch_panel_fitting(intel_crtc, config, > > > > > + DRM_MODE_SCALE_FULLSCREEN); > > > > > } > > > > > > > > > > /* Encoder is capable of this output, lets commit to CRTC */ > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > > > > index 96c2cbd..b6a32c4 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > > @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, > > > > > > > > > > /* Native modes don't need fitting */ > > > > > if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && > > > > > - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) > > > > > + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && > > > > > + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) > > > > > > > > I don't like that the knowledge that YCBCR420 needs the scaler is spread > > > > everywhere, including this function that should only deal with panel fitting. I > > > > would rather add a force parameter to intel_pch_panel_fitting() and > > > > skl_update_scaler() that would mean setup the scaler even if looks like it's not > > > > necessary. That way the hdmi code would just call them with force enabled, > > > > without sprinkling "hdmi_output == YCBCR420" everywhere. > > > > > > As explained in the comment above, its special case usage of pipe > > > scalar, and that's why being checked specifically. > > > > I'm still not convinced about this. If this is a special scaler mode that is not > > panel fitting, then this should definitely not touch a function in > > intel_panel.c, specially one with "panel fitting" in the name. > > > > My point is, if this is panel fitting, then it should use panel fitting code > > without special casing. If it isn't, then it should have its own code paths. But > > this patch just creates a blurry line where the panel fitting code needs to be > > special cased for when the HDMI output is YCBCR420 and we are *not* actually > > doing panel fitting. > > My understanding of the code is (please correct me if that's not) that > any pipe scalar usages we are keeping it at > panel_fitting level, whereas any plane level stuff is called > plane_scalar. As this new user was a usage of pipe_scalar > with a special configuration and setting, I accommodated the code in > panel fitter level. We can also create new code > for it, but that would be unnecessary duplication of code which does the > same thing, isn't it ? The code as it is in this patch asks for something called an HDMI_OUTPUT scaler, and then when it is time to enable it, it asks the panel fitter to be enabled. Notice the inconsistency? I like to either call it a panel fitter and treat it as panel fitter, or call it a new type of scaler and have a proper interface for it. I don't think the current solution reused much of the code anyway. The relevant part from intel_pch_panel_fitting() is case DRM_MODE_SCALE_FULLSCREEN: x = y = 0; width = adjusted_mode->crtc_hdisplay; height = adjusted_mode->crtc_vdisplay; break; pipe_config->pch_pfit.pos = (x << 16) | y; pipe_config->pch_pfit.size = (width << 16) | height; pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0; . The other part is recycling the call to skylake_pfit_enable() in haswell_crtc_enable(). That call is just scaler register programming, basically: if (... pch_pfit.enabled) { ... id = scaler_state->scaler_id; I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN | PS_FILTER_MEDIUM | scaler_state->scalers[id].mode); I915_WRITE(SKL_PS_WIN_POS(pipe, id), crtc->config->pch_pfit.pos); I915_WRITE(SKL_PS_WIN_SZ(pipe, id), crtc->config->pch_pfit.size); } I think the issue here is that there isn't a good layer of abstraction between "skylake" panel fitting and the scalers. It would probably be much better if the register programming logic for them would be in a scaler_enable() function since that is duplicated here and there. It would probably be good to have a proper struct scaler that we can pass to certain functions too. Anyway, I think the scaler interface could be improved in general. I'm not saying it should be done as part of this patch, but at the same time I'd like to avoid creating an even bigger mess to sort out in the future. To sum it up, I'd be equaly fine with either of the proposals below: - find a way to not have a special HDMI OUTPUT scaler and reuse the panel fitting code completely, without sprinkling checks for YCBCR420 into it; - create a new path for the new type of scaler, completely independent of panel fitting. Anything else makes it unclear which part of the code is responsible for what, which I believe is a bad thing. Ander > For this usage, we just need to attach a pipe scalar, and then program > the PIPEMISC (which we do in crtc->mode_set). Do you > think its any good reason for write a new code path altogether ? > > - Shashank > > > > Ander > > > >
Regards Shashank On 6/30/2017 7:45 PM, Ander Conselvan De Oliveira wrote: > On Fri, 2017-06-30 at 17:29 +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 6/30/2017 5:04 PM, Ander Conselvan De Oliveira wrote: >>> On Fri, 2017-06-30 at 11:20 +0530, Sharma, Shashank wrote: >>>> Regards >>>> >>>> Shashank >>>> >>>> >>>> On 6/27/2017 5:46 PM, Ander Conselvan De Oliveira wrote: >>>>> On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: >>>>>> To get a YCBCR420 output from intel platforms, we need one >>>>>> scaler to scale down YCBCR444 samples to YCBCR420 samples. >>>>>> >>>>>> This patch: >>>>>> - Does scaler allocation for HDMI ycbcr420 outputs. >>>>>> - Programs PIPE_MISC register for ycbcr420 output. >>>>>> - Adds a new scaler user "HDMI output" to plug-into existing >>>>>> scaler framework. This output type is identified using bit >>>>>> 30 of the scaler users bitmap. >>>>>> >>>>>> V2: rebase >>>>>> V3: rebase >>>>>> V4: rebase >>>>>> >>>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> >>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ >>>>>> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ >>>>>> drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- >>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ >>>>>> drivers/gpu/drm/i915/intel_panel.c | 3 ++- >>>>>> 5 files changed, 53 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >>>>>> index 36d4e63..a8c9ac5 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c >>>>>> @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, >>>>>> >>>>>> /* panel fitter case: assign as a crtc scaler */ >>>>>> scaler_id = &scaler_state->scaler_id; >>>>>> + } else if (i == SKL_HDMI_OUTPUT_INDEX) { >>>>>> + name = "HDMI-OUTPUT"; >>>>>> + idx = intel_crtc->base.base.id; >>>>>> + >>>>>> + /* hdmi output case: needs a pipe scaler */ >>>>>> + scaler_id = &scaler_state->scaler_id; >>>>>> } else { >>>>>> name = "PLANE"; >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>> index da29536..983f581 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>> @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, >>>>>> */ >>>>>> need_scaling = src_w != dst_w || src_h != dst_h; >>>>>> >>>>>> + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 >>>>>> + && scaler_user == SKL_HDMI_OUTPUT_INDEX) >>>>> Is it really necessary to check for both? If it is, what's the point of creating >>>>> SKL_HDMI_OUTPUT_INDEX? >>>> Yes, else this will affect scaler update for planes, as this function >>>> gets called to update the plane scalers too, at that time the output >>>> will be still valid (as its at CRTC level), but the >>>> scaler user would be different. >>> Right, so there is a need to check for scaler_user == HDMI_OUTPUT. But then >>> wouldn't it be a bug if hdmi_output != YCBCR420 ? Point is, if the caller asked >>> for a HDMI_OUTPUT scaler hopefully it knows what its doing, no need to second >>> guess it. >> skl_update_scaler function gets called for all the users, which are: >> - panel fitter >> - all the planes >> - newly added user hdmi_output >> what about the case (assume) when we have handled hdmi_output and now we >> are handling for a plane, which doesnt need scaling. >> >> in this case, the code will look like: >> if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) >> need_scaling = true /* which is wrong, as this function call is >> for plane scalar, and scaler_user = scaler */ >> >> if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 && >> scaler_user == HDMI_OUT) >> need_scaling = true /* this is correct, as I want to consume scalar only when both the conditions are true. > > I meant to do just > > if (scaler_user == SKL_HDMI_OUTPUT_INDEX) > ...; > > I failed to notice the ambiguity in abbreviating to just HDMI_OUTPUT. But the > caller shouldn't request it if it doesn't need it, right? I agree, I dont see a reason why not ! Will do the needful. Regards Shashank >>>>>> + /* YCBCR 444 -> 420 conversion needs a scaler */ >>>>>> + need_scaling = true; >>>>>> + >>>>>> /* >>>>>> * if plane is being disabled or scaler is no more required or force detach >>>>>> * - free scaler binded to this plane/crtc >>>>>> @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, >>>>>> } >>>>>> >>>>>> /** >>>>>> + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. >>>>>> + * HDMI YCBCR 420 output needs a scaler, for downsampling. >>>>>> + * >>>>>> + * @state: crtc's scaler state >>>>>> + * >>>>>> + * Return >>>>>> + * 0 - scaler_usage updated successfully >>>>>> + * error - requested scaling cannot be supported or other error condition >>>>>> + */ >>>>>> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) >>>>>> +{ >>>>>> + const struct drm_display_mode *mode = &state->base.adjusted_mode; >>>>>> + >>>>>> + return skl_update_scaler(state, !state->base.active, >>>>>> + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, >>>>>> + state->pipe_src_w, state->pipe_src_h, >>>>>> + mode->crtc_hdisplay, mode->crtc_vdisplay); >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. >>>>>> * >>>>>> * @state: crtc's scaler state >>>>>> @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) >>>>>> { >>>>>> struct drm_i915_private *dev_priv = to_i915(crtc->dev); >>>>>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>>>> + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; >>>>>> >>>>>> if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { >>>>>> u32 val = 0; >>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>>>> index 38fe56c..2206aa8 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>>> @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { >>>>>> * >>>>>> * If a bit is set, a user is using a scaler. >>>>>> * Here user can be a plane or crtc as defined below: >>>>>> - * bits 0-30 - plane (bit position is index from drm_plane_index) >>>>>> + * bits 0-29 - plane (bit position is index from drm_plane_index) >>>>>> + * bit 30 - hdmi output >>>>>> * bit 31 - crtc >>>>>> * >>>>>> * Instead of creating a new index to cover planes and crtc, using >>>>>> @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { >>>>>> * avilability. >>>>>> */ >>>>>> #define SKL_CRTC_INDEX 31 >>>>>> + >>>>>> + /* >>>>>> + * HDMI YCBCR 420 output consume a scaler. So adding a user >>>>>> + * for HDMI output 420 requirement. >>>>>> + */ >>>>>> +#define SKL_HDMI_OUTPUT_INDEX 30 >>>>> I'm not convinced about this. The scaler is still used as a pipe scaler and the >>>>> patch even adds a call intel_pch_panel_fitting(). >>>> This is a special mode usage of scalar defined in the bspec, when we >>>> want to run the scalar into 5x3 auto mode for YCBCR444->YCBCR420 scale down. >>>> It can be used only when the PIPE_MISC is also programmed to act >>>> accordingly. So this is different from using it as a panel fitter, and >>>> that's why added >>>> a new scalar user all together. >>>>>> unsigned scaler_users; >>>>>> >>>>>> /* scaler used by crtc for panel fitting purpose */ >>>>>> @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, >>>>>> struct intel_crtc_state *pipe_config); >>>>>> >>>>>> int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); >>>>>> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); >>>>>> int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); >>>>>> >>>>>> static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) >>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> index 22da5cd..8d5aa1e 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, >>>>>> } >>>>>> >>>>>> if (type == DRM_HDMI_OUTPUT_YCBCR420) { >>>>>> + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); >>>>>> >>>>>> /* YCBCR420 TMDS rate requirement is half the pixel clock */ >>>>>> config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; >>>>>> @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, >>>>>> *clock_12bpc /= 2; >>>>>> *clock_8bpc /= 2; >>>>>> >>>>>> + /* YCBCR 420 output conversion needs a scaler */ >>>>>> + if (skl_update_scaler_crtc_hdmi_output(config)) { >>>>>> + DRM_ERROR("Scaler allocation for output failed\n"); >>>>>> + return DRM_HDMI_OUTPUT_INVALID; >>>>>> + } >>>>>> + >>>>>> + /* Bind this scaler to pipe */ >>>>>> + intel_pch_panel_fitting(intel_crtc, config, >>>>>> + DRM_MODE_SCALE_FULLSCREEN); >>>>>> } >>>>>> >>>>>> /* Encoder is capable of this output, lets commit to CRTC */ >>>>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >>>>>> index 96c2cbd..b6a32c4 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_panel.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>>>>> @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, >>>>>> >>>>>> /* Native modes don't need fitting */ >>>>>> if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && >>>>>> - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) >>>>>> + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && >>>>>> + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) >>>>> I don't like that the knowledge that YCBCR420 needs the scaler is spread >>>>> everywhere, including this function that should only deal with panel fitting. I >>>>> would rather add a force parameter to intel_pch_panel_fitting() and >>>>> skl_update_scaler() that would mean setup the scaler even if looks like it's not >>>>> necessary. That way the hdmi code would just call them with force enabled, >>>>> without sprinkling "hdmi_output == YCBCR420" everywhere. >>>> As explained in the comment above, its special case usage of pipe >>>> scalar, and that's why being checked specifically. >>> I'm still not convinced about this. If this is a special scaler mode that is not >>> panel fitting, then this should definitely not touch a function in >>> intel_panel.c, specially one with "panel fitting" in the name. >>> >>> My point is, if this is panel fitting, then it should use panel fitting code >>> without special casing. If it isn't, then it should have its own code paths. But >>> this patch just creates a blurry line where the panel fitting code needs to be >>> special cased for when the HDMI output is YCBCR420 and we are *not* actually >>> doing panel fitting. >> My understanding of the code is (please correct me if that's not) that >> any pipe scalar usages we are keeping it at >> panel_fitting level, whereas any plane level stuff is called >> plane_scalar. As this new user was a usage of pipe_scalar >> with a special configuration and setting, I accommodated the code in >> panel fitter level. We can also create new code >> for it, but that would be unnecessary duplication of code which does the >> same thing, isn't it ? > The code as it is in this patch asks for something called an HDMI_OUTPUT scaler, > and then when it is time to enable it, it asks the panel fitter to be enabled. > Notice the inconsistency? > > I like to either call it a panel fitter and treat it as panel fitter, or call it > a new type of scaler and have a proper interface for it. I don't think the > current solution reused much of the code anyway. The relevant part from > intel_pch_panel_fitting() is > > case DRM_MODE_SCALE_FULLSCREEN: > x = y = 0; > width = adjusted_mode->crtc_hdisplay; > height = adjusted_mode->crtc_vdisplay; > break; > > pipe_config->pch_pfit.pos = (x << 16) | y; > pipe_config->pch_pfit.size = (width << 16) | height; > pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0; > > . The other part is recycling the call to skylake_pfit_enable() in > haswell_crtc_enable(). That call is just scaler register programming, basically: > > if (... pch_pfit.enabled) { > ... > id = scaler_state->scaler_id; > I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN | > PS_FILTER_MEDIUM | scaler_state->scalers[id].mode); > I915_WRITE(SKL_PS_WIN_POS(pipe, id), crtc->config->pch_pfit.pos); > I915_WRITE(SKL_PS_WIN_SZ(pipe, id), crtc->config->pch_pfit.size); > } > > I think the issue here is that there isn't a good layer of abstraction between > "skylake" panel fitting and the scalers. It would probably be much better if the > register programming logic for them would be in a scaler_enable() function since > that is duplicated here and there. It would probably be good to have a proper > struct scaler that we can pass to certain functions too. > > Anyway, I think the scaler interface could be improved in general. I'm not > saying it should be done as part of this patch, but at the same time I'd like to > avoid creating an even bigger mess to sort out in the future. To sum it up, I'd > be equaly fine with either of the proposals below: > > - find a way to not have a special HDMI OUTPUT scaler and reuse the panel > fitting code completely, without sprinkling checks for YCBCR420 into it; > - create a new path for the new type of scaler, completely independent of panel > fitting. > > Anything else makes it unclear which part of the code is responsible for what, > which I believe is a bad thing. > > Ander > >> For this usage, we just need to attach a pipe scalar, and then program >> the PIPEMISC (which we do in crtc->mode_set). Do you >> think its any good reason for write a new code path altogether ? >> >> - Shashank >>> Ander >>> >>
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 36d4e63..a8c9ac5 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, /* panel fitter case: assign as a crtc scaler */ scaler_id = &scaler_state->scaler_id; + } else if (i == SKL_HDMI_OUTPUT_INDEX) { + name = "HDMI-OUTPUT"; + idx = intel_crtc->base.base.id; + + /* hdmi output case: needs a pipe scaler */ + scaler_id = &scaler_state->scaler_id; } else { name = "PLANE"; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index da29536..983f581 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, */ need_scaling = src_w != dst_w || src_h != dst_h; + if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420 + && scaler_user == SKL_HDMI_OUTPUT_INDEX) + /* YCBCR 444 -> 420 conversion needs a scaler */ + need_scaling = true; + /* * if plane is being disabled or scaler is no more required or force detach * - free scaler binded to this plane/crtc @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, } /** + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI. + * HDMI YCBCR 420 output needs a scaler, for downsampling. + * + * @state: crtc's scaler state + * + * Return + * 0 - scaler_usage updated successfully + * error - requested scaling cannot be supported or other error condition + */ +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) +{ + const struct drm_display_mode *mode = &state->base.adjusted_mode; + + return skl_update_scaler(state, !state->base.active, + SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id, + state->pipe_src_w, state->pipe_src_h, + mode->crtc_hdisplay, mode->crtc_vdisplay); +} + +/** * skl_update_scaler_crtc - Stages update to scaler state for a given crtc. * * @state: crtc's scaler state @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output; if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { u32 val = 0; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 38fe56c..2206aa8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { * * If a bit is set, a user is using a scaler. * Here user can be a plane or crtc as defined below: - * bits 0-30 - plane (bit position is index from drm_plane_index) + * bits 0-29 - plane (bit position is index from drm_plane_index) + * bit 30 - hdmi output * bit 31 - crtc * * Instead of creating a new index to cover planes and crtc, using @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { * avilability. */ #define SKL_CRTC_INDEX 31 + + /* + * HDMI YCBCR 420 output consume a scaler. So adding a user + * for HDMI output 420 requirement. + */ +#define SKL_HDMI_OUTPUT_INDEX 30 unsigned scaler_users; /* scaler used by crtc for panel fitting purpose */ @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_state *pipe_config); int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 22da5cd..8d5aa1e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, } if (type == DRM_HDMI_OUTPUT_YCBCR420) { + struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc); /* YCBCR420 TMDS rate requirement is half the pixel clock */ config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, *clock_12bpc /= 2; *clock_8bpc /= 2; + /* YCBCR 420 output conversion needs a scaler */ + if (skl_update_scaler_crtc_hdmi_output(config)) { + DRM_ERROR("Scaler allocation for output failed\n"); + return DRM_HDMI_OUTPUT_INVALID; + } + + /* Bind this scaler to pipe */ + intel_pch_panel_fitting(intel_crtc, config, + DRM_MODE_SCALE_FULLSCREEN); } /* Encoder is capable of this output, lets commit to CRTC */ diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 96c2cbd..b6a32c4 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, /* Native modes don't need fitting */ if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && + pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420) goto done; switch (fitting_mode) {
To get a YCBCR420 output from intel platforms, we need one scaler to scale down YCBCR444 samples to YCBCR420 samples. This patch: - Does scaler allocation for HDMI ycbcr420 outputs. - Programs PIPE_MISC register for ycbcr420 output. - Adds a new scaler user "HDMI output" to plug-into existing scaler framework. This output type is identified using bit 30 of the scaler users bitmap. V2: rebase V3: rebase V4: rebase Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 3 ++- 5 files changed, 53 insertions(+), 2 deletions(-)