[PATCH] drm/i915/gvt: Refine new_mmio_info to override the added MMIO_reg

[Zhao, Yakui]

On 2018年06月01日 10:46, Colin Xu wrote:
> On 05/31/2018 10:25 AM, Zhao, Yakui wrote:
> 
>> Hi,
>>       Any comment about this patch?
>>       After the overriding is supported, it can reduce the effect on 
>> the other platform when adding one
>> mmio handle for one specific platform.
>>
>> Thanks
>>       Yakui
> 
> trade-off against the convenience:
> - Unnecessary delete and re-add.
> - For a specific mmio, it's unclear how it's handled until go through 
> all init functions, which
> transfers the effort to debugging stage, especially for issue with low 
> fail rate.
> 

Thanks for your comments.

Considering the below scenario:
    One platform needs to specify one new mmio_handler/attribute for one 
mmio_reg while this mmio_reg exists one multi platforms. (Also exist on 
the generic platform).

Based on the current implementation, we need to remove this mmio_reg 
from the generic_mmio_info and then add it for each platform. (Of course 
it is also ok to refine the callback mmio_handler and add the 
platform-check). In such case it is not convenient to handle it.
If we need to handle it for multiple mmio_regs, it will become one 
burden especially when the code needs to be rebased frequently.

If the overriding is supported, this will be quite simple. We only need 
to care how to handle the required mmio_reg for one platform and there 
is no side effect on the other platforms.


> BR
> Colin
> 
>>> -----Original Message-----
>>> From: Zhao, Yakui
>>> Sent: Thursday, May 17, 2018 11:58 AM
>>> To: intel-gvt-dev at lists.freedesktop.org
>>> Cc: Zhao, Yakui <yakui.zhao at intel.com>
>>> Subject: [PATCH] drm/i915/gvt: Refine new_mmio_info to override the 
>>> added
>>> MMIO_reg
>>>
>>> GVT-g uses new_mmio_info function to track the required the mmio_reg. If
>>> the required mmio_reg is already added, error is returned. If one 
>>> platform
>>> hopes to add new specific MMIO read/write callback for one mmio_reg,  
>>> the
>>> change needs to be applied on all the platforms.
>>>
>>> So the new_mmio_info is implemented as override. If it is already 
>>> added, it
>>> will be replaced by the new attributes(mask, read/write callback and 
>>> so on).
>>>
>>> Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/gvt/handlers.c | 25 +++++++++++--------------
>>> 1 file changed, 11 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
>>> b/drivers/gpu/drm/i915/gvt/handlers.c
>>> index 653ba2b..a7bf6d5 100644
>>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>>> @@ -107,22 +107,19 @@ static int new_mmio_info(struct intel_gvt *gvt,
>>>     end = offset + size;
>>>
>>>     for (i = start; i < end; i += 4) {
>>> -        info = kzalloc(sizeof(*info), GFP_KERNEL);
>>> -        if (!info)
>>> -            return -ENOMEM;
>>> -
>>> -        info->offset = i;
>>> -        p = find_mmio_info(gvt, info->offset);
>>> +        p = find_mmio_info(gvt, i);
>>>         if (p) {
>>> -            WARN(1, "dup mmio definition offset %x\n",
>>> -                info->offset);
>>> -            kfree(info);
>>> +            /* It will be removed from hash_list if found */
>>> +            hash_del(&p->node);
>>> +            info = p;
>>> +            gvt_dbg_mmio("VGPU MMIO_reg %x is added.
>>> Override\n",
>>> +                    p->offset);
>>> +        } else {
>>> +            info = kzalloc(sizeof(*info), GFP_KERNEL);
>>> +            if (!info)
>>> +                return -ENOMEM;
>>>
>>> -            /* We return -EEXIST here to make GVT-g load fail.
>>> -             * So duplicated MMIO can be found as soon as
>>> -             * possible.
>>> -             */
>>> -            return -EEXIST;
>>> +            info->offset = i;
>>>         }
>>>
>>>         info->ro_mask = ro_mask;
>>> -- 
>>> 2.7.4
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>