[RESEND-CI v4 11/15] drm/i915: prepare scaler for YCBCR420 modeset
Sharma, Shashank
shashank.sharma at intel.com
Fri Jun 30 14:27:15 UTC 2017
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 at linux.intel.com>
>>>>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira at intel.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma at 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
>>>
>>
More information about the dri-devel
mailing list