Re: [PATCH v2 06/15] drm/i915/dsb: Update i915_write to call dsb-write.

From: Jani Nikula <jani.nikula@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Animesh Manna <animesh.manna@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 06/15] drm/i915/dsb: Update i915_write to call dsb-write.
Date: Thu, 22 Aug 2019 16:11:14 +0300	[thread overview]
Message-ID: <87v9upbb0t.fsf@intel.com> (raw)
In-Reply-To: <156641215065.20466.3333013217026008208@skylake-alporthouse-com>

On Wed, 21 Aug 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Animesh Manna (2019-08-21 07:32:26)
>> Existing mmio-reg-write need intel_uncore handle which is part
>> of dev_priv structure and the same design is followed by
>> adding dsb handle in dev_priv for programming registers through DSB.
>> 
>> I915_WRITE is modified to check for register capability and call
>> dsb-reg-write based on its capability.
>> 
>> No changes in I915_READ definition as DSB do not have support to
>> read any register.
>> 
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
>>  drivers/gpu/drm/i915/i915_drv.h          | 6 +++++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 4fe8cac6246a..6f1999140085 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -123,7 +123,7 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>         u32 *buf = dsb->cmd_buf;
>>  
>>         if (!buf) {
>> -               I915_WRITE(reg, val);
>> +               intel_uncore_write(&(dev_priv)->uncore, reg, val);
>>                 return;
>>         }
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 643fd6d6fd73..7aed957362c9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1753,6 +1753,8 @@ struct drm_i915_private {
>>         /* Mutex to protect the above hdcp component related values. */
>>         struct mutex hdcp_comp_mutex;
>>  
>> +       struct intel_dsb *dsb;
>> +
>>         /*
>>          * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>>          * will be rejected. Instead look for a better place.
>> @@ -2414,7 +2416,9 @@ int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>>         intel_uncore_##op__(&(dev_priv__)->uncore, __VA_ARGS__)
>>  
>>  #define I915_READ(reg__)        __I915_REG_OP(read, dev_priv, (reg__))
>> -#define I915_WRITE(reg__, val__) __I915_REG_OP(write, dev_priv, (reg__), (val__))
>> +#define I915_WRITE(reg__, val__) \
>> +       (reg__.cap) ? intel_dsb_reg_write(dev_priv->dsb, (reg__), (val__)) : \
>> +       __I915_REG_OP(write, dev_priv, (reg__), (val__))
>
> Jani, over to you.

This is not right, in so many levels.

First of all, I don't think we should hide DSB usage in the driver like
this. It needs to be explicit in the code.

I'm not sure if it's even possible to do it like this. When should the
writes take place? Who's going to "commit" the writes to DSB? The DSB
only makes sense for batches. In code with just I915_WRITE, you can't
tell whether the commit is needed or not.

You have a number of DSB engines per pipe, and this forces using just
one in dev_priv.

Please stick to the plan of adding the simplest API that I think we've
agreed on, the one that allows DSB use call sites to switch over to
intel_dsb_reg_write() etc. and allows us to tweak the guts of the DSB
implementation later. Then we don't need to change the DSB use sites so
much, merely the internals.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx