[PATCH 2/4] drm/mgag200: Simplify offset and scale computation.

[Jocelyn Falempe]

On 08/05/2023 09:44, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.05.23 um 14:43 schrieb Jocelyn Falempe:
>> Now that the driver handles only 16, 24 and 32-bit framebuffer,
>> it can be simplified.
> 
> I think it should say that the driver never really handled 8-bit colors. 
> Or at least I'm not aware of.

Ok I can change that. This patch is just a cleanup, and is not really 
necessary for DMA.
> 
>>
>> No functional changes.
>>
>> offset:
>> 16bit: (bppshift = 1)
>> offset = width >> (4 - bppshift) => width / 8 => pitch / 16
>>
>> 24bit:  (bppshift = 0)
>> offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16
>>
>> 32bit:  (bppshift = 2)
>> offset = width >> (4 - bppshift) => width / 4 => pitch / 16
>>
>> scale:
>> 16bit:
>> scale = (1 << bppshift) - 1 => 1
>> 24bit:
>> scale = ((1 << bppshift) * 3) - 1 => 2
>> 32bit:
>> scale = (1 << bppshift) - 1 => 3
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++-------------------
>>   1 file changed, 16 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 9a24b9c00745..7d8c65372ac4 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device 
>> *mdev, const struct drm_display_mod
>>       WREG8(MGA_MISC_OUT, misc);
>>   }
>> -static u8 mgag200_get_bpp_shift(const struct drm_format_info *format)
>> -{
>> -    static const u8 bpp_shift[] = {0, 1, 0, 2};
>> -
>> -    return bpp_shift[format->cpp[0] - 1];
>> -}
>> -
>>   /*
>>    * Calculates the HW offset value from the framebuffer's pitch. The
>>    * offset is a multiple of the pixel size and depends on the display
>> - * format.
>> + * format. With width in pixels and pitch in bytes, the formula is:
>> + * offset = width * bpp / 128 = pitch / 16
>>    */
>>   static u32 mgag200_calculate_offset(struct mga_device *mdev,
>>                       const struct drm_framebuffer *fb)
>>   {
>> -    u32 offset = fb->pitches[0] / fb->format->cpp[0];
>> -    u8 bppshift = mgag200_get_bpp_shift(fb->format);
>> -
>> -    if (fb->format->cpp[0] * 8 == 24)
>> -        offset = (offset * 3) >> (4 - bppshift);
>> -    else
>> -        offset = offset >> (4 - bppshift);
>> -
>> -    return offset;
>> +    return fb->pitches[0] >> 4;
> 
> 24-bit is different from the rest. I don't understand how you simplified 
> this code.

This code was overly complex, but this special case is compensated by 
the "bpp_shift" thing. The formula width * bpp / 128 is in the G200 
documentation 4.6.5

u32 offset = fb->pitches[0] / fb->format->cpp[0];

so offset is now the width in pixels. (pitches[0] is the width in bytes)
bppshift is 0 for 24 bits, 1 for 16 bits and 2 for 32 bits

offset:
16bit: (bppshift = 1) ; pitch = width * 2
offset = width >> (4 - bppshift) => width / 8 => pitch / 16

24bit:  (bppshift = 0) ; pitch = width * 3
offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16

32bit:  (bppshift = 2) ; pith = width * 4
offset = width >> (4 - bppshift) => width / 4 => pitch / 16

> 
> Best regards
> Thomas
> 
>>   }
>>   static void mgag200_set_offset(struct mga_device *mdev,
>> @@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device 
>> *mdev,
>>   void mgag200_set_format_regs(struct mga_device *mdev, const struct 
>> drm_format_info *format)
>>   {
>>       struct drm_device *dev = &mdev->base;
>> -    unsigned int bpp, bppshift, scale;
>> +    unsigned int scale;
>>       u8 crtcext3, xmulctrl;
>> -    bpp = format->cpp[0] * 8;
>> -
>> -    bppshift = mgag200_get_bpp_shift(format);
>> -    switch (bpp) {
>> -    case 24:
>> -        scale = ((1 << bppshift) * 3) - 1;
>> -        break;
>> -    default:
>> -        scale = (1 << bppshift) - 1;
>> -        break;
>> -    }
>> -
>> -    RREG_ECRT(3, crtcext3);
>> -
>> -    switch (bpp) {
>> -    case 8:
>> -        xmulctrl = MGA1064_MUL_CTL_8bits;
>> -        break;
>> -    case 16:
>> -        if (format->depth == 15)
>> -            xmulctrl = MGA1064_MUL_CTL_15bits;
>> -        else
>> -            xmulctrl = MGA1064_MUL_CTL_16bits;
>> +    switch (format->format) {
>> +    case DRM_FORMAT_RGB565:
>> +        xmulctrl = MGA1064_MUL_CTL_16bits;
>>           break;
>> -    case 24:
>> +    case DRM_FORMAT_RGB888:
>>           xmulctrl = MGA1064_MUL_CTL_24bits;
>>           break;
>> -    case 32:
>> +    case DRM_FORMAT_XRGB8888:
>>           xmulctrl = MGA1064_MUL_CTL_32_24bits;
>>           break;
>>       default:
>>           /* BUG: We should have caught this problem already. */
>> -        drm_WARN_ON(dev, "invalid format depth\n");
>> +        drm_WARN_ON(dev, "invalid drm format\n");
>>           return;
>>       }
>> -    crtcext3 &= ~GENMASK(2, 0);
>> -    crtcext3 |= scale;
>> -
>>       WREG_DAC(MGA1064_MUL_CTL, xmulctrl);
>>       WREG_GFX(0, 0x00);
>> @@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device 
>> *mdev, const struct drm_format_in
>>       WREG_GFX(7, 0x0f);
>>       WREG_GFX(8, 0x0f);
>> +    /* scale is the number of bytes per pixels - 1 */
>> +    scale = format->cpp[0] - 1;
>> +
>> +    RREG_ECRT(3, crtcext3);
>> +    crtcext3 &= ~GENMASK(2, 0);
>> +    crtcext3 |= scale;
>>       WREG_ECRT(3, crtcext3);
>>   }
> 


-- 

Jocelyn