[Intel-xe] [PATCH 2/2] drm/xe: Implement xe DPT slightly differently.

[Shankar, Uma]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Jani Nikula
> Sent: Tuesday, August 15, 2023 11:13 PM
> To: Maarten Lankhorst <dev at lankhorst.se>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH 2/2] drm/xe: Implement xe DPT slightly differently.
> 
> On Tue, 15 Aug 2023, Maarten Lankhorst <dev at lankhorst.se> wrote:
> > Even for 1 line changes like this? (Adding an if that is always true
> > on
> > i915.)
> 
> Yes. At the moment the dependencies are pretty complex, because the xe changes
> here depend on a lot of xe commits that in turn depend on a bunch of i915 commits
> that need to be refactored.
> 
> If we can isolate the i915 patches such that they don't depend on anything and can
> be queued upstream first thing, it makes our lives easier.
> 
> And obviously it's *much* easier to squash and juggle patches later on than to split
> them up.

Hi Maarten,
If the changes are mixed, it will create problem in rebases and clear separation etc. 
Its good to have changes in separate patches.

Otherwise the change looks an ok workaround to unblock DPT. Having a custom dummy
implementation in Xe. However this will require a proper handling with clean fix later on.
To unblock Xe development and enable tiling and rest of the DPT dependent test, I feel we
can go with the current approach. A cleaner solution can be planned directly in upstream.
Please re-float the changes and will ack the same.

Regards,
Uma Shankar

> BR,
> Jani.
> 
> 
> >
> > On 2023-08-15 11:23, Jani Nikula wrote:
> >> On Mon, 14 Aug 2023, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> >>> On Mon, Aug 14, 2023 at 10:37:44AM +0200, Maarten Lankhorst wrote:
> >>>> From: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>>
> >>>> Just create a dummy to make DPT work.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst
> >>>> <maarten.lankhorst at linux.intel.com>
> >>>> ---
> >>>>   .../drm/i915/display/skl_universal_plane.c    |  3 +-
> >>>>   drivers/gpu/drm/xe/Makefile                   |  1 +
> >>>>   drivers/gpu/drm/xe/display/xe_dpt.c           | 46 +++++++++++++++++++
> >>>>   3 files changed, 49 insertions(+), 1 deletion(-)
> >>>>   create mode 100644 drivers/gpu/drm/xe/display/xe_dpt.c
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >>>> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >>>> index c28f4198aa15..9469ec5a0417 100644
> >>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >>>
> >>> We need to make i915 and xe changes in separated patches.
> >>> Ideally we make all i915 changes in a way that it could even go
> >>> directly to drm-intel-next already...
> >>
> >> Yes, please!
> >>
> >> BR,
> >> Jani.
> >>
> >>>
> >>> Cc: Jani Nikula <jani.nikula at intel.com>
> >>>
> >>>> @@ -1010,7 +1010,8 @@ static u32 skl_surf_address(const struct
> intel_plane_state *plane_state,
> >>>>   		 * The DPT object contains only one vma, so the VMA's offset
> >>>>   		 * within the DPT is always 0.
> >>>>   		 */
> >>>> -		drm_WARN_ON(&i915->drm, plane_state->dpt_vma->node.start);
> >>>> +		if (plane_state->dpt_vma)
> >>>> +			drm_WARN_ON(&i915->drm, plane_state->dpt_vma-
> >node.start);
> >>>>   		drm_WARN_ON(&i915->drm, offset & 0x1fffff);
> >>>>   		return offset >> 9;
> >>>>   	} else {
> >>>> diff --git a/drivers/gpu/drm/xe/Makefile
> >>>> b/drivers/gpu/drm/xe/Makefile index 6d9196ab275c..33bfe33632db
> >>>> 100644
> >>>> --- a/drivers/gpu/drm/xe/Makefile
> >>>> +++ b/drivers/gpu/drm/xe/Makefile
> >>>> @@ -141,6 +141,7 @@ $(obj)/i915-display/%.o:
> $(srctree)/drivers/gpu/drm/i915/display/%.c FORCE
> >>>>   # Display code specific to xe
> >>>>   xe-$(CONFIG_DRM_XE_DISPLAY) += \
> >>>>   	xe_display.o \
> >>>> +	display/xe_dpt.o \
> >>>>   	display/xe_fb_pin.o \
> >>>>   	display/xe_hdcp_gsc.o \
> >>>>   	display/xe_plane_initial.o \
> >>>> diff --git a/drivers/gpu/drm/xe/display/xe_dpt.c
> >>>> b/drivers/gpu/drm/xe/display/xe_dpt.c
> >>>> new file mode 100644
> >>>> index 000000000000..0695886045e3
> >>>> --- /dev/null
> >>>> +++ b/drivers/gpu/drm/xe/display/xe_dpt.c
> >>>> @@ -0,0 +1,46 @@
> >>>> +// SPDX-License-Identifier: MIT
> >>>> +/*
> >>>> + * Copyright © 2023 Intel Corporation  */
> >>>> +
> >>>> +#include "intel_dpt.h"
> >>>> +
> >>>> +#include "i915_reg.h"
> >>>> +
> >>>> +#include "intel_de.h"
> >>>> +#include "intel_display.h"
> >>>> +#include "intel_display_types.h"
> >>>> +
> >>>> +void intel_dpt_destroy(struct i915_address_space *vm) { }
> >>>> +
> >>>> +struct i915_address_space *
> >>>> +intel_dpt_create(struct intel_framebuffer *fb) {
> >>>> +	return NULL;
> >>>> +}
> >>>> +
> >>>> +void intel_dpt_configure(struct intel_crtc *crtc) {
> >>>> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >>>> +
> >>>> +	if (DISPLAY_VER(i915) == 14) {
> >>>> +		enum pipe pipe = crtc->pipe;
> >>>> +		enum plane_id plane_id;
> >>>> +
> >>>> +		for_each_plane_id_on_crtc(crtc, plane_id) {
> >>>> +			if (plane_id == PLANE_CURSOR)
> >>>> +				continue;
> >>>> +
> >>>> +			intel_de_rmw(i915, PLANE_CHICKEN(pipe, plane_id),
> >>>> +				     PLANE_CHICKEN_DISABLE_DPT,
> >>>> +				     i915->params.enable_dpt ? 0 :
> PLANE_CHICKEN_DISABLE_DPT);
> >>>> +		}
> >>>> +	} else if (DISPLAY_VER(i915) == 13) {
> >>>> +		intel_de_rmw(i915, CHICKEN_MISC_2,
> >>>> +			     CHICKEN_MISC_DISABLE_DPT,
> >>>> +			     i915->params.enable_dpt ? 0 :
> CHICKEN_MISC_DISABLE_DPT);
> >>>> +	}
> >>>> +}
> >>>> +
> >>>> --
> >>>> 2.39.2
> >>>>
> >>
> 
> --
> Jani Nikula, Intel Open Source Graphics Center